Skip to content

Fix integer overflow in gz_compress_mmap#2110

Merged
Dead2 merged 1 commit intozlib-ng:developfrom
phprus:issue-2109
Jan 21, 2026
Merged

Fix integer overflow in gz_compress_mmap#2110
Dead2 merged 1 commit intozlib-ng:developfrom
phprus:issue-2109

Conversation

@phprus
Copy link
Copy Markdown
Contributor

@phprus phprus commented Jan 17, 2026

Fix for issue #2109

Added a 2GB (INT_MAX) mmap I/O limit.

@phprus phprus mentioned this pull request Jan 17, 2026
Closed
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Jan 17, 2026 •
edited
Loading

Walkthrough

Reworked mmap-based compression paths in test/minigzip.c and test/fuzz/fuzzer_minigzip.c: switched buffer types to void*/size_t, added robust overflow checks against PTRDIFF_MAX, replaced gzwrite with gzfwrite, updated mmap error checks to MAP_FAILED, and ensured proper stream closing and return values.

Changes

Cohort / File(s) Summary
mmap/type-safety & validation
test/minigzip.c, test/fuzz/fuzzer_minigzip.c		 Changed mmap buffer/type from char*/off_t to void*/size_t, added overflow protection (check sb.st_size vs PTRDIFF_MAX and reject non-positive/oversized sizes), use NULL for mmap addr and check MAP_FAILED.
I/O write path and control flow
test/minigzip.c, test/fuzz/fuzzer_minigzip.c		 Replaced gzwrite with gzfwrite using (buf, 1, buf_len, out) and size_t length; tightened comparison to len != buf_len; added gzclose(out) and return Z_OK.

Possibly related issues

Suggested labels

bug

Suggested reviewers

  • nmoinvaz
  • Dead2
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically identifies the main change: fixing integer overflow in gz_compress_mmap.
Description check ✅ Passed The description relates to the changeset by referencing issue #2109 and mentions the 2GB mmap I/O limit that is implemented in the code changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 17, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.29%. Comparing base (fb9a6ce) to head (1bafdcf).

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #2110      +/-   ##
===========================================
- Coverage    84.31%   83.29%   -1.03%     
===========================================
  Files          158      158              
  Lines        13085    13089       +4     
  Branches      3222     3225       +3     
===========================================
- Hits         11033    10902     -131     
- Misses         989     1129     +140     
+ Partials      1063     1058       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mtl1979
Copy link
Copy Markdown
Collaborator

mtl1979 commented Jan 18, 2026 •
edited
Loading

Like I mentioned in the issue, gzip file format doesn't support files that are 2 GB or larger due to file systems using signed 32-bit integers for file size in stat structure on 32-bit operating systems without large file support.

gzip silently truncates file size after checksum in the end of the stream to modulo 2^32. Adding support for larger files would require using one of the reserved bits in the gzip header to signify new file format version that increase the size field to 8 bytes. This obviously breaks compatibility with older versions of both gzip and minigzip.

It's debatable if we should gracefully fail both when using mmap and when not using.

CodeCov failure is due to not being in sync with current development tree, currently 19 commits behind.

nmoinvaz
nmoinvaz reviewed Jan 19, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@nmoinvaz nmoinvaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what we should be using is:

// Instead of:
len = PREFIX(gzwrite)(out, (char *)buf, (unsigned)buf_len);

// Use:
len = PREFIX(gzfwrite)((char *)buf, 1, buf_len, out);

f prefix doesn't imply FILE* rather it was part of a set of changes that utilized size_t.

@phprus
Fix integer overflow in gz_compress_mmap
1bafdcf 
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
@phprus
Copy link
Copy Markdown
Contributor Author

phprus commented Jan 20, 2026

@mtl1979

You're right.
But I think work compression and truncated file size in gzip trailer is better than unexpected reading just part of the input file.

Code without MMAP is also susceptible to the gzip format issue.

@nmoinvaz

I changed the gz_compress function so that it now uses the PREFIX(gzfwrite).

nmoinvaz
nmoinvaz approved these changes Jan 20, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@nmoinvaz nmoinvaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

mtl1979
mtl1979 approved these changes Jan 20, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@mtl1979 mtl1979 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Dead2 Dead2 merged commit 51ec71d into zlib-ng:develop Jan 21, 2026
170 of 173 checks passed
@Dead2 Dead2 mentioned this pull request Jan 30, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nmoinvaz nmoinvaz nmoinvaz approved these changes

@Dead2 Dead2 Dead2 approved these changes

+1 more reviewer

@mtl1979 mtl1979 mtl1979 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Warnings fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@phprus @mtl1979 @nmoinvaz @Dead2