Skip to content
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

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

Closed
chortos mannequin opened this issue Jul 27, 2011 · 11 comments
Closed
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@chortos
Copy link
Mannequin

chortos mannequin commented Jul 27, 2011

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
    Copy link
    Mannequin Author

    chortos mannequin commented Jul 27, 2011

    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 chortos mannequin added type-bug An unexpected behavior, bug, or error extension-modules C modules in the Modules dir labels Jul 27, 2011
    @chortos
    Copy link
    Mannequin Author

    chortos mannequin commented Jul 27, 2011

    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
    Copy link
    Mannequin Author

    chortos mannequin commented Jul 27, 2011

    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
    Copy link
    Mannequin

    nadeemvawda mannequin commented Jul 31, 2011

    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
    Copy link
    Member

    pitrou commented Aug 2, 2011

    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
    Copy link
    Mannequin Author

    chortos mannequin commented Aug 2, 2011

    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
    Copy link
    Mannequin Author

    chortos mannequin commented Aug 6, 2011

    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
    Copy link
    Mannequin

    nadeemvawda mannequin commented Aug 6, 2011

    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
    Copy link
    Mannequin

    nadeemvawda mannequin commented Aug 6, 2011

    Here's the patch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 13, 2011

    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
    Copy link
    Mannequin

    python-dev mannequin commented Aug 13, 2011

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

    @nadeemvawda nadeemvawda mannequin closed this as completed Aug 13, 2011
    @nadeemvawda nadeemvawda mannequin added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Aug 13, 2011
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant