[Storage] Fix use-after-free in ProcessResp*Output; use ScratchBufferAllocator for API-returned PinnedSpanByte#1672
Merged
Conversation
badrishc
force-pushed
the
badrishc/fix-process-resp-output-use-after-free
branch
2 times, most recently
from
28f2280 to
227fef3
Compare
ProcessResp2ArrayOutput, ProcessResp3ArrayOutput, ProcessRespArrayOutputAsPairs, and ProcessRespSingleTokenOutput return PinnedSpanByte values pointing into IMemoryOwner<byte> buffers that are disposed in their finally blocks. Since the IMemoryOwner buffers from MemoryPool<byte>.Shared are not pinned (RespMemoryWriter unpins them on dispose), the returned PinnedSpanByte pointers are both dangling (due to dispose) and pointing to unpinned memory. This affects all callers that use new ObjectOutput() (which forces IMemory allocation): ZMPOP, LMPOP, BLMPOP, BZMPOP, and any custom procedures or Lua scripts calling HashGet, HashGetAll, HashGetMultiple, HashRandomField, SetMembers, SetPop, SortedSetPop, SortedSetRange, SortedSetIncrement, or ObjectScan through IGarnetApi. Fix: copy parsed data to scratchBufferBuilder (which uses GC.AllocateArray with pinned=true) inside the fixed block before the finally disposes the Memory. Uses single-allocation helpers to avoid scratch buffer reallocation. Co-authored-by: Copilot <[email protected]>
badrishc
force-pushed
the
badrishc/fix-process-resp-output-use-after-free
branch
from
227fef3 to
cbb558c
Compare
…or output - Add ScratchBufferAllocator parameter to StorageSession constructor, symmetric with ScratchBufferBuilder. All callers updated to pass an instance. - Fix GET(key, out PinnedSpanByte value) to copy output to SBA instead of SBB, so returned values remain valid across multiple API calls. - Update OutOfOrderFreeBuffer test to reflect that GET output no longer consumes SBB space. Co-authored-by: Copilot <[email protected]>
- Add GetScratchBufferAllocatorOffset() and ResetScratchBufferAllocator() to IGarnetApi, GarnetApi, and GarnetWatchApi (symmetric with existing SBB APIs but without offset param since SBA only supports full reset). - Make ScratchBufferAllocator.ScratchBufferOffset public. - Update LargeGet/LargeGetTxn to use SBA reset to prevent unbounded growth. - Update OutOfOrderFreeBuffer to test SBA offset advancement and reset. Co-authored-by: Copilot <[email protected]>
- Add ViewRemainingArgSlice to ScratchBufferAllocator for zero-copy GET - Fix GET to use SBA: zero-copy in common case (SpanByte), copy only on IMemory overflow - Fix InvokeCustomRawStringCommand and InvokeCustomObjectCommand to use SBA instead of SBB for returned output - Simplify IGarnetApi: remove GetScratchBufferOffset and ResetScratchBuffer(int offset), rename ResetScratchBufferAllocator to ResetScratchBuffer() (parameterless full reset) - Update tests for new API Co-authored-by: Copilot <[email protected]>
…torageSession Both are implementation details used only within libs/server/. SBB is used for temporary workspace (Lua, command input building). SBA is used for API-returned PinnedSpanByte values. Co-authored-by: Copilot <[email protected]>
…rBase Co-authored-by: Copilot <[email protected]>
Add outstandingSlices counter (DEBUG only) that asserts no more than one PinnedSpanByte slice is live at a time. Fires on CreateArgSlice and ExpandScratchBuffer when an unrewound slice exists. Add CreateArgSliceAsOffset API returning (offset, length) instead of PinnedSpanByte — safe for multiple calls since offsets survive reallocation. Fix missing rewinds caught by the new assert: - SortedSetIncrement: rewind incrSlice after RMW - KeyAdminCommands (RESTORE): rewind valArgSlice after SET_Conditional - CollectionItemBroker: rewind asKey in finally block - SortedSetRange LIMIT: single contiguous allocation instead of 3 separate - Lua AddKey: create+use+rewind per key iteration - ArgSliceVector: use CreateArgSliceAsOffset instead of CreateArgSlice Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…ice are immediate-use views Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a use-after-free / unpinned-pointer bug in RESP output parsing by ensuring PinnedSpanByte results no longer reference IMemoryOwner<byte> buffers that are disposed/unpinned after parsing. This primarily affects object-API callers that force heap IMemory allocation (e.g., via new ObjectOutput()), including custom procedures and Lua.
Changes:
- Copy parsed RESP outputs into pinned
ScratchBufferAllocatorstorage when output came from heapIMemoryOwner<byte>so returnedPinnedSpanByteremains valid. - Route string/object command outputs (e.g.,
GET, custom command invocations) throughScratchBufferAllocatorrather thanScratchBufferBuilderwhen results must survive disposal of heap-backed memory. - Simplify scratch-buffer API by removing offset-based reset from
IGarnetApiand adding safer builder patterns (CreateArgSliceAsOffset) plus DEBUG assertions to catch unsafe multi-slice usage.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Garnet.test/RespCustomCommandTests.cs | Updates custom-procedure tests to use the new scratch-buffer reset API. |
| libs/server/Storage/Session/StorageSession.cs | Threads ScratchBufferAllocator through StorageSession so parsing/copy helpers can allocate pinned storage. |
| libs/server/Storage/Session/ObjectStore/SortedSetOps.cs | Adjusts scratch usage (rewind / single-allocation LIMIT) to avoid pointer invalidation risks. |
| libs/server/Storage/Session/ObjectStore/Common.cs | Copies RESP-parsed PinnedSpanByte arrays/pairs/single tokens into pinned scratch allocation before disposing heap memory. |
| libs/server/Storage/Session/MainStore/MainStoreOps.cs | Makes GET output allocation use ScratchBufferAllocator (including “view remaining” + claim/copy behavior). |
| libs/server/Resp/RespServerSession.cs | Passes allocator into StorageSession construction. |
| libs/server/Resp/LocalServerSession.cs | Instantiates and passes a ScratchBufferAllocator for local sessions. |
| libs/server/Resp/KeyAdminCommands.cs | Rewinds scratch buffer slice used to build RESTORE value argument. |
| libs/server/Objects/ItemBroker/CollectionItemBroker.cs | Rewinds key slice allocated from scratchBufferBuilder after use. |
| libs/server/Lua/LuaRunner.cs | Rewinds temporary key slices after computing/storing key hashes for transactions. |
| libs/server/Databases/DatabaseManagerBase.cs | Creates allocator alongside builder for background/maintenance StorageSessions. |
| libs/server/Custom/CustomRespCommands.cs | Switches response formatting/copying to allocator-backed slices to avoid disposed heap memory. |
| libs/server/ArgSlice/ScratchBufferBuilder.cs | Adds DEBUG tracking/asserts for outstanding slices; replaces offset-reset API with offset-based slice creation helper. |
| libs/server/ArgSlice/ScratchBufferAllocator.cs | Adds ViewRemainingArgSlice to support “write into remaining space then claim” patterns. |
| libs/server/ArgSlice/ArgSliceVector.cs | Uses offset-based builder API to safely store many items without returning unstable pointers. |
| libs/server/API/IGarnetApi.cs | Replaces offset-based scratch buffer APIs with a single ResetScratchBuffer() method. |
| libs/server/API/GarnetWatchApi.cs | Updates wrapper to match new IGarnetApi.ResetScratchBuffer() API. |
| libs/server/API/GarnetApi.cs | Implements ResetScratchBuffer() by resetting the session’s ScratchBufferAllocator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The object store output path was copying to SBA but not disposing the IMemoryOwner, leaking pooled buffers. The raw string path already had the dispose. Found by GPT 5.4 review. Co-authored-by: Copilot <[email protected]>
badrishc
changed the title
Remove FormatScratchAsResp (both overloads), GetSliceFromTail, and GetRespFormattedStringLength from ScratchBufferBuilder — no callers. Rename scratchBufferManager parameter to scratchBufferBuilder in LuaRunner.Functions.cs for consistency. Co-authored-by: Copilot <[email protected]>
- Fix ScratchBufferAllocator.ViewRemainingArgSlice doc: 'ArgSlice' -> 'PinnedSpanByte' - LargeGet: validate HashGet result content, not just length - OutOfOrderFreeBuffer: validate GET results are non-empty and identical (verifies data integrity, not just pointer liveness) Co-authored-by: Copilot <[email protected]>
Document SBB vs SBA roles, rules for returning PinnedSpanByte, rewind discipline, IMemoryOwner disposal, and view API safety. Co-authored-by: Copilot <[email protected]>
Set test values to a verifiable pattern instead of all-zeros. Validate actual content in LargeGet (HashGet) and OutOfOrderFreeBuffer (GET) to catch data corruption from dangling pointers, not just empty results. Co-authored-by: Copilot <[email protected]>
badrishc
changed the title
Match SBB's 64-byte minimum to avoid excessive small pinned array allocations on first use. Applies to RespServerSession, LocalServerSession, and DatabaseManagerBase SBA instances. Co-authored-by: Copilot <[email protected]>
Set the default at the class level instead of at each call site. Co-authored-by: Copilot <[email protected]>
vazois
reviewed
Apr 6, 2026
Co-authored-by: Copilot <[email protected]>
vazois
reviewed
Apr 6, 2026
Clarify that SBB lays out data sequentially in one buffer and expansion invalidates pointers. Clarify that SBA maintains fragmented buffers with old ones kept rooted so pointers remain valid. Co-authored-by: Copilot <[email protected]>
vazois
reviewed
Apr 6, 2026
Test explicitly passes minSizeBuffer: 2 since it tests internal sizing logic that depends on the minimum buffer size. Co-authored-by: Copilot <[email protected]>
Previously minSizeBuffer was applied before RoundUpToPowerOf2, causing minSizeBuffer=64 with a 5-byte request to allocate 128 bytes (64+1 rounded up). Now the power-of-2 rounding is done on the actual request size first, then clamped to minSizeBuffer. A 5-byte request with minSizeBuffer=64 now allocates exactly 64 bytes. Co-authored-by: Copilot <[email protected]>
vazois
reviewed
Apr 7, 2026
vazois
approved these changes
Apr 7, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Root Cause
ProcessResp2ArrayOutput,ProcessResp3ArrayOutput,ProcessRespArrayOutputAsPairs, andProcessRespSingleTokenOutputinStorageSessionreturnPinnedSpanBytevalues pointing intoIMemoryOwner<byte>buffers fromMemoryPool<byte>.Shared. These buffers are not pinned —RespMemoryWriterpins them temporarily viaMemoryHandleduring write, but unpins on dispose. TheProcessResp*methods then dispose theIMemoryOwnerin theirfinallyblocks, returning dangling pointers into freed, unpinned memory.This only affects the
new ObjectOutput()code path (SpanByte invalidated, forcing IMemory allocation). The RESP handler path usesGetObjectOutput()backed by the pinned session network buffer, so it is unaffected.Same bug exists on
mainwithArgSlice/GarnetObjectStoreOutputtypes.Affected Commands
ZMPOP, LMPOP, BLMPOP, BZMPOP, and any custom procedures or Lua scripts calling
HashGet,HashGetAll,HashGetMultiple,HashRandomField,SetMembers,SetPop,SortedSetPop,SortedSetRange,SortedSetIncrement,GET, orObjectScanthroughIGarnetApi.Description of Change
Use existing
ScratchBufferAllocator(SBA) alongside existingScratchBufferBuilder(SBB) inStorageSession. SBA uses pinned arrays (GC.AllocateArray(_, true)) and keeps old buffers rooted, so returnedPinnedSpanBytevalues remain valid across multiple API calls within a batch.Key changes:
Common.cs): Copy parsed output to SBA insidefixedblock beforeIMemoryOwnerdisposalMainStoreOps.cs): Use SBAViewRemainingArgSlicefor zero-copy common case; copy on IMemory overflowCustomRespCommands.cs): All returnedPinnedSpanByteoutput uses SBA + proper Memory disposalStorageSession,DatabaseManagerBase,RespServerSession,LocalServerSessionIGarnetApi): RemovedGetScratchBufferOffset()andResetScratchBuffer(int), addedResetScratchBuffer()(full SBA reset)ScratchBufferBuilder.cs): Debug-onlyoutstandingSlicescounter asserts no more than one livePinnedSpanByteat a timeSortedSetIncrement,KeyAdminCommands,CollectionItemBroker; single contiguous allocation forSortedSetRange LIMIT; create+use+rewind for LuaAddKeyCreateArgSliceAsOffsetreturns(Offset, Length)instead ofPinnedSpanByte— safe for multi-alloc (used byArgSliceVector)scratchBufferBuilderandscratchBufferAllocatormarkedinternalonStorageSessionFormatScratchAsResp,GetSliceFromTail,ResetScratchBuffer(int)from SBBKey Technical Details
SBB vs SBA roles:
ScratchBufferBuilder— contiguous temporary workspace (Lua serialization, command input building with rewind). Single buffer, reallocates in place.ScratchBufferAllocator— persistent output storage for API-returnedPinnedSpanByte. Keeps old buffers rooted in a stack. Reset between batches.Affected types:
StorageSession— newscratchBufferAllocatorfieldIGarnetApi/GarnetApi/GarnetWatchApi— simplified scratch buffer APIScratchBufferBuilder— debug counter,CreateArgSliceAsOffset, doc comments on view APIs, removed unused APIsWhat NOT to Do
PinnedSpanBytefromScratchBufferBuilderfor values that must survive across multiple API calls — useScratchBufferAllocatorCreateArgSliceon SBB without rewinding before the nextCreateArgSlice— debug asserts enforce thisMemory.Dispose()after copying fromIMemoryOwnerto SBA — causes pool buffer leaksViewFullArgSlice/ViewRemainingArgSliceresults — they are immediate-use views invalidated by subsequent operations