zlib.Decompress.decompress/flush do not raise any exceptions when given truncated input streams

[chortos]

BPO	12646
Nosy	@pitrou, @akheron
Files	zlib-fail.py zlib.Decompress.flush.patch zlibflushstrict.patch zlib-eof.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 2011-08-13.15:47:38.673>
created_at = <Date 2011-07-27.18:29:20.599>
labels = ['extension-modules', 'type-feature']
title = 'zlib.Decompress.decompress/flush do not raise any exceptions when given truncated input streams'
updated_at = <Date 2011-08-13.15:47:38.672>
user = 'https://bugs.python.org/chortos'

bugs.python.org fields:

activity = <Date 2011-08-13.15:47:38.672>
actor = 'nadeem.vawda'
assignee = 'none'
closed = True
closed_date = <Date 2011-08-13.15:47:38.673>
closer = 'nadeem.vawda'
components = ['Extension Modules']
creation = <Date 2011-07-27.18:29:20.599>
creator = 'chortos'
dependencies = []
files = ['22777', '22782', '22828', '22847']
hgrepos = []
issue_num = 12646
keywords = ['patch']
message_count = 11.0
messages = ['141257', '141275', '141276', '141461', '141570', '141571', '141708', '141713', '141719', '142020', '142022']
nosy_count = 5.0
nosy_names = ['pitrou', 'nadeem.vawda', 'chortos', 'python-dev', 'petri.lehtinen']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue12646'
versions = ['Python 2.7', 'Python 3.2', 'Python 3.3']

[chortos]

If a truncated input stream is given to the zlib.decompress function, it raises a zlib.error. However, if the same stream is fed to a zlib.Decompress object, no exception is raised during the entire lifetime of the object. Attached is an example demonstrating the discrepancy.

zlib.decompress raises this exception when it gets a Z_BUF_ERROR from zlib, while the implementation of zlib.Decompress.decompress just skips every Z_BUF_ERROR it gets as described in this (apparently inaccurate) comment:
/* We will only get Z_BUF_ERROR if the output buffer was full
but there wasn't more output when we tried again, so it is
not an error condition.
*/

[chortos]

I believe the attached patch fixes this problem, making zlib.Decompress.flush() raise the exception raised by zlib.decompress().

In the same patch, I also took the opportunity to correct a wrong comment in the implementation of flush() and change the error messages given by zlib.{De,C}ompress.flush() on {in,de}flateEnd() errors to the more end-user-friendly ones given in the same occasions by zlib.{de,}compress(). If this does not sound like a good thing to do, feel free (whoever ends up committing this) to remove these changes.

One uncomfortable issue I see with the patch is that zlib.Decompress.flush() now potentially gives an error message with Z_OK as the error code, but unless I misunderstand the comments in the real zlib’s zlib.h and that never happens (I was unable to produce a situation that would cause this), the only other options are faking another error code and setting an exception message whose format is different from all other exceptions raised by the zlib module.

[chortos]

faking another error code

Actually, I think another call to inflate(), which would be valid at that point, would just return the other error code, so it can as well be faked.

[nadeemvawda]

Looking at Lib/test/test_zlib.py, it appears that this behaviour is intentional
(see bpo-8672). I agree that having flush() raise an exception is the Right
Thing, but breaking existing code (which we know depends on this behavior) is
clearly a bad idea. @pitrou - anything thoughts on this?

[pitrou]

I'm not a zlib specialist, but I think what this means is that the stream is not finished, but it's valid anyway.

For example, you get the same behaviour by doing:

c = zlib.compressobj()
s = c.compress(b'This is just a test string.')
s += c.flush(zlib.Z_FULL_FLUSH)

The resulting bytestring is a non-terminated zlib stream. It still decompresses to the original data fine.

I think the appropriate fix is to add an argument to flush(). Here is a patch, I named the argument "strict" by lack of imagination :)

[chortos]

I like the new patch, but shouldn’t the default be to behave the same way zlib.decompress() behaves, i. e. raise? (Or perhaps zlib.decompress() should be modified not to raise instead. I’m just aiming at consistency.) Of course this will break code that relies on the old behaviour but the fix (adding strict=False) is trivial.

[chortos]

I have another proposition (as an alternative). The new _bz2.BZ2Decompressor objects have an attribute called eof which is False until the end of the stream is read. The same attribute could be added to zlib.Decompress objects.

[nadeemvawda]

I have another proposition (as an alternative). The new _bz2.BZ2Decompressor
objects have an attribute called eof which is False until the end of the
stream is read. The same attribute could be added to zlib.Decompress objects.

+1, I like the idea of being consistent across related modules.
I'll put together a patch.

[nadeemvawda]

Here's the patch.

[python-dev]

New changeset bb6c2d5c811d by Nadeem Vawda in branch 'default':
Issue bpo-12646: Add an 'eof' attribute to zlib.Decompress.
http://hg.python.org/cpython/rev/bb6c2d5c811d

[python-dev]

New changeset 65d61ed991d9 by Nadeem Vawda in branch 'default':
Fix incorrect comment in zlib.Decompress.flush().
http://hg.python.org/cpython/rev/65d61ed991d9