Fix ZSTD_DCtx leak when a frame fails to decode - #148
Open
Watson1978 wants to merge 1 commit into
Open
Conversation
Zstd.decompress creates a ZSTD_DCtx and frees it after decode_one_frame
returns, but decode_one_frame raises whenever libzstd reports an error, so the
free is skipped and the context is lost. libzstd sizes the context's inBuff and
outBuff from the frame header before decoding any block, so the leak carries
those buffers with it -- and the header is attacker-supplied, which is what
decides how big they are.
Measured with a valid header followed by a body that fails to decode: 200 such
calls grow RSS by ~435 MB, about 2.2 MB per call, and it does not come back.
A header declaring the maximum default window leaks far more.
Run the decode under rb_ensure so the context is freed on every path. This also
covers the scratch buffer, which leaked too if rb_str_cat raised while
appending output. decode_one_frame now owns the context it is given, so
rb_decompress no longer frees it. set_decompress_params still frees the context
itself before raising, so it has to run before the ensure takes ownership --
otherwise the two would double free.
The new spec only walks the failure path; it asserts the raise, not the leak.
Under `rake spec:valgrind` that path reports
189,976 (95,976 direct, 94,000 indirect) bytes in 1 blocks are definitely lost
malloc
*ZSTD_createDCtx (at .../zstdruby.so)
*rb_decompress (at .../zstdruby.so)
before this change, and nothing after it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Zstd.decompresscreates aZSTD_DCtxand frees it afterdecode_one_framereturns, butdecode_one_frameraises whenever libzstd reports an error, so the free is skipped and the context is lost.The size matters because libzstd allocates the context's
inBuffandoutBufffrom the frame header, before decoding any block. The header is attacker-supplied, so the caller decides how much is leaked per failed call — up to the default window cap.Measured with a valid header followed by a body that fails to decode: 200 such calls grow RSS by ~435 MB, about 2.2 MB per call, and it does not come back. A header declaring the maximum default window leaks far more. Anything that decompresses untrusted bytes can be walked into OOM by repeating a single malformed request.
Fix
Run the decode under
rb_ensure, so the context is freed on every path. This also covers the scratch buffer, which leaked too ifrb_str_catraised while appending output.decode_one_framenow owns the context it is given, sorb_decompressno longer frees it. One ordering detail is load-bearing:set_decompress_paramsfrees the context itself before raising, so it has to run before theensuretakes ownership — otherwise the two would double free.Tests
The new spec only walks the failure path and asserts the raise; it does not try to measure memory. The leak detection belongs to
rake spec:valgrind(#147), which reports this on the path the spec exercises:With this change that report is gone and the task exits 0.
Compatibility
No API or behavior change. The same inputs raise the same errors; only the memory that was previously abandoned is now released.
Note on overlap
This touches the same loop in
decode_one_frameas #143, so whichever lands first will leave the other with a textual conflict. They are independent changes and I am happy to rebase this one on top of that.🤖 Generated with Claude Code