classification
Title: zlib.Decompress.decompress/flush do not raise any exceptions when given truncated input streams
Type: enhancement Stage: resolved
Components: Extension Modules Versions: Python 3.3, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: chortos, nadeem.vawda, petri.lehtinen, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2011-07-27 18:29 by chortos, last changed 2011-08-13 15:47 by nadeem.vawda. This issue is now closed.

Files
File name Uploaded Description Edit
zlib-fail.py chortos, 2011-07-27 18:29
zlib.Decompress.flush.patch chortos, 2011-07-27 21:07 review
zlibflushstrict.patch pitrou, 2011-08-02 15:38 review
zlib-eof.patch nadeem.vawda, 2011-08-06 15:22 review
Messages (11)
msg141257 - (view) Author: Oleg Oshmyan (chortos) Date: 2011-07-27 18:29
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.
        */
msg141275 - (view) Author: Oleg Oshmyan (chortos) Date: 2011-07-27 21:07
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.
msg141276 - (view) Author: Oleg Oshmyan (chortos) Date: 2011-07-27 21:16
> 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.
msg141461 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2011-07-31 10:37
Looking at Lib/test/test_zlib.py, it appears that this behaviour is intentional
(see issue8672). 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?
msg141570 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-02 15:38
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 :)
msg141571 - (view) Author: Oleg Oshmyan (chortos) Date: 2011-08-02 16:24
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.
msg141708 - (view) Author: Oleg Oshmyan (chortos) Date: 2011-08-06 05:20
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.
msg141713 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2011-08-06 09:24
> 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.
msg141719 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2011-08-06 15:20
Here's the patch.
msg142020 - (view) Author: Roundup Robot (python-dev) Date: 2011-08-13 13:28
New changeset bb6c2d5c811d by Nadeem Vawda in branch 'default':
Issue #12646: Add an 'eof' attribute to zlib.Decompress.
http://hg.python.org/cpython/rev/bb6c2d5c811d
msg142022 - (view) Author: Roundup Robot (python-dev) Date: 2011-08-13 13:45
New changeset 65d61ed991d9 by Nadeem Vawda in branch 'default':
Fix incorrect comment in zlib.Decompress.flush().
http://hg.python.org/cpython/rev/65d61ed991d9
History
Date User Action Args
2012-10-28 16:26:20nadeem.vawdalinkissue5210 superseder
2011-08-13 15:47:38nadeem.vawdasetstatus: open -> closed
type: behavior -> enhancement
resolution: fixed
stage: patch review -> resolved
2011-08-13 13:45:46python-devsetmessages: + msg142022
2011-08-13 13:28:42python-devsetnosy: + python-dev
messages: + msg142020
2011-08-08 07:34:15nadeem.vawdasetstage: patch review
2011-08-06 15:22:40nadeem.vawdasetfiles: - zlib-eof
2011-08-06 15:22:30nadeem.vawdasetfiles: + zlib-eof.patch
2011-08-06 15:20:52nadeem.vawdasetfiles: + zlib-eof

messages: + msg141719
2011-08-06 09:24:50nadeem.vawdasetmessages: + msg141713
2011-08-06 05:20:31chortossetmessages: + msg141708
2011-08-02 16:24:05chortossetmessages: + msg141571
2011-08-02 15:38:32pitrousetfiles: + zlibflushstrict.patch

messages: + msg141570
2011-07-31 10:37:57nadeem.vawdasetnosy: + pitrou
messages: + msg141461
2011-07-27 21:16:31chortossetmessages: + msg141276
2011-07-27 21:07:48chortossetfiles: + zlib.Decompress.flush.patch
keywords: + patch
messages: + msg141275
2011-07-27 20:27:24nadeem.vawdasetnosy: + nadeem.vawda
2011-07-27 19:00:51petri.lehtinensetnosy: + petri.lehtinen
2011-07-27 18:29:20chortoscreate