gh-156057: Wrap compression library exceptions in tarfile.ReadError - #156143
gh-156057: Wrap compression library exceptions in tarfile.ReadError#156143JMak-Security wants to merge 3 commits into
Conversation
…rror _FileInFile.read() (used by TarFile.extractfile() for seekable-mode archives) called the underlying compressed fileobj's read() directly. If a member's compressed payload was corrupted after the header had already been read successfully, the codec's own exception (zlib.error, OSError from bz2, lzma.LZMAError, or zstd.ZstdError) would leak straight through instead of being wrapped in tarfile.ReadError, unlike the existing streaming-mode (r|gz etc.) code path in _Stream, which already handles this correctly via a self.exception attribute set per codec. Give TarFile (and _FileInFile) the same per-codec exception attribute, set by gzopen/xzopen/zstopen to the specific error type (bz2 already matches the OSError class default), and wrap the read() call in _FileInFile with it. Verified by reproducing the leak against current main for gzip, bzip2, and xz (zstd untestable here, no compression.zstd module available in the local Python used to verify this), confirming the same input now raises tarfile.ReadError for all three, and running a full read/write/extract round-trip across all four compression modes to confirm no regression.
The check-warnings CI job correctly flagged :meth:TarFile.extractfile`n as unresolvable - Sphinx needs the fully qualified module.Class.method path for cross-file references. Use tarfile.TarFile.extractfile instead, matching how Doc/library/tarfile.rst defines it under the tarfile module.
Aniketsy
left a comment
There was a problem hiding this comment.
@JMak-Security thanks for the PR
Could you add tests for this ?
|
@Aniketsy, sure, I'll get back to you later when I add the tests. |
Adds direct unit tests for _FileInFile.read()'s new exception-wrapping behavior (both the positive case, wrapping the configured exception type into ReadError, and the negative case, letting an unconfigured exception type propagate unwrapped), plus an integration test confirming the real gzopen() -> ExFileObject -> _FileInFile wiring surfaces a zlib.error raised mid-extraction as tarfile.ReadError. Verified these tests fail against the pre-fix source (TypeError on the new exception= parameter for the unit tests; the raw zlib.error propagating uncaught for the integration test) and pass against the post-fix source. Signed-off-by: Jason Mak <squrrielbro@gmail.com>
|
@Aniketsy done! Added I verified these against both states of |
Fixes gh-156057.
The bug
TarFile.extractfile()reads a member's payload through_FileInFile.read(), which calls the underlying compressedfileobj.read()(agzip.GzipFile/bz2.BZ2File/lzma.LZMAFile/zstd.ZstdFilefor seekable-moder:gz/r:bz2/r:xz/r:zstarchives) with no exception handling. If the member's compressed data is corrupted after the header was already read successfully (rather than at initial archive-open time), the codec's own exception -zlib.error,OSErrorfrom bz2,lzma.LZMAError, orzstd.ZstdError- leaks straight through to the caller instead of being wrapped intarfile.ReadError.The streaming-mode (
r|gzetc.) code path already handles this correctly:_Streamsets aself.exceptionattribute to the specific codec's error type and catches it around its owndecompress()calls. The seekable-mode path via_FileInFilehad no equivalent.The fix
Give
TarFile(and_FileInFile) the same per-codecexceptionattribute pattern already used by_Stream:TarFile.exceptiondefaults toOSError(correct forbz2, and for uncompressedtarmode where it's simply unused).gzopen()/xzopen()/zstopen()set it to the specific type (zlib.error,LZMAError,ZstdError) after a successful open, mirroring what they already do fort._extfileobj.ExFileObject.__init__passestarfile.exceptionthrough to_FileInFile, which now wraps itsfileobj.read()call and re-raises astarfile.ReadError.Verification
Reproduced the leak against current
mainfor gzip, bzip2, and xz (couldn't test zstd locally - nocompression.zstdmodule in the Python available here) using a corrupted-mid-stream archive for each, confirming all three previously leaked the raw codec exception. Same test against the patched version confirms all three now raisetarfile.ReadErrorinstead. Also ran a full write/read/extract round-trip across all four compression modes (uncompressed, gzip, bzip2, xz) to confirm no regression in normal operation.