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

Fix up some potential memory leaks in fwrite #6757

Merged
merged 12 commits into from
Jan 24, 2025
Merged

Conversation

MichaelChirico
Copy link
Member

Closes #6733.

Looked for any STOP or return before free() is called on any malloc() object, and made sure to free() before those exits if not already done & that object has successfully malloc()'d.

Also

  • Moved the #ifndef guards for NOZLIB outside the args.is_gzip checks, so that gzip-disabled installations don't even bother checking is_gzip (we error if is_gzip is attempted in gzip-disabled installations above)
  • Moved some DTPRINT() calls alerting about allocations to after we checked that the allocation succeeded (the error message is informative enough in case of failure)

Thanks @aitap for pointing out the mystream issue too. cc @philippechataignon.

@MichaelChirico MichaelChirico requested a review from aitap January 23, 2025 18:05
Copy link

github-actions bot commented Jan 23, 2025

Comparison Plot

Generated via commit 0a0bcfe

Download link for the artifact containing the test results: ↓ atime-results.zip

Task Duration
R setup and installing dependencies 4 minutes and 29 seconds
Installing different package versions 8 minutes and 26 seconds
Running and plotting the test cases 2 minutes and 36 seconds

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.62%. Comparing base (44a87a8) to head (799c3c5).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6757      +/-   ##
==========================================
- Coverage   98.62%   98.62%   -0.01%     
==========================================
  Files          79       79              
  Lines       14641    14635       -6     
==========================================
- Hits        14440    14434       -6     
  Misses        201      201              

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

src/fwrite.c Outdated Show resolved Hide resolved
@philippechataignon
Copy link
Contributor

A lot of allocation stuff in fwrite was written by Matt. I remove allocation for zlib streams and declare them as z_stream, not z_stream*. I've had an malloc(0) for freeing zbuffPool in all case (except if NOZLIB is defined).

MichaelChirico and others added 3 commits January 23, 2025 13:16
This isn't covered by the tests, but manually failing this allocation in
vgdb results in a leak otherwise:

268,096 (5,952 direct, 262,144 indirect) bytes in 1 blocks are definitely lost
in loss record 1,574 of 1,601
   at 0x48407B4: malloc (vg_replace_malloc.c:381)
   by 0x74ACD86: deflateInit2_ (in /lib/x86_64-linux-gnu/libz.so.1.2.13)
   by 0x90EA5232: init_stream (fwrite.c:576)
   by 0x90EA5EB0: fwriteMain (fwrite.c:806)
   by 0x90EA79EE: fwriteR (fwriteR.c:310)
Copy link
Contributor

@aitap aitap left a comment

Choose a reason for hiding this comment

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

Thank you! We needed those deflateEnds in addition to the frees. One remaining leak was visible through testing (zero-row fwrite) and one more leak on the error path is not easily testable but was confirmed manually in the debugger. There are no remaining complaints from LeakSanitizer.

src/fwrite.c Outdated Show resolved Hide resolved
src/fwrite.c Outdated Show resolved Hide resolved
@MichaelChirico MichaelChirico merged commit 79aed53 into master Jan 24, 2025
8 of 9 checks passed
@MichaelChirico MichaelChirico deleted the fwrite-leaks branch January 24, 2025 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak in fwrite
3 participants