-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-39039: tarfile raises descriptive exception from zlib.error #27766
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.
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
Conversation
* during tarfile parsing, a zlib error indicates invalid data * tarfile.open now raises a descriptive exception from the zlib error * this makes it clear to the user that they may be trying to open a corrupted tar file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. This will need some changes and a test.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@ambv I am having a hard time figuring out how to reproduce this other than just using the file originally attached on the bpo. See my comment on the bpo, and I appreciate if you can offer some help! https://bugs.python.org/issue39039 |
@ambv ok, I implemented all the suggested changes and also added a test. However, the test is obviously not good because I want the test to genuinely reproduce the bug condition by passing in a file which actually causes the zlib error to be raised. However, I can't do that because I don't really know how the file uploaded to the bpo can be reproduced. I also haven't gone through with investigating lzma or bz2 files. Partly because I can't figure out how to corrupt files in a way to elicit the problem response. I haven't been able to make lzma or bz2 archives that fit the edge case because I don't know how. I'm going to keep experimenting with this and try to learn more. If you or anyone else has any insight into how the corrupt bpo-uploaded file was created, I would appreciate any advice because I have had no luck. Also, note that the history of what I have tried and what I (don't) understand is written out here in a bit more detail: https://gist.github.com/jdevries3133/acbb5ba2a19093d3bcc214733ef85e5a |
Since Ethan is interested in this, I won't be approving or further requesting changes.
In this case leave this for a future improvement. Thanks for trying it out, Jack! |
except ImportError: | ||
raise e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Observation: this particular raise
will be pretty noisy since it will be a chained failure. But this is good, it's an unexpected issue for us and we want all the context we can get so that somebody might report a good bug to us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I figured. I could even do raise Exception('please submit a bug report!') from e
but maybe that is too much.
@ambv ok, I all the feedback has been addressed. Łukasz, if you do feel that there are more potential edge cases that I haven't been able to address, maybe you could summarize them on the bpo? The original poster of the bpo did add some additional context for how he made the file, so maybe in combination with that, others can look into further edge cases. For the bot: I have made the requested changes; please review again |
Thanks for making the requested changes! @ambv: please review the changes made to this pull request. |
This PR is stale because it has been open for 30 days with no activity. |
Thanks @jdevries3133 for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
Thanks @jdevries3133 for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
Sorry, @jdevries3133 and @ambv, I could not cleanly backport this to |
Sorry @jdevries3133 and @ambv, I had trouble checking out the |
pythonGH-27766) * during tarfile parsing, a zlib error indicates invalid data * tarfile.open now raises a descriptive exception from the zlib error * this makes it clear to the user that they may be trying to open a corrupted tar file (cherry picked from commit b6fe857) Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>
GH-28613 is a backport of this pull request to the 3.10 branch. |
…pythonGH-27766) * during tarfile parsing, a zlib error indicates invalid data * tarfile.open now raises a descriptive exception from the zlib error * this makes it clear to the user that they may be trying to open a corrupted tar file. (cherry picked from commit b6fe857) Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>
GH-28614 is a backport of this pull request to the 3.9 branch. |
GH-27766) (GH-28613) * during tarfile parsing, a zlib error indicates invalid data * tarfile.open now raises a descriptive exception from the zlib error * this makes it clear to the user that they may be trying to open a corrupted tar file (cherry picked from commit b6fe857) Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>
…GH-27766) (GH-28614) * during tarfile parsing, a zlib error indicates invalid data * tarfile.open now raises a descriptive exception from the zlib error * this makes it clear to the user that they may be trying to open a corrupted tar file. (cherry picked from commit b6fe857) Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>
|
|
|
|
|
corrupted tar file
I think that this small changes achieves a fix for the bpo. Tests still pass,
so that's a good sign that maybe this change does not introduce side-effects,
but I am hoping that someone with knowledge of this library help me think
of some edge cases to try out.
Please note that I am a relative novice developer, and while this patch is
good to the best of my understanding, my understanding is not the best!
https://bugs.python.org/issue39039