sqllogictest: account before alloc to avoid panic-after-alloc hazards#22742
Merged
avantgardnerio merged 1 commit intoJun 3, 2026
Merged
Conversation
`AccountingAllocator` forwarded every op to `inner` first and called `track` after. `track` can `panic_any` on overdraft, and the "inner first / account after" order produces two distinct hazards: 1. **`realloc`** — `inner.realloc` frees the caller's old pointer on success. If `track` then panics, unwind starts with the caller's `Vec` (or similar growing container) still holding the freed old pointer; the next `Drop` produces glibc's `double free or corruption (out)` and SIGABRT, masking the underlying untracked-allocation panic the framework was trying to surface. 2. **`alloc` / `alloc_zeroed`** — `inner.alloc` succeeds and bytes are physically allocated. If `track` then panics, no caller ever receives the pointer → unwind leaks the very bytes that pushed the bank over budget — the opposite of what the kill panic is for. `dealloc` is safe: it's a credit only, and `track` short-circuits on `delta >= 0` before any panic decision. Reorder all three ops: `track(delta)` first, forward to `inner` second. On overdraft we panic before touching `inner`, so the caller's pointer (`alloc`/`alloc_zeroed`: never returned, never owned; `realloc`: still valid old pointer) is in a consistent state for unwind. If `inner` then returns null we refund the delta so the bank stays consistent. Manually verified with a `GroupedHashAggregateStream` + `List<Utf8>` group-key reproducer that routes through `GroupValuesRows::intern` → `RowConverter::append` → `Vec::resize` → `__rust_realloc`: - before: exit 101, `double free or corruption (out)`, signal 6 SIGABRT - after: exit 1, clean `allocator overdraft` panic at the same `RowConverter::append` stack frame. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
avantgardnerio
force-pushed
the
brent/rowconverter-append-untracked-alloc
branch
from
624b8cd to
d8cc8c3
Compare
avantgardnerio
changed the title
andygrove
approved these changes
Jun 3, 2026
Member
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
Makes sense. Thanks @avantgardnerio
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
2 participants
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.
Which issue does this PR close?
Follow-up defect fix in the allocator-level accounting framework added by #22626.
Rationale for this change
AccountingAllocator::realloc(introduced in #22626) calledinner.reallocfirst — which frees the caller's old pointer on success — then calledtrack, which canpanic_anyon overdraft. The unwind starts with the caller'sVec(or similar growing container) still holding the now-freed old pointer; the nextDropproduces glibc'sdouble free or corruption (out)and SIGABRT, masking the underlying untracked-allocation panic the framework was trying to surface.The same hazard does not apply to
alloc/alloc_zeroed/dealloc:alloc/alloc_zeroed— the caller has no preexisting pointer to be invalidated; panicking after the inner alloc just leaks the new allocation, which is fine on the kill path.dealloc— credits the bank, never panics.Only
reallochas a live caller-side pointer that gets invalidated by the inner operation before the panic decision.What changes are included in this PR?
Reorder
AccountingAllocator::reallocto account first, forward toinner.reallocsecond:track(delta)first — on overdraft we panic with the caller's pointer still valid; unwind drops the live container cleanly, no abort.inner.reallocreturns null after a successful track, refund the delta so the bank stays consistent with what actually got allocated.Are these changes tested?
Manually verified using a
GroupedHashAggregateStream+List<Utf8>group-key reproducer (routes throughGroupValuesRows::intern→RowConverter::append→Vec::resize→__rust_realloc, which is one of the untracked-allocation sites this framework is meant to catch):double free or corruption (out), signal 6 SIGABRTallocator overdraft: account balance at panic = -1.7 MBat the sameRowConverter::appendstack frameA regression test for "panic from inside
reallocdoes not double-free" would require constructing the precise realloc-mid-grow scenario plus acatch_unwindharness; deferred as follow-up.Are there any user-facing changes?
No. Confined to
sqllogictest'smemory-accountingfeature, which is internal CI tooling.