From d8cc8c324329f67ce859457ae1a88ee70e06f50e Mon Sep 17 00:00:00 2001 From: Brent Gardner Date: Wed, 3 Jun 2026 13:27:16 -0600 Subject: [PATCH] sqllogictest: account before alloc to avoid panic-after-alloc hazards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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` 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) --- datafusion/sqllogictest/src/accounting.rs | 35 +++++++++++++++++------ 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/datafusion/sqllogictest/src/accounting.rs b/datafusion/sqllogictest/src/accounting.rs index 7514d73571084..46b6120c24d28 100644 --- a/datafusion/sqllogictest/src/accounting.rs +++ b/datafusion/sqllogictest/src/accounting.rs @@ -276,11 +276,17 @@ impl AccountingAllocator { unsafe impl GlobalAlloc for AccountingAllocator { unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + // Account BEFORE the inner alloc. If we panicked AFTER `inner.alloc` + // succeeded, the bytes are physically allocated but no caller ever + // sees the pointer → unwind leaks the very bytes that pushed us + // over the budget — the opposite of what the kill panic is for. + let delta = -(layout.size() as isize); + track(delta); // SAFETY: layout is forwarded unchanged. let ptr = unsafe { self.inner.alloc(layout) }; - if !ptr.is_null() { - // Allocation debits the bank. - track(-(layout.size() as isize)); + if ptr.is_null() { + // Allocator refused — refund so the bank matches reality. + track(-delta); } ptr } @@ -288,25 +294,36 @@ unsafe impl GlobalAlloc for AccountingAllocator { unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { // SAFETY: caller upholds GlobalAlloc invariants; we forward unchanged. unsafe { self.inner.dealloc(ptr, layout) }; - // Free credits the bank. + // Credit only; `track()` short-circuits on `delta >= 0` and never + // panics, so ordering relative to `inner.dealloc` doesn't matter. track(layout.size() as isize); } unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { + // Same panic-then-leak hazard as `alloc`; account first. + let delta = -(layout.size() as isize); + track(delta); // SAFETY: layout is forwarded unchanged. let ptr = unsafe { self.inner.alloc_zeroed(layout) }; - if !ptr.is_null() { - track(-(layout.size() as isize)); + if ptr.is_null() { + track(-delta); } ptr } unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + // Account BEFORE the inner realloc so a kill panic doesn't strand the + // caller with a freed `ptr`. `inner.realloc` frees `ptr` on success; + // if we panicked after that, the caller's `Vec`-or-similar would + // still hold the old pointer and double-free on unwind (glibc + // "double free or corruption (out)" + SIGABRT). + let delta = layout.size() as isize - new_size as isize; + track(delta); // SAFETY: caller upholds GlobalAlloc invariants; we forward unchanged. let new_ptr = unsafe { self.inner.realloc(ptr, layout, new_size) }; - if !new_ptr.is_null() { - // Growth debits, shrink credits. - track(layout.size() as isize - new_size as isize); + if new_ptr.is_null() { + // Allocator refused — refund so the bank matches reality. + track(-delta); } new_ptr }