Fix 23978 - ICE: dip1021 memory corruption #15302

dkorpel · 2023-06-09T13:50:18Z

@ibuclaw It's not actually a regression, -preview=dip1021 has been broken from the start. It just happened to manifest in the report's piece of code after the _d_newclassT PR.

I don't want to add a slow test for this, so I added a comment instead of PERMUTE_ARGS to run it 256 times.

dlang-bot · 2023-06-09T13:50:23Z

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	23978	regression	[REG 2.103.0] ICE: dip1021 memory corruption

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + dmd#15302"

dkorpel · 2023-06-09T13:53:08Z

Honestly we should just deprecate dip1021, it's a broken design with a poor implementation. There's not even a migration path needed: all it did was add errors that don't accomplish anything, so it could just be treated as a no-op.

CyberShadow · 2023-06-09T14:00:26Z

To prevent this class of bugs in the future, would it make sense to wrap all mem.xrealloc into a templated wrapper which checks that the given type doesn't have indirections? Or have some kind of checks in debug builds that it doesn't have pointers to GC-owned objects?

dkorpel · 2023-06-09T14:08:22Z

I think mem.xrealloc should just go away entirely. There are currently only 4 calls to mem.xrealloc left.

dkorpel · 2023-06-09T14:16:28Z

Ubuntu 22.04 x64, GDC fails on fail_compilation/fob2.d:

free(): invalid pointer
Aborted (core dumped)

I suspect one of the free() calls in ob.d is the culprit.

maxhaton · 2023-06-09T14:17:26Z

I think mem.xrealloc should just go away entirely. There are currently only 4 calls to mem.xrealloc left.

Kill it

dkorpel · 2023-06-09T14:25:52Z

I just noticed that xrealloc is aware of the GC:

static void* xrealloc(void* p, size_t size) pure nothrow
    {
        if (isGCEnabled)
            return GC.realloc(p, size);

But the documentation of GC.realloc says: "If p was not allocated by the GC, points inside a block, or is null, no action will be taken."

If that's true, how would it have ever begun to allocate an array when combining -preview=dip1021 and -lowmem? I think the documentation is incorrect and it allocated anyway, but the block is not being scanned.

ibuclaw · 2023-06-09T15:46:40Z

I just noticed that xrealloc is aware of the GC:

Yes, which is why I'm not sure whether this is correct.

ibuclaw · 2023-06-09T15:48:00Z

compiler/src/dmd/escape.d

-        // Clear the new section
-        memset(newPtr + escapeBy.length, 0, (len - escapeBy.length) * EscapeBy.sizeof);
-        escapeBy = newPtr[0 .. len];
+        escapeBy.length = len;


_d_arraysetlength extends the array by using allocate+copy without marking the old data as "free" in the GC.

So this no different to reimplementing xrealloc to allocate and copy inline.

--- a/compiler/src/dmd/root/rmem.d +++ b/compiler/src/dmd/root/rmem.d @@ -71,7 +71,18 @@ extern (C++) struct Mem static void* xrealloc(void* p, size_t size) pure nothrow { if (isGCEnabled) - return GC.realloc(p, size); + { + if (auto p2 = GC.malloc(size)) + { + const psize = GC.sizeOf(p); + if (psize < size) + size = psize; + //p2[0 .. size] = p[0 .. size]; + memcpy(p2, p, size); + return p2; + } + return null; + } if (!size) {

[The druntime function is probably a bit smarter than that and tries to reuse the existing block.]

~~The memset-to-zero is missing now, so the existing elements would be reused in the loop below, that's probably problematic.~~ Edit: Oh sorry, it previously just memset the newly allocated elements, all good.

dkorpel · 2023-06-09T16:04:44Z

Yes, which is why I'm not sure whether this is correct.

It does seem to be related to the EscapeBy[] array though, any idea what's going on?

ibuclaw · 2023-06-09T16:33:37Z

Yes, which is why I'm not sure whether this is correct.

It does seem to be related to the EscapeBy[] array though, any idea what's going on?

Not beyond the pointer dump I hacked together and posted an abridged version of (the 14MB file) in the issue.

Running it through valgrind might help locate who is referencing a given memory address. I have my doubts it'd be capable though.

rainers · 2023-06-09T16:41:34Z

Maybe the failure is more reproducible if the reset-loop at the end of the function actually clears the arrays in EscapeByResults (it currently only resets the length) and also clears EscapeBy.param.

ibuclaw · 2023-06-09T17:14:58Z

I suspect the test is more reproducible if you insert import std; in the middle of struct V. However that's not so suitable for the testsuite. It's still non-deterministic though.

kinke · 2023-06-09T17:41:58Z

It just happened to manifest in the report's piece of code after the _d_newclassT PR.

The switch to _d_newclassT circumvents the _d_newclass override in

dmd/compiler/src/dmd/root/rmem.d

Lines 237 to 244 in f2b3b97

    
           extern (C) Object _d_newclass(const ClassInfo ci) nothrow 
        
           { 
        
               const initializer = ci.initializer; 
        
               auto p = mem.isGCEnabled ? allocClass(ci) : allocmemoryNoFree(initializer.length); 
        
               memcpy(p, initializer.ptr, initializer.length); 
        
               return cast(Object) p; 
        
           }

- when using a host compiler with that new lowering (e.g., LDC hasn't switched yet). So this e.g. means that in default no-GC mode (no -lowmem), all class instances in the compiler are now allocated by the (disabled) GC, not with the bump-pointer scheme anymore.

dkorpel · 2023-06-09T19:55:23Z

That's a good point about rmem, it seems like dmd now only half-overrides druntime's allocating functions.

compiler/test/compilable/test23978.d

ibuclaw · 2023-06-10T08:31:44Z

The switch to _d_newclassT circumvents the _d_newclass override

Yep, good call. I completely overlooked that.

Both GC.realloc and GC.free explicitly mark the passed address as "free" to reuse, however we (sort of) have little guarantee that any memory explicitly allocated via Mem has no other live references within the compiler at the moment either of these functions are called.

I'd be more inclined to remove all uses of both these GC hooks, letting GC.collect do the work by itself.

…jects

ibuclaw · 2023-06-10T17:11:39Z

As mentioned in the bug report, I've finally tracked it down.

The data being Mem.realloc'd contains many Array(T) fields, some of which have self references in their data.ptr field thanks to the smallarray optimization.

Naturally then, the memcpy from old GC data to new retains those self referenced addresses, and the GC marks the old data as "free". Some time later GC.malloc will return a pointer to said "free" data. So now we have two GC references to the same memory. One that is treating the data as an Array(VarDeclaration) in dmd.escape.escapeByStorage, and the other as an AA in the symtab of a dmd.dsymbol.ScopeDsymbol.

Long story short. Don't call Mem.xrealloc on an Array(T), or any data type have contains Array(T) fields!

kinke · 2023-06-10T17:24:35Z

Array has a disabled postblit, but it's un-movable too due to the potential self-reference. So I guess the same memcpy problem occurs whenever appending to/extending a GC-allocated array containing any Array fields?

ibuclaw · 2023-06-10T17:41:26Z

Array has a disabled postblit, but it's un-movable too due to the potential self-reference. So I guess the same memcpy problem occurs whenever appending to/extending a GC-allocated array containing any Array fields?

The short answer is no. Why? Because only GC.realloc(p, size) and GC.free(p) call freeNoSync(p).

The memcpy itself is not harmful, because when the GC scan happens, it will see there are still references to the old data, so will never mark it as free implicitly.

kinke · 2023-06-10T17:50:43Z

Not sure I follow, I'm talking in general about this unmovable Array bad boy, not specifically about Mem.realloc. Any Array with self-reference (length 1) becomes invalid after a move, with a dangling pointer, and then freeing it when getting destructed:

dmd/compiler/src/dmd/root/array.d

Lines 52 to 57 in f2b3b97

    
           ~this() pure nothrow 
        
           { 
        
               debug (stomp) memset(data.ptr, 0xFF, data.length); 
        
               if (data.ptr != &smallarray[0]) 
        
                   mem.xfree(data.ptr); 
        
           }

So GC-array expansions (with implicit moves) are problematic just as moving any aggregate containing some Array field. It's a real bad boy.

ibuclaw · 2023-06-10T18:48:33Z

Not sure I follow, I'm talking in general about this unmovable Array bad boy, not specifically about Mem.realloc. Any Array with self-reference (length 1) becomes invalid after a move, with a dangling pointer, and then freeing it when getting destructed:

dmd/compiler/src/dmd/root/array.d

Lines 52 to 57 in f2b3b97

~this() pure nothrow

{

debug (stomp) memset(data.ptr, 0xFF, data.length);

if (data.ptr != &smallarray[0])

mem.xfree(data.ptr);

}

So GC-array expansions (with implicit moves) are problematic just as moving any aggregate containing some Array field. It's a real bad boy.

GC array expansions won't explicitly mark the old memory as "free" though. It still leaves that up for the next garbage collection run to clear that up. And the GC won't do that because it still sees live references via the new expanded memory.

kinke · 2023-06-10T19:59:47Z

Yeah okay, but then later we get the free(): invalid pointer errors when finalizing the EscapeBy array here.

ibuclaw · 2023-06-10T22:41:04Z

No, we don't. Because the Mem routines (well, GC) just allocate memory from the GC directly without any type information attached. RAII doesn't apply here, so ~this is never ran.

kinke · 2023-06-10T23:04:41Z

[Well, we did here for CI with the intermediate commit where Dennis extended the GC array length. And now don't anymore with the dropped cache, and allocating a fresh new array each time now (for -preview=dip1021 and @live functions only, so probably no big deal).]

ibuclaw · 2023-06-10T23:24:17Z

[Well, we did here for CI with the intermediate commit where Dennis extended the GC array length. And now don't anymore with the dropped cache, and allocating a fresh new array each time now (for -preview=dip1021 and @live functions only, so probably no big deal).]

Ah, I didn't see that intermediate commit, was it perhaps using .length to grow the array? So there was RTTI attached to the memory block?

kinke · 2023-06-11T00:31:34Z

Yes, you commented on it ;) - #15302 (comment)

baryluk · 2023-06-11T17:10:43Z

Thank you Iain, Dennis, et al for the debugging and fix. Amazing.

I submitted original bug report to gdc, Iain minimized it further and did extensive debugging.

Honestly we should just deprecate dip1021, it's a broken design with a poor implementation. There's not even a migration path needed: all it did was add errors that don't accomplish anything, so it could just be treated as a no-op.

I didn't actually intended to use dip1021, but I used -fpreview=all (more out of curiosity), which does enable dip1021 by default too. Since then I switched to enable only relevant DIPs there.

…e::lookup Backports patch from upstream dmd mainline for fixing PR110113. The data being Mem.xrealloc'd contains many Array(T) fields, some of which have self references in their data.ptr field thanks to the smallarray optimization used by Array. Naturally then, the memcpy from old GC data to new retains those self referenced addresses, and the GC marks the old data as "free". Some time later GC.malloc will return a pointer to said "free" data. So now we have two GC references to the same memory. One that is treating the data as an Array(VarDeclaration) in dmd.escape.escapeByStorage, and the other as an AA in the symtab of a dmd.dsymbol.ScopeDsymbol. Fix this memory corruption by not storing the data in a global variable for reuse. If there are no more live references, the GC will free it. PR d/110113 gcc/d/ChangeLog: * dmd/escape.d (checkMutableArguments): Always allocate new buffer for computing escapeBy. gcc/testsuite/ChangeLog: * gdc.test/compilable/test23978.d: New test. Reviewed-on: dlang/dmd#15302 (cherry picked from commit ae3a4ce)

…e::lookup Backports patch from upstream dmd mainline for fixing PR110113. The data being Mem.xrealloc'd contains many Array(T) fields, some of which have self references in their data.ptr field thanks to the smallarray optimization used by Array. Naturally then, the memcpy from old GC data to new retains those self referenced addresses, and the GC marks the old data as "free". Some time later GC.malloc will return a pointer to said "free" data. So now we have two GC references to the same memory. One that is treating the data as an Array(VarDeclaration) in dmd.escape.escapeByStorage, and the other as an AA in the symtab of a dmd.dsymbol.ScopeDsymbol. Fix this memory corruption by not storing the data in a global variable for reuse. If there are no more live references, the GC will free it. PR d/110113 gcc/d/ChangeLog: * dmd/escape.d (checkMutableArguments): Always allocate new buffer for computing escapeBy. gcc/testsuite/ChangeLog: * gdc.test/compilable/test23978.d: New test. Reviewed-on: dlang/dmd#15302

dlang-bot added the Bug Fix label Jun 9, 2023

CyberShadow approved these changes Jun 9, 2023

View reviewed changes

ibuclaw reviewed Jun 9, 2023

View reviewed changes

dkorpel force-pushed the stable branch from 80a3796 to c43c495 Compare June 9, 2023 21:06

dkorpel changed the title ~~Fix 23978 - ICE: EscapeBy[] is malloced, but contains GC-allocated objects~~ Fix 23978 - ICE: dip1021 memory corruption Jun 9, 2023

ibuclaw reviewed Jun 10, 2023

View reviewed changes

compiler/test/compilable/test23978.d Outdated Show resolved Hide resolved

dkorpel force-pushed the stable branch from c43c495 to 32a4a5d Compare June 10, 2023 16:44

Fix 23978 - ICE: EscapeBy[] is malloced, but contains GC-allocated ob…

32a4a5d

…jects

dkorpel merged commit 167b050 into dlang:stable Jun 10, 2023
43 of 44 checks passed

dkorpel deleted the stable branch June 10, 2023 18:15

dkorpel restored the stable branch June 10, 2023 18:16

ibuclaw mentioned this pull request Jun 15, 2023

merge stable #15325

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 23978 - ICE: dip1021 memory corruption #15302

Fix 23978 - ICE: dip1021 memory corruption #15302

dkorpel commented Jun 9, 2023 •

edited

dlang-bot commented Jun 9, 2023 •

edited

dkorpel commented Jun 9, 2023

CyberShadow commented Jun 9, 2023

dkorpel commented Jun 9, 2023

dkorpel commented Jun 9, 2023

maxhaton commented Jun 9, 2023

dkorpel commented Jun 9, 2023

ibuclaw commented Jun 9, 2023

ibuclaw Jun 9, 2023

ibuclaw Jun 9, 2023

kinke Jun 9, 2023 •

edited

dkorpel commented Jun 9, 2023

ibuclaw commented Jun 9, 2023

rainers commented Jun 9, 2023

ibuclaw commented Jun 9, 2023

kinke commented Jun 9, 2023

dkorpel commented Jun 9, 2023

ibuclaw commented Jun 10, 2023

ibuclaw commented Jun 10, 2023

kinke commented Jun 10, 2023 •

edited

ibuclaw commented Jun 10, 2023

kinke commented Jun 10, 2023 •

edited

ibuclaw commented Jun 10, 2023

kinke commented Jun 10, 2023

ibuclaw commented Jun 10, 2023 •

edited

kinke commented Jun 10, 2023

ibuclaw commented Jun 10, 2023

kinke commented Jun 11, 2023

baryluk commented Jun 11, 2023

Fix 23978 - ICE: dip1021 memory corruption #15302

Fix 23978 - ICE: dip1021 memory corruption #15302

Conversation

dkorpel commented Jun 9, 2023 • edited

dlang-bot commented Jun 9, 2023 • edited

Bugzilla references

Testing this PR locally

dkorpel commented Jun 9, 2023

CyberShadow commented Jun 9, 2023

dkorpel commented Jun 9, 2023

dkorpel commented Jun 9, 2023

maxhaton commented Jun 9, 2023

dkorpel commented Jun 9, 2023

ibuclaw commented Jun 9, 2023

ibuclaw Jun 9, 2023

Choose a reason for hiding this comment

ibuclaw Jun 9, 2023

Choose a reason for hiding this comment

kinke Jun 9, 2023 • edited

Choose a reason for hiding this comment

dkorpel commented Jun 9, 2023

ibuclaw commented Jun 9, 2023

rainers commented Jun 9, 2023

ibuclaw commented Jun 9, 2023

kinke commented Jun 9, 2023

dkorpel commented Jun 9, 2023

ibuclaw commented Jun 10, 2023

ibuclaw commented Jun 10, 2023

kinke commented Jun 10, 2023 • edited

ibuclaw commented Jun 10, 2023

kinke commented Jun 10, 2023 • edited

ibuclaw commented Jun 10, 2023

kinke commented Jun 10, 2023

ibuclaw commented Jun 10, 2023 • edited

kinke commented Jun 10, 2023

ibuclaw commented Jun 10, 2023

kinke commented Jun 11, 2023

baryluk commented Jun 11, 2023

dkorpel commented Jun 9, 2023 •

edited

dlang-bot commented Jun 9, 2023 •

edited

kinke Jun 9, 2023 •

edited

kinke commented Jun 10, 2023 •

edited

kinke commented Jun 10, 2023 •

edited

ibuclaw commented Jun 10, 2023 •

edited