Limit decompressed data when reading from LZMAFile and BZ2File

[vadmium]

BPO	23529
Nosy	@pitrou, @vadmium, @serhiy-storchaka, @wm75
Files	LZMAFile.v2.patch LZMAFile-etc.v3.patch: LZMAFile and BZ2File done; no open(buffering) LZMAFile-etc.v4.patch: LZMAFile, BZ2File, GzipFile done LZMAFile-etc.v5.patch LZMAFile-etc.v6.patch: Dropped buffer_size parameter LZMAFile-etc.v7.patch LZMAFile-etc.v8.patch LZMAFile-etc.v9.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2015-04-10.22:33:50.397>
created_at = <Date 2015-02-26.11:06:26.181>
labels = ['type-feature', 'library']
title = 'Limit decompressed data when reading from LZMAFile and BZ2File'
updated_at = <Date 2015-04-10.22:33:50.396>
user = 'https://github.com/vadmium'

bugs.python.org fields:

activity = <Date 2015-04-10.22:33:50.396>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2015-04-10.22:33:50.397>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2015-02-26.11:06:26.181>
creator = 'martin.panter'
dependencies = []
files = ['38245', '38367', '38397', '38514', '38619', '38674', '38686', '38733']
hgrepos = ['301']
issue_num = 23529
keywords = ['patch']
message_count = 23.0
messages = ['236663', '236747', '236862', '236902', '237424', '237586', '238070', '238124', '238250', '238372', '238675', '238742', '238773', '239195', '239215', '239245', '239297', '239369', '239370', '239449', '239562', '240453', '240454']
nosy_count = 7.0
nosy_names = ['pitrou', 'Arfrever', 'nikratio', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'wolma']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue23529'
versions = ['Python 3.5']

[vadmium]

This is a follow-on from bpo-15955, which has added low-level support for limiting the amount of data from the LZMA and bzip decompressors. The high-level LZMAFile and BZ2File reader APIs need to make use of the new low level max_length parameter.

I am starting off with a patch for LZMAFile, based on a patch I posted to bpo-15955. I split out a _RawReader class that does the actual decompress() calls, and then wrapped that in a BufferedReader. The LZMAFile then just delegates the read methods to the BufferedReader. This avoids needing any special code to implement buffering, readline(), etc. This involved some changes in the API though:

• LZMAFile now uses BufferedReader.peek(). The current implementation seems appropriate, but I am not comfortable with the current specification in the documentation, which says it is allowed to not return any useful data. See bpo-5811.
• read() now accepts size=None, because BufferedReader does. I had to change a test case for this.
• BufferedReader.seek() raises a different exception for invalid “whence”

Once the LZMAFile patch sees a bit of review and looks like it might be acceptable, I plan to change the BZ2File class in a similar manner.

[pitrou]

LZMAFile now uses BufferedReader.peek(). The current implementation seems appropriate, but I am not comfortable with the current specification in the documentation, which says it is allowed to not return any useful data.

What do you mean with "useful data"? peek() should always return at least one byte (except on EOF or on non-blocking streams, of course).

[vadmium]

## BufferedReader.peek() ##
See <https://bugs.python.org/issue5811#msg233750\>, but basically my concern is that the documentation says “the number of bytes returned may be less or more than requested”, without any mention of EOF or other conditions.

## Buffer sizing ##
In the code review, Nikolaus raised the idea of allowing a custom “buffer_size” parameter for the BufferedReader. I think this would need a bit of consideration about how it should work:

1. Should it be a direct wrapper around BufferedReader(buffer_size=...)?
2. Should it also support an unbuffered reader mode like open(buffering=0), socket.makefile(buffering=0), and subprocess.Popen(bufsize=0)?
3. Should there be any consideration for buffering in write mode (mirroring the equivalent open(), etc parameters)?

## Common raw decompression stream class ##
Having a common base class mapping the generic decompressor object API to the RawIOBase API is a good thing. I will try to make one in my next patch iteration. In the mean time, here are a couple of issues to consider:

• What module should it reside in? Perhaps “gzip”, because it is the most commonly-used of the three decompressors? Perhaps “io”, being the most relevant common dependency? Or a brand new internal module?!
• The zlib.decompressobj() API is slightly different, since it has the “unconsumed_tail” attribute and flush() methods, and decompress(max_length=0) does not mean return zero bytes (bpo-23200).

In bpo-23528, Nikolaus also pointed out that “GzipFile would need to additionally overwrite read() and write() in order to handle the CRC and gzip header.”

[nikratio]

On Feb 27 2015, Martin Panter <report@bugs.python.org> wrote:

In the code review, Nikolaus raised the idea of allowing a custom
“buffer_size” parameter for the BufferedReader. I think this would
need a bit of consideration about how it should work:

1. Should it be a direct wrapper around
   BufferedReader(buffer_size=...)?

I'm not 100% sure what you mean by that, but I think the answer is Yes.

2. Should it also support an unbuffered reader mode like
   open(buffering=0), socket.makefile(buffering=0), and
   subprocess.Popen(bufsize=0)?

Yes, in that case the DecompressReader should simply be used without
encapsulating it in BufferedReader.

3. Should there be any consideration for buffering in write mode
   (mirroring the equivalent open(), etc parameters)?

I don't think so. There's not much to be gained by buffering data before
passing it to the compressor, so the only point of buffering is to avoid
short writes (or reads in case of decompressing). But the low-level
compressor code already avoids short writes.

Best,
-Nikolaus

--
GPG encrypted emails preferred. Key id: 0xD113FCAC3C4E599F
Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

         »Time flies like an arrow, fruit flies like a Banana.«

[vadmium]

I am posting LZMAFile-etc.v3.patch, where I have implemented a “buffer_size” parameter to the buffered LZMAFile etc classes. I have not implemented open(buffering=...) for the time being (which should probably delegate to the buffer_size parameter or return a raw _DecompressReader object, at least for read mode). Other changes:

• Restored the _MODE_WRITE = 3 value
• Explained the _pos and _size attributes in _DecompressReader
• Factored out decomp_factory and args, decomp_error parameters to make _DecompressReader generic
• BZ2File modified similarly to LZMAFile
• I removed the deprecated and unused BZ2File(buffering=...) parameter; buffer_size takes its place. The old buffering parameter appears to have been documented, but never implemented in C Python, so hopefully this is not a big deal.

Still to do: Need to find a better home for the _DecompressReader and _BaseStream classes. Currently it lives in “lzma”, but apparently it is possible for any of the gzip, bz2, lzma modules to not be importable, so it would have to live elsewhere. Possible options are the io module, or a brand new internal module (e.g. Lib/_compression.py). Thoughts?

Also I am about to see if I can make GzipFile use the _DecompressReader class. I will have to add GzipFile(buffer_size=...) as a keyword-only parameter since the third parameter position is already taken. There are quite a few quirks with gzip and zlib compared to bz2 and lzma, so I will see how I go.

[vadmium]

I have decided not to reuse the same _DecompressReader for the "gzip" module. This is because the code for parsing the "gzip" header pulls in data by calling read(), and I would have to adapt this code to accept whatever data is pushed to it via a decompress() method. However I have rewritten the “gzip” module to use BufferedReader, removing its own readline() etc implementations.

I have split the BaseStream and DecompressReader classes into a new file called Lib/_compression.py. Let me know if there is anything else to be done to add a new file in the Lib/ directory, or if you have any better ideas where these classes should live.

Other changes in the new patch (LZMAFile-etc.v4.patch):

• Fix documented GzipFile.peek(n) signature to match implementation
• Removed unused and inconsistent code paths in gzip._PaddedFile
• New code assumes zlib.decompressobj().flush() returns a limited amount of data, since it has no “max_length” parameter. In reality I think flush() does not do anything at all and should be deprecated or removed; see bpo-23200.

I hope that the patch does not need any more major changes, so it should be ready for review. There are a few more suggested enhancements that I have not implemented. While they would be nice, I would prefer to handle them in separate bugs and patches. I believe strengthening Python against gzip bombs is important, and any extra refactoring of the code will probably make things harder to review, more likely to break, and less likely to make it into 3.5.

Some possible enhancements I did not do:

• Support open(buffering=...) parameter, passing through to the [...]File(buffer_size=...) parameter
• open(buffering=0) returning an unbuffered reader object, probably just a direct DecompressReader instance
• detach() method on the BufferedIOBase file classes
• Further factoring out of a common CompressedFile base class
• Apply the buffer size parameter to write mode
• Rewrite the gzip module to use the common DecompressReader base class

[nikratio]

On Mar 06 2015, Martin Panter <report@bugs.python.org> wrote:

Still to do: Need to find a better home for the _DecompressReader and
_BaseStream classes. Currently it lives in “lzma”, but apparently it
is possible for any of the gzip, bz2, lzma modules to not be
importable, so it would have to live elsewhere.

Yes.

Possible options are
the io module, or a brand new internal module
(e.g. Lib/_compression.py). Thoughts?

I think a new internal module would be the right choice, but I don't
know what needs to be done to properly add it to the build system. So
for now I'd just put it in the io module and wait for a core committer
to complain :-).

Best,
-Nikolaus

GPG encrypted emails preferred. Key id: 0xD113FCAC3C4E599F
Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

         »Time flies like an arrow, fruit flies like a Banana.«

[vadmium]

Well my last patch just added the _compression.py file without doing anything special, and it seems to be installed properly with “make install” so I will assume nothing else needs to be done.

[vadmium]

Posting LZMAFile-etc.v5.patch with the following changes:

• Merged with current code
• Changed BZ2File(buffer_size=...) to a keyword-only parameter and restored previous unused “buffering” parameter. Also changed the LZMAFile parameter to keyword-only for consistency.
• Dropped documenting how buffer_size affects the compressed data chunk size or the fast-forward chunk size for seeking
• Dropped backslash from \* in function signatures
• Documented and tested that buffer_size=0 is not valid for the LZMAFile etc classes
• Clarified a few code comments

There is still Nikolaus’s concern about setting the buffer size to zero and doing short reads, discussed on Rietveld. Apart from that, I think this patch addresses the rest of the comments. Let me know if I missed something!

[nikratio]

If you want to add support for buffer_size=0 in a separate patch/issue I think that's fine. But in that case I would not add a buffer_size parameter now at all. IMO, not having it is better as having it but not supporting zero (even if it's documented that's pretty surprising).

[wm75]

The patch has a huge block replacement in the gzip.py module starting at GzipFile.write, which makes it large and hard to identify changes. Could that be ameliorated or is it too much work right now?