Repository navigation
Fix NULL deref when CCtx workspace reservation fails - #4863
jianzhuguo wants to merge 1 commit into
Conversation
Check ZSTD_cwksp_reserve_failed() after every reserve in ZSTD_resetCCtx_internal / ZSTD_reset_matchState before the pointer is used (especially before ZSTD_memset), and before marking the CCtx initialized. Zero-byte reserves return NULL without setting allocFailed, so the sticky reserve_failed flag is the correct failure signal. Adds tests/wksp_alloc_fail_test.c: a custom-allocator sweep that forces reservation failures and asserts clean ERROR(memory_allocation) returns. Related: meta#3950
|
Hi @jianzhuguo! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Fix NULL deref when CCtx workspace reservation fails
Summary
ZSTD_resetCCtx_internal()andZSTD_reset_matchState()callZSTD_cwksp_reserve_*()and store the results without checking for reservation failure before using the pointers.When a workspace reservation fails (typical trigger: CCtx resize under memory pressure —
ZSTD_cwksp_createwith a newneededSpaceafterworkspaceTooSmall/workspaceWasteful), those pointers stayNULL, but the function still:memseting further bufferszc->initialized = 10(success)The next compression then writes through the NULL pointers. The most visible crash site is
ZSTD_seqToCodes()viaseqStore.llCode/mlCode/ofCode:Some reservation sites are immediately followed by
ZSTD_memset()on the reserved pointer (ldmState.hashTable,ldmState.bucketOffsets, CDicttagTable), so they crash insideZSTD_resetCCtx_internalitself, before any later check could run.Crash signature
EXCEPTION_ACCESS_VIOLATION(Windows0xc0000005) / SIGSEGVmemory allocation of N bytes failednearby) when the reservation failed because the process was under allocation pressureZSTD_seqToCodes,ZSTD_compressBlock_*, or theZSTD_memsetontagTable/bucketOffsets)Related: #3950 reports a segfault at the same crash site (
ZSTD_resetCCtx_internal→ seqStore code tables). That report attributes it to uninitialized workspace contents and acallocworkaround. This patch addresses the distinct case where the reservation itself fails and is not checked, leaving NULL pointers. Both issues can produce crashes at the same call site; this fix is independent of initialization.How to reproduce (sketch)
ZSTD_createCCtx_advanced) whosecustomAllocreturns NULL after a budget is exhausted (or wrapmallocwith a fail-Nth counter).ZSTD_resetCCtx_internalto rebuild the workspace (vary block size /pledgedSrcSize/ compression level between calls soworkspaceTooSmallorworkspaceWastefulis hit).llCode/tagTableis NULL.ZSTD_compress*returnsERROR(memory_allocation).Fix
For every
ZSTD_cwksp_reserve_*in the CCtx / match-state / CDict setup path:ZSTD_cwksp_reserve_failed(ws)before the pointer is used (especially beforeZSTD_memset)ERROR(memory_allocation)on failureZSTD_cwksp_reserve_failed(ws)check beforeinitialized = 1, so any missed site still fails cleanlyWhy
reserve_failedand notptr == NULL:ZSTD_cwksp_reserve_internal()returnsNULLfor a zero-byte reserve without settingallocFailed(e.g.hashTable3whenhashLog3 == 0). A bareptr == NULLcheck would treat those legitimate zero-size reserves as failures and reject every compress. The sticky flag is only set on a real reservation failure.Sites covered
ZSTD_initStaticCCtxprevCBlock,nextCBlock,tmpWorkspace(fixed-size objects;ptr == NULLis correct here)ZSTD_reset_matchStatetagTable(both branches —memsetin CDict path)ZSTD_resetCCtx_internalsequencesStart,ldm hashTable,ldmSequences,extSeqBuf,litStart,inBuff,outBuff,ldm bucketOffsets,llCode,mlCode,ofCode+ sticky catch-allZSTD_initCDict_internalentropyWorkspaceZSTD_reset_matchStatealready had sticky checks after the table reserves and at the end; thetagTablepath needed an immediate check because of thememset.Testing
A dedicated regression test is included (see
tests/wksp_alloc_fail_test.cin the attachment / follow-up commit). It plugs a customZSTD_customMemwhosecustomAlloccan fail the Nth allocation or enforce a byte budget, then compresses while sweeping those failure points.Coverage:
test_control_successtest_sweep_alloc_indexZSTD_resetCCtx_internaltest_sweep_byte_budgetallocFailedpath across multiple reservestest_tag_table_row_match_finderZSTD_fast/dfast/greedy/lazystrategies →tagTable+memsetpathtest_opt_parser_workspacelitFreq, …) inZSTD_reset_matchStatetest_reuse_rebuildpledgedSrcSizeforces workspace rebuild; fail mid-resettest_cdict_alloc_failZSTD_createCDict_advancedwith failing allocator (entropyWorkspace, match state)Contract asserted: a failed reservation must produce a clean zstd error (
ZSTD_isError, preferablyZSTD_error_memory_allocation) or a successful compress when the failed allocation was unused — never a crash.Verified locally against zstd 1.5.7 + this patch (MSVC x64): all cases pass. Without the patch, the same test crashes (access violation) at the sites above.
Happy to fold the test into
tests// the existing fuzzer harness in whatever form the maintainers prefer.Notes
ERROR(memory_allocation). Callers that already handle zstd errors are unaffected.ZSTD_cwksp_reserve_failed()is sticky (allocFailedflag), so the catch-all also covers reserves insideZSTD_reset_matchState.