Skip to content

rax.c: add OOM guard in raxRemove for incomplete ancestor stack#580

Open
ft-ioxcs wants to merge 1 commit into
antirez:mainfrom
ft-ioxcs:fix/rax-remove-oom-guard
Open

rax.c: add OOM guard in raxRemove for incomplete ancestor stack#580
ft-ioxcs wants to merge 1 commit into
antirez:mainfrom
ft-ioxcs:fix/rax-remove-oom-guard

Conversation

@ft-ioxcs

Copy link
Copy Markdown

Summary

raxRemove calls raxLowWalk with a stack to track ancestor nodes, but raxLowWalk discards the return value of raxStackPush. If a stack heap-expansion fails (tree depth > 32, malloc(512)), ts.oom is set but the ancestor is silently dropped. raxRemove then proceeds to mutate the tree and call raxRemoveCleanup with the incomplete stack, which pops wrong ancestors and can pass a non-child to raxRemoveChild. That function has an unbounded while(1) scan documented as UB. The fix adds a ts.oom check immediately after raxLowWalk returns, matching the pattern already used in raxSeek at line 1966.

Files changed: 1 file, +4 lines.

Causal Chain

Step Line What happens
Root cause raxLowWalk:613 raxStackPush(ts,h) return value discarded. On OOM, ts->oom = 1 but the ancestor node is not pushed.
Trigger raxRemove:1284 Passes &ts to raxLowWalk — the only mutating caller that supplies a real stack.
Propagation (inline leaf) raxRemove:1298 Calls raxRemoveCleanup(rax, h, &ts, &trycompress) before the ts.oom guard at 1331.
Propagation (normal) raxRemove:1321 Same, for the non-inline-leaf deletion path.
Existing guard (too late) raxRemove:1331 if (trycompress && ts.oom) trycompress = 0; — only suppresses recompression; cleanup has already run.
Corruption raxRemoveCleanup:~1243 h = raxStackPop(ts) — with a truncated stack, pops the wrong ancestor or NULL.
Crash / UB raxRemoveChild:1217-1226 while(1) { ... cp++; } — scans past the child-array boundary when child is not in the wrong parent.
Documented UB raxFindParentLink:1128-1129 Comment: "otherwise the operation is an undefined behavior (it will continue scanning the memory without any bound checking)." — same unbounded pattern.

Correct Pattern (already in this file)

raxSeek:1966 checks ts.oom immediately after its own raxLowWalk call:

size_t i = raxLowWalk(it->rt, ele, len, &it->node, &parentlink,
                      &splitpos, &it->stack, &inline_leaf);

/* Return OOM on incomplete stack info. */
if (it->stack.oom) return 0;

raxRemove at line 1284 simply omits this check.

Impact

The stack static buffer holds 32 entries (RAX_STACK_STATIC_ITEMS in rax.h:151). Trees deeper than 32 nodes trigger a heap expansion (malloc(512) on first expansion). If that allocation fails — under system OOM, custom allocator policy, container memory limits, or setrlimit — the stack is silently incomplete. Depending on tree shape:

  • Best case: raxRemoveCleanup pops NULL and dereferences h->iskey at line 1246 (immediate segfault).
  • Worst case: cleanup operates on wrong-but-valid nodes, freeing nodes that should remain in the tree, leaving dangling parent pointers. raxRemove returns 1 (success) while the tree is silently corrupted.

Verification

A fault-injection test was built using RAX_MALLOC_INCLUDE (supported at rax.c:41-45) to substitute a custom allocator that can be instructed to fail. The test:

  1. Builds a 40-node key-bearing radix tree chain (depth > 32, forcing heap stack expansion).
  2. Arms the fault injector to fail the next malloc call.
  3. Calls raxRemove on the deepest key.

Without the fix: raxRemove returns 1 (claims success), tree size and key presence are corrupted.
With the fix: raxRemove returns 0, tree is unchanged, all keys still present.

A subsequent normal raxRemove (no fault) on the same tree confirms the non-OOM path is unaffected.

make ds4_test (which links rax.o) builds cleanly with the patch applied — no regressions.

raxLowWalk:613 discards raxStackPush return value. If the stack
heap-expansion fails (depth > 32, malloc(512)), ts.oom is set
but the ancestor is silently dropped. raxRemove then calls
raxRemoveCleanup before the existing ts.oom guard at line 1331
(which only suppresses recompression, not cleanup). The
incomplete stack causes raxRemoveCleanup to pop wrong ancestors,
potentially passing a non-child to raxRemoveChild, whose
while(1) scan reads past the child array (UB, per the comment
at raxFindParentLink:1128).

Fix: check ts.oom immediately after raxLowWalk, matching the
existing pattern at raxSeek:1966. Fault-injection test confirms:
without fix, raxRemove returns 1 (false success) with corrupted
tree; with fix, returns 0, tree intact.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant