Skip to content

gh-107398: Fix tarfile stream mode exception when process the file with extra header data #126304

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

gh-107398: Fix tarfile stream mode exception when process the file with extra header data #126304

wants to merge 8 commits into from

Conversation

@Zheaoli
Copy link
Contributor

@Zheaoli Zheaoli commented Nov 1, 2024 •
edited by bedevere-app bot
Loading

@Zheaoli Zheaoli requested a review from ethanfurman as a code owner November 1, 2024 18:03
@bedevere-app bedevere-app bot mentioned this pull request Nov 1, 2024
Open
Eclips4
Eclips4 reviewed Nov 1, 2024
View reviewed changes
@Zheaoli
Copy link
Contributor Author

Zheaoli commented Nov 4, 2024

@serhiy-storchaka PTAL when you got time(

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Nov 22, 2024

@serhiy-storchaka ping (

@serhiy-storchaka serhiy-storchaka self-requested a review November 22, 2024 14:30
serhiy-storchaka
serhiy-storchaka reviewed Nov 22, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some refactoring, but except a struct.error the new code looks equivalent to the old one. Could you please point what was changed in the behavior?

Lib/tarfile.py Outdated
if self.__read(2) != b"\037\213":
raise ReadError("not a gzip file")
if self.__read(1) != b"\010":
(method, flag, _) = struct.unpack("<BBIxx", self.__read(8))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen if it read less than 8 bytes?

Why use I and then ignore the result?

serhiy-storchaka
serhiy-storchaka reviewed Nov 22, 2024
View reviewed changes
Lib/tarfile.py
xlen = ord(self.__read(1)) + 256 * ord(self.__read(1))
self.read(xlen)
extra_len, = struct.unpack("<H", self.__read(2))
self.__read(extra_len)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the only relevant change is in this line.

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Nov 26, 2024

Hi @serhiy-storchaka Thanks for the review!

  1. You are right, the core change is self.__read(extra_len) so we can skip the header correctly
  2. I prefer use the struct.pack instead of cast the binary offset manually. I think this will make the code more readable and keep same style with gzip.py

@Zheaoli
Copy link
Contributor Author

Zheaoli commented May 22, 2025

@serhiy-storchaka PTAL when you get time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@Eclips4 Eclips4 Eclips4 left review comments

@ethanfurman ethanfurman Awaiting requested review from ethanfurman ethanfurman is a code owner

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tarfiles can't open tgz files with gzip features like FEXTRA & FCOMMENT when mode='r|*'

3 participants

@Zheaoli @serhiy-storchaka @Eclips4