This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Thread locks in zlib module may go wrong in rare case
Type: behavior Stage: commit review
Components: Library (Lib) Versions: Python 3.10, Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: gregory.p.smith, lukasz.langa, malin, miss-islington
Priority: normal Keywords: patch

Created on 2020-09-07 03:14 by malin, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
test_flush.py malin, 2021-11-17 04:05
Pull Requests
URL Status Linked Edit
PR 22126 merged malin, 2020-09-07 03:17
PR 22130 merged malin, 2020-09-07 13:47
PR 22132 merged malin, 2020-09-07 13:53
PR 29587 merged malin, 2021-11-17 03:31
PR 29588 merged malin, 2021-11-17 03:41
PR 29811 merged miss-islington, 2021-11-27 00:18
PR 29812 closed miss-islington, 2021-11-27 00:21
Messages (9)
msg376473 - (view) Author: Ma Lin (malin) * Date: 2020-09-07 03:14
The code in zlib module:

    self->zst.next_in = data->buf;  // set next_in
    ...
    ENTER_ZLIB(self);   // acquire thread lock

`self->zst` is a `z_stream` struct defined in zlib, used to record states of a compress/decompress stream:

    typedef struct z_stream_s {
        Bytef    *next_in;  /* next input byte */
        uInt     avail_in;  /* number of bytes available at next_in */
        uLong    total_in;  /* total number of input bytes read so far */

        Bytef    *next_out; /* next output byte will go here */
        uInt     avail_out; /* remaining free space at next_out */
        uLong    total_out; /* total number of bytes output so far */
        
        ... // Other states
    } z_stream;

Setting `next_in` before acquiring the thread lock may mix up compress/decompress state in other threads.

Moreover, modify `ENTER_ZLIB` macro, don't release the GIL when the thread lock can be acquired immediately. This behavior is the same as the bz2/lzma modules.
msg392045 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-04-27 08:37
Thanks! ✨ 🍰 ✨
msg392049 - (view) Author: Ma Lin (malin) * Date: 2021-04-27 09:03
Thanks for review.
msg406447 - (view) Author: Ma Lin (malin) * Date: 2021-11-17 04:05
Sorry, I found an omission.

The previous PRs fixed the bug in these methods:

    zlib.Compress.compress()
    zlib.Decompress.decompress()

This method also has this bug, fix in PR29587 (main/3.10) and PR29588 (3.9-):

    zlib.Decompress.flush()
    
Attached file `test_flush.py` can reliably reproduce the bug.

This time I carefully checked bz2/lzma/zlib modules, it should be no problem anymore.

Gregory P. Smith should understand these codes, add him to nosy list.
msg407115 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-11-27 00:18
New changeset 7edb6270a78c695e4c2ae2432797dc18105374fc by Ma Lin in branch 'main':
bpo-41735: Fix thread lock in zlib.Decompress.flush() may go wrong (GH-29587)
https://github.com/python/cpython/commit/7edb6270a78c695e4c2ae2432797dc18105374fc
msg407116 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-11-27 00:21
New changeset 86c1265cdc64030c8921e0da5fcae2ac64299c26 by Ma Lin in branch '3.9':
[3.9] bpo-41735: Fix thread lock in zlib.Decompress.flush() may go wrong (GH-29588)
https://github.com/python/cpython/commit/86c1265cdc64030c8921e0da5fcae2ac64299c26
msg407119 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-11-27 00:24
Thanks malin!
msg407122 - (view) Author: miss-islington (miss-islington) Date: 2021-11-27 00:42
New changeset 57100c86baa8451a568348646834380cd425b858 by Miss Islington (bot) in branch '3.10':
[3.10] bpo-41735: Fix thread lock in zlib.Decompress.flush() may go wrong (GH-29587) (GH-29811)
https://github.com/python/cpython/commit/57100c86baa8451a568348646834380cd425b858
msg407132 - (view) Author: Ma Lin (malin) * Date: 2021-11-27 04:14
Thanks for review!
History
Date User Action Args
2022-04-11 14:59:35adminsetgithub: 85901
2021-11-27 04:14:33malinsetmessages: + msg407132
2021-11-27 00:42:08miss-islingtonsetmessages: + msg407122
2021-11-27 00:24:26gregory.p.smithsetstatus: open -> closed
resolution: fixed
messages: + msg407119

stage: patch review -> commit review
2021-11-27 00:21:30miss-islingtonsetpull_requests: + pull_request28044
2021-11-27 00:21:30gregory.p.smithsetmessages: + msg407116
2021-11-27 00:18:27miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request28043
2021-11-27 00:18:25gregory.p.smithsetmessages: + msg407115
2021-11-17 19:12:23gregory.p.smithsetassignee: gregory.p.smith
resolution: later -> (no value)
stage: resolved -> patch review
2021-11-17 04:05:37malinsetstatus: closed -> open
files: + test_flush.py

nosy: + gregory.p.smith
messages: + msg406447

resolution: fixed -> later
2021-11-17 03:41:00malinsetpull_requests: + pull_request27831
2021-11-17 03:31:40malinsetpull_requests: + pull_request27830
2021-04-27 09:03:07malinsetmessages: + msg392049
2021-04-27 08:37:33lukasz.langasetstatus: open -> closed

nosy: + lukasz.langa
messages: + msg392045

resolution: fixed
stage: patch review -> resolved
2020-09-07 13:53:26malinsetpull_requests: + pull_request21213
2020-09-07 13:47:13malinsetpull_requests: + pull_request21211
2020-09-07 03:17:21malinsetkeywords: + patch
stage: patch review
pull_requests: + pull_request21208
2020-09-07 03:14:46malincreate