Bug 24856 – std.array.Appender.ensureAddable can create stale memory references

Status
RESOLVED
Resolution
FIXED
Severity
normal
Priority
P1
Component
phobos
Product
D
Version
D2
Platform
All
OS
All
Creation time
2024-11-13T02:08:44Z
Last change time
2024-11-14T23:59:10Z
Keywords
pull
Assigned to
No Owner
Creator
Steven Schveighoffer
See also
https://issues.dlang.org/show_bug.cgi?id=24860

Comments

Comment #0 by schveiguy — 2024-11-13T02:08:44Z
The scenario: 1. A large block of memory is allocated, and a small block of memory is allocated with 8 pointers to the large block. 2. The small block is freed 3. You use std.array.Appender!(void*[]), and append one pointer of `null` What happens under the hood in step 3: 1. It calls ensureAddable(1). 2. ensureAddable realizes the current capacity (0) is too small, so it calculates a new capacity (minimum 8). 3. It calls GC.qalloc(8 * elemsize), which tells the GC "I want a block that holds 8*elemsize bytes at least". 4. The newly freed block is used, because it's the right size. 5. Since T contains pointers (it's a void*), the GC will zero everything *after* the 8 elements asked for. 6. The Appender adds the one pointer, but does *not* initialize the other 7 asked for. If the other elements are not appended to, then the stale data which points at the large block is still there. This keeps the large block pinned, since the GC will scan the entire block. In fact, this is exactly what happened to me, and it took an inordinate amount of resources to track this down. I was able to reproduce the issue on run.dlang.io with this code, but no guarantee it always works: ``` void main() { import std.array; import core.memory; import std.stdio; void*[] blk = new void*[7]; blk[] = cast(void*)0xdeadbeef; GC.free(blk.ptr); foreach(i; 0 .. 10000) { Appender!(void*[]) app; app.put(null); if(app.data.ptr !is blk.ptr) continue; writeln(app.data.ptr[0 .. 7]); break; } } ``` The output from this was: [null, 560745EC8560, DEADBEEF, DEADBEEF, DEADBEEF, DEADBEEF, DEADBEEF] DEADBEEF represents the large allocation that is kept in memory. I used a 7-element array, because 8 elements would have grown into the next size block. But you can clearly see, the stale data is present. The fix is to zero any new data asked for, but not initialized, inside Appender. Both in the `extend` call and the `qalloc` call, when the element type contains pointers. Reference to the code in question: https://github.com/dlang/phobos/blob/db798688e4dd492d6f739cac98069518e8290e97/std/array.d#L3613
Comment #1 by apz28 — 2024-11-13T04:55:39Z
In light of this bug, can build in array also affected by this bug since there are various bugs reported of runtime memory leak?
Comment #2 by nick — 2024-11-13T13:04:58Z
I've made a pull to add warnings to core.memory about uninitialized bytes containing GC pointers: https://github.com/dlang/dmd/pull/17061
Comment #3 by schveiguy — 2024-11-13T20:45:11Z
(In reply to apham from comment #1) > In light of this bug, can build in array also affected by this bug since > there are various bugs reported of runtime memory leak? You are right. The same mistake is made there. Very interesting! https://github.com/dlang/dmd/blob/5cce98ed7fbd80cad0f7dcce08c09da3c1bffad8/druntime/src/rt/lifetime.d#L1906
Comment #4 by schveiguy — 2024-11-13T20:48:08Z
(In reply to apham from comment #1) > since there are various bugs reported of runtime memory leak? Do you have references to these bugs?
Comment #5 by dlang-bot — 2024-11-14T03:56:27Z
@schveiguy created dlang/phobos pull request #9084 "Fix appender code that might not initialize memory that might point at huge allocations" fixing this issue: - Fix bugzilla issue 24856 - Make sure appender doesn't leave some stale pointers in unallocated space, thereby pinning unused memory. https://github.com/dlang/phobos/pull/9084
Comment #6 by schveiguy — 2024-11-14T18:20:30Z
(In reply to Steven Schveighoffer from comment #3) > You are right. The same mistake is made there. Very interesting! > I made a bug report for this, but I'm realizing it's not actually a problem in current released compilers, nor will it be a problem in 2.110, due to a quirk in the way small allocations are grown, and the way large allocations are scanned. So this would not explain any existing bugs with memory leaks. The 2.111 version however, was changed to adjust the growth factor for small blocks, so this problem would appear in that unless we do something. In any case, I created issue 24860 to track that, it is separate from this.
Comment #7 by dlang-bot — 2024-11-14T23:59:10Z
dlang/phobos pull request #9084 "Fix appender code that might not initialize memory that might point at huge allocations" was merged into stable: - 120e8195c93d6778fd99a143963bbdb1bb5872ec by Steven Schveighoffer: Fix bugzilla issue 24856 - Make sure appender doesn't leave some stale pointers in unallocated space, thereby pinning unused memory. https://github.com/dlang/phobos/pull/9084