Fix use-after-free of the CDict/DDict referenced by a stream - #146
Open
Watson1978 wants to merge 1 commit into
Open
Fix use-after-free of the CDict/DDict referenced by a stream#146Watson1978 wants to merge 1 commit into
Watson1978 wants to merge 1 commit into
Conversation
ZSTD_CCtx_refCDict and ZSTD_DCtx_refDDict only borrow the pointer they are
given; zstd.h states the dictionary "must outlive its usage within CCtx".
StreamingCompress and StreamingDecompress keep the context in their struct,
but nothing kept the Ruby CDict/DDict object reachable, so a dictionary the
caller did not store was collected while the context still pointed at it.
stream = Zstd::StreamingCompress.new(dict: Zstd::CDict.new(bytes, 5))
# nothing references the CDict any more
stream << data # reads freed memory
Store the dictionary VALUE in each struct with RB_OBJ_WRITE and handle it in
the mark and compact callbacks, next to the buffers already tracked there.
set_compress_params/set_decompress_params now return the dictionary object so
the streaming initializers can retain it; the one-shot Zstd.compress and
Zstd.decompress paths ignore it, since their dictionary stays reachable from
the caller's frame for the whole call. A String dictionary needs no handling
either way: ZSTD_CCtx_loadDictionary copies it (ZSTD_dlm_byCopy).
Confirmed with AddressSanitizer. Before this change both new specs report
heap-use-after-free -- reading the CDict in ZSTD_CCtx_init_compressStream2 and
the DDict in ZSTD_decompressBegin_usingDDict, both freed by ZSTD_freeCDict /
ZSTD_freeDDict from the GC's free callback. After it the full suite runs clean
under ASan.
The existing dictionary specs hold the dictionary in a `let`, which keeps it
reachable, so they never exercised this; the new ones deliberately keep no
reference.
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_CCtx_refCDictandZSTD_DCtx_refDDictonly borrow the pointer they are given —zstd.hsays the dictionary "must outlive its usage within CCtx".StreamingCompressandStreamingDecompresskeep the context in their struct for the life of the object, but nothing kept the RubyCDict/DDictobject reachable, so a dictionary the caller did not store was collected while the context still pointed at it.The freed allocation holds the dictionary's entropy tables and its
dictContentpointer and size. Once the slot is reused — a same-sized Ruby String is enough — those become whatever now occupies that memory, which is why this is a memory-safety bug and not only a crash.Verification
Built with
-fsanitize=addressand run underLD_PRELOAD=$(gcc -print-file-name=libasan.so).Before this change, both new specs report
heap-use-after-free:In both traces the free comes from the GC's own free callback, so the dictionary is being collected exactly as expected — it simply has no owner. After this change the full suite runs clean under ASan, with zero AddressSanitizer reports.
Scope
Only the streaming classes are affected. The one-shot
Zstd.compressandZstd.decompresspaths borrow a dictionary too, but it stays reachable from the caller's frame for the whole call — including acrossrb_thread_call_without_gvl, since the GC scans the machine stack of every thread — so they need no ownership and are left alone.A
Stringdictionary needs no handling on either path:ZSTD_CCtx_loadDictionaryandZSTD_DCtx_loadDictionarypassZSTD_dlm_byCopy, so libzstd owns its own copy.set_compress_params/set_decompress_paramsnow return the dictionary object so the streaming initializers can retain it. Their existing callers ignore the return value and are otherwise unchanged.Why the existing specs did not catch this
The dictionary specs — including the ones that already call
GC.compact— hold the dictionary in alet, which keeps it reachable for the whole example. The two new specs deliberately keep no reference to it, then runGC.start/GC.compactand allocate into the freed slot before using the stream.Compatibility
No API or behavior change. The dictionary object is now reachable from the stream, so it is collected after the stream instead of possibly before it.
🤖 Generated with Claude Code