Repository navigation
Free the compression context when Zstd.compress raises - #162
Open
sribalakumar wants to merge 1 commit into
Open
sribalakumar wants to merge 1 commit into
sribalakumar wants to merge 1 commit into
Conversation
Zstd.compress frees its ZSTD_CCtx only after compression returns normally, so any exception raised in between abandons the context. That includes interrupts: zstd_compress releases the GVL, and when it reacquires it Ruby delivers pending interrupts (Thread#raise, Timeout, Thread#kill, Sidekiq shutdown) by raising out of the call. An interrupted level-19 compression of a 32 MiB input leaks roughly 75-85 MB per call. The same applies to argument errors raised after the context exists: an unknown keyword or a level that does not fit in an int leaked the context from both Zstd.compress and StreamingCompress#initialize. Run the body of Zstd.compress under rb_ensure so the context is freed on every path. set_compress_params and convert_compression_level used to free the context themselves before raising; with an ensure also owning it that would be a double free, so they now raise and leave it to the owner. StreamingCompress#initialize therefore assigns sc->ctx before calling set_compress_params, so the object's free callback owns it. This mirrors the decompression-side change in SpringMT#148.
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.
This fixes the compress half of #160; the decompress half is handled separately by #148. It mirrors #148's approach, applied to the compress side.
Changes:
Zstd.compressnow runs its body underrb_ensure, so theZSTD_CCtxis freed on every path — normal return, argument errors, and interrupts.set_compress_paramsandconvert_compression_levelused to free the context themselves before raising. With anrb_ensurealso owning it that would double-free, so they now just raise and leave the context to its owner. (convert_compression_level's now-unusedctxparameter is dropped.)StreamingCompress#initializenow assignssc->ctxbefore callingset_compress_params, so the object's free callback owns the context if it raises.New specs (6 examples):
In
spec/zstd-ruby_spec.rb, under "when it raises after creating the compression context":ArgumentErrorfor an unknown keywordRangeErrorfor a level that does not fit in an intArgumentErrorfor adict:that is neither aCDictnor aStringThread#raisewhile compressingIn
spec/zstd-ruby-streaming-compress_spec.rb, under "initialize raising after creating the compression context":ArgumentErrorfor an unknown keywordArgumentErrorfor adict:that is neither aCDictnor aStringLeaks aren't visible from Ruby, so these specs pass both before and after this change on their own — what turns a leak into a failing test is the repo's
rake spec:valgrind. That's how the before/after below was produced.Before/after:
rake spec:valgrindLinux, Docker, ruby 3.4, run on just these 6 examples (
SPEC_OPTS='-e "creating the compression context"'), "before" being current main's C code plus the new specs.Before: 6 examples, 0 failures, but Valgrind fails the task with three "definitely lost" records:
...followed by: "Valgrind reported errors (e.g. memory leak or use-after-free)"
After: 6 examples, 0 failures, Valgrind clean, task exits 0.
Before/after: macOS
leaks --atExitRUBY_FREE_AT_EXIT=1, 20 calls per scenario (baseline from Ruby itself is 1 leak / 192 bytes):Zstd.compresslevel 19 of 32 MiB, interruptedZstd.compressunknown keywordZstd.compresslevel:2**40StreamingCompress.new(unknown: 1)Zstd.compress/StreamingCompress.newwithdict: 123Full suite: 97 examples, 0 failures.
This composes with #148: the two branches merge cleanly, and with both applied the suite passes (99 examples, 0 failures) with every compress and decompress leak scenario back to baseline. With both applied, the full
rake spec:valgrindrun is also clean — 99 examples, 0 failures, no Valgrind errors — so the Valgrind job that has been failing on main since #152 would go green.No API or behaviour change — the same inputs raise the same errors as before.