bpo-45509: Check gzip headers for corrupted fields by rhpvorderman · Pull Request #29028 · python/cpython · GitHub

Check whether the COMMENT, NAME and HCRC fields are correct.

EDIT: Additionally, slightly increase performance for header checking for headers produced by gzip.compress or zlib.compress.

https://bugs.python.org/issue45509

This PR is stale because it has been open for 30 days with no activity.

Try to use a bytearray instead of io.BytesIO. Would it be faster? In what cases?

Performance improvement was necessary (see bug report). So I made it better the following way:

When the data is read from fp a bytes object is created. Instead of passing it anonymously to the functions that performs the check it is saved as a local variable.
The FHCRC flag is checked and the result is saved as fhcrc, so the check runs only once.
When fhcrc is truthy, a header_parts list is created.
Every time fp is read and fhcrc is truthy the already created bytes objects are appended to the header_parts list. List appending has a very low overhead. It simply expands an array of PyObject pointers. Pointers that we already have as the named bytes objects.
When the header should be checked, a b"".join(header_parts) is called to recreate the header.

This means for the most common use cases (no flags or only FNAME set) that only a few extra truthy checks are performed. This comes at a very low cost of 3% (see the bug report for results).

This is worth it since it enables correct truncation checking for FNAME and FCOMMENT as well as enabling checking of the header CRC when FHCRC is set.

@serhiy-storchaka Thank you for taking a look and thank you for your insights. I have updated the PR accordingly. It is much better now in terms of performance.

Call the bool method and cache the result for faster truth checking. Do not test for empty bytes but use "not magic" instead for faster truth checking.

I made a small tweek that shaves off another 2 microseconds

Use if not magic instead of if magic == b''
Use fhcrc = bool(flag & FHCRC) instead of fhcrc = flag & FHCRC

Those are: + Only FNAME set. (Created by gzip and python's GzipFile) + No flags set. (Created by gzip.compress and zlib.compress with wbits=31)

With further performance tweaks, reading the gzip header is now either faster or neutral compared to the most common flags (no flags, or only FNAME set). See bpo-45509 for more performance details.

The performance tweaks are:

Check if flags == 0. If so return last_mtime immediately and skip any further processing.
Check if flags == FNAME. If so read until the NULL byte terminating the name and return last_mtime. Skip any further processing.
In all other cases. Use a slower code path were all parts of the header are saved for header crc checking. (Even if FHCRC is not set!). A single code path for all other cases is used here to keep the code simple(r). Since these cases will be rare, it makes no sense complicating the code for an optimal code path where parts of the header are only saved if FHCRC is set.

@serhiy-storchaka The PR is ready for re-review when you have the time. The performance issue has been addressed.

All commit authors signed the Contributor License Agreement.

This PR is stale because it has been open for 30 days with no activity.

I got a message that this branch is stale. So I rechecked the code again. It's been a few years.

I think it comes down whether CPython wants to comply to RFC1952 or not. This PR makes the header parsing spec compliant. In practice header CRCs are mostly not used. So the other angle is to argue that having less code to maintain is more important than to the letter spec compliance. My view on this is that this code can live untouched for decades to come (just like the old code was left untouched for at least a decade). So I'd be inclined to merge it.

Remove double import of zlib

Thank you for your patience @rhpvorderman. I am not very happy that the code becomes more complicated, but checking the CRC of the header if it is provided is the right thing. You have made considerable efforts to minimize the cost. I am inclined to accept this PR.

Yet one question. Did you consider to use a bytearray instead of a list to accumulate the header data?

Hi. Thank you for your suggestions. I have factored out the reading until NULL to make the code more simple and use a bytearray as suggested. This means there is a small increase in cost when only the FNAME flag is set, but the code is simpler. Technically that code could also be replaced with the original code for less simplicity but more speed.

With these changes I think the number of extra lines are minimized (+23) and readability is hopefully not aversely affected.

What if pass the bytearray buffer as argument to _read_until_null()? Would not it be faster?

Yes. That makes more sense since it is a one-off function.

Okay I fiddled a bit with it. Should be as close to optimal now as possible. If no flags or if flag==FNAME the function exits early and with minimal investment.
If the flags are more complex, the header is saved for CRC checking. Only one bytesarray object is created (unfortunately it will result in a few PyMem_Realloc calls as the header is resized, but that is inevitable).

If people get really stressed out about the performance, they should probably be using python-zlib-ng and python-isal anyway. There this whole machinery is written in C.

Thanks for reviewing and providing feedback. It is always a nice learning opportunity to contribute back to CPython!

I did check the BytesIO and Bytearray code to see what would be more costly, but they use the same memory amortization techniques so it probably does not matter. I did find the += of bytearray to be more visually appealing so I opted for that as you suggested.

In python-isal and python-zlib-ng I use a sort of buffered structure where the whole header is in the buffer already. The CRC check is just checking the buffer a few bytes back rather than having to save everything in a separate object. That only works by virtue of it being written in C though, so not really viable here.

Thanks @rhpvorderman for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.15.
🐍🍒⛏🤖

GH-149769 is a backport of this pull request to the 3.15 branch.

…H-149769) Check the header checksum it the HCRC field is present. (cherry picked from commit dd94457) Co-authored-by: Ruben Vorderman <r.h.p.vorderman@lumc.nl>

bedevere-bot added the awaiting review label Oct 18, 2021

the-knights-who-say-ni added the CLA signed label Oct 18, 2021

rhpvorderman changed the title bpo: 45509: Check gzip headers for corrupted fields bpo-45509: Check gzip headers for corrupted fields Oct 18, 2021

github-actions Bot added the stale Stale PR or inactive for long period of time. label Nov 18, 2021

serhiy-storchaka reviewed Nov 22, 2021

View reviewed changes

rhpvorderman force-pushed the bpo-45509 branch 2 times, most recently from 1f308a4 to 453ba11 Compare November 23, 2021 06:15

Check gzip headers for corrupted fields

617e064

rhpvorderman force-pushed the bpo-45509 branch from 453ba11 to 617e064 Compare November 23, 2021 06:28

github-actions Bot removed the stale Stale PR or inactive for long period of time. label Nov 24, 2021

Minor performance tweaks to _read_gzip_header

3d5bb47

Call the bool method and cache the result for faster truth checking. Do not test for empty bytes but use "not magic" instead for faster truth checking.

Optimize _read_gzip_header for the most common code paths

e68e76e

Those are: + Only FNAME set. (Created by gzip and python's GzipFile) + No flags set. (Created by gzip.compress and zlib.compress with wbits=31)

rhpvorderman mannequin mentioned this pull request Apr 10, 2022

Gzip header corruption not properly checked. #89672

Closed

ezio-melotti removed the CLA signed label Jul 13, 2022

rhpvorderman mentioned this pull request Oct 17, 2022

Give python-isal a mention in the zlib/gzip documentation #98347

Closed

Merge branch 'main' into bpo-45509

2dfc96c

serhiy-storchaka self-requested a review January 30, 2023 15:24

serhiy-storchaka added the type-feature A feature request or enhancement label Jan 30, 2023

github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 9, 2026

Merge branch 'main' into bpo-45509

65108b6

Update test_gzip.py

64ad8e9

Remove double import of zlib

github-actions Bot removed the stale Stale PR or inactive for long period of time. label May 11, 2026

Merge branch 'main' into bpo-45509

a8fe591

serhiy-storchaka reviewed May 11, 2026

View reviewed changes

rhpvorderman added 3 commits May 12, 2026 16:14

Factor out reading until null and use a bytearray

4d13ece

Simplify comments. Better grouping of statements

6ea0f0e

Fix error in method

4d0f01d

serhiy-storchaka reviewed May 12, 2026

View reviewed changes

rhpvorderman added 2 commits May 13, 2026 11:40

Do not store header for common FNAME flag

2fb1ce0

Reduce number of object creations

ae60e94

serhiy-storchaka approved these changes May 13, 2026

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting review labels May 13, 2026

serhiy-storchaka added the needs backport to 3.15 pre-release feature fixes, bugs and security fixes label May 13, 2026

serhiy-storchaka enabled auto-merge (squash) May 13, 2026 10:16

serhiy-storchaka merged commit dd94457 into python:main May 13, 2026
57 of 58 checks passed

bedevere-app Bot removed the awaiting merge label May 13, 2026

bedevere-app Bot removed the needs backport to 3.15 pre-release feature fixes, bugs and security fixes label May 13, 2026

+              def _read_until_null(fp):
+                  '''Read until the first encountered null byte in fp'''
+                  result = io.BytesIO()

bpo-45509: Check gzip headers for corrupted fields by rhpvorderman · Pull Request #29028 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-45509: Check gzip headers for corrupted fields - #29028

bpo-45509: Check gzip headers for corrupted fields#29028serhiy-storchaka merged 12 commits intopython:mainpython/cpython:mainfrom rhpvorderman:bpo-45509rhpvorderman/cpython:bpo-45509Copy head branch name to clipboard

Conversation

rhpvorderman commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Nov 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

rhpvorderman Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhpvorderman commented Nov 23, 2021

Uh oh!

rhpvorderman commented Nov 24, 2021

Uh oh!

rhpvorderman commented Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhpvorderman commented Nov 29, 2021

Uh oh!

python-cla-bot Bot commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 9, 2026

Uh oh!

rhpvorderman commented Apr 10, 2026

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

rhpvorderman commented May 12, 2026

Uh oh!

serhiy-storchaka May 12, 2026

Choose a reason for hiding this comment

Uh oh!

rhpvorderman May 13, 2026

Choose a reason for hiding this comment

Uh oh!

rhpvorderman commented May 13, 2026

Uh oh!

rhpvorderman commented May 13, 2026

Uh oh!

Uh oh!

miss-islington-app Bot commented May 13, 2026

Uh oh!

bedevere-app Bot commented May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-45509: Check gzip headers for corrupted fields#29028
serhiy-storchaka merged 12 commits into
python:mainfrom
rhpvorderman:bpo-45509

rhpvorderman commented Oct 18, 2021 •
edited
Loading

rhpvorderman Nov 23, 2021 •
edited
Loading

rhpvorderman commented Nov 24, 2021 •
edited
Loading

python-cla-bot Bot commented Apr 18, 2025 •
edited
Loading