Bug 11777 – [ICE] dmd memory corruption as `Scope::pop` `free`s `fieldinit` used also in `enclosing`

The issue is caused by refactoring commit 1e99eed73c06ae450c1c13352021e4b629d2bba8 [1] from dmd pull 2771 [2]. Remove `mem.free(fieldinit)` added at line 192 of `scope.c` by the causing commit [1] to detrigger the issue. At first sight looks like `free`d memory is used. But the compiler segfaults at line 203 of `callfunc` in `e2ir.c` because `arg` has invalid vtable address (0x0037382d for me): --- ea = arg->toElem(irs); --- Also `arg` is derived from `Expression` so is at least `sizeof(Expression)` = 22 bytes long, but if we look at its memory: 4 bytes: invalid vtable 10 bytes: allocated but uninitialized (0xCD byte in MS CRT) 4 bytes: allocation end guard (0xFD byte in MS CRT) So it is only 14 bytes long and the only initialized part is vtable pointer. Clear memory corruption. Sorry, the testcase is big and proprietary and can be only obtained directly from the issue author by e-mailing him. [1] https://github.com/D-Programming-Language/dmd/commit/1e99eed73c06ae450c1c13352021e4b629d2bba8 [2] https://github.com/D-Programming-Language/dmd/pull/2771

A workarounding pull: https://github.com/D-Programming-Language/dmd/pull/2990

https://github.com/D-Programming-Language/dmd/commit/89e778a9eee645d2975cbb134e5cfd578bc1ab01 This will be much more likely to stay fixed if you can find a reduced test case... A possible way to find it would be to NULL out the array instead of freeing. Please confirm it's fixed in your program.

DustMite can be used to reduce AND obfuscate test cases. I'd be glad to assist in using DustMite if you're having any problems applying it - just contact me.

(In reply to comment #2) > https://github.com/D-Programming-Language/dmd/commit/89e778a9eee645d2975cbb134e5cfd578bc1ab01 > > This will be much more likely to stay fixed if you can find a reduced test > case... Please, no. Memory corruption cause must be detected. I will investigate today. > A possible way to find it would be to NULL out the array instead of freeing. Nop. MS CRT already marks uninitialized and freed memory appropriately, it also checks previously freed memory isn't changed when allocation returns previously freed pointer. > Please confirm it's fixed in your program. Of course the bug is detriggered as I'm the pull author. ) (In reply to comment #3) > DustMite can be used to reduce AND obfuscate test cases. I'd be glad to assist > in using DustMite if you're having any problems applying it - just contact me. Thanks, but I don't think it will help a lot with random memory corruption. So, the investigation: `callfunc` is unrelated, just a random victim who was unlucky to first access corrupted memory. There are random failures in other functions too. Here is the guilty code, it is in from `Scope::pop`: --- for (size_t i = 0; i < dim; i++) enclosing->fieldinit[i] |= fieldinit[i]; // line 2 mem.free(fieldinit); // line 3 --- The issue trace: 1. There is a `Scope` with `fieldinit` and there is another reference to `fieldinit`. 2. `Scope::pop` `free`-s `fieldinit` at line 3. 3. This memory is reused for different purpose. 4. Now there is another scope with same `fieldinit`. 5. `Scope::pop` executed with such scope as `enclosing` and corrupts reused memory at line 2. 6. Random crashed/wrong code/whatever.

The more precise issue trace: 1. There is a `Scope` with `fieldinit == enclosing->fieldinit` 2. This scope's `pop` `free`-s `fieldinit`. 3. `free`-d memory is reused for different purpose. 5. `pop` on another scope with same `enclosing` corrupts reused memory. 6. Same bad end. Also in my case "another scope" is initially nested in the first scope (3-level child, if we call 1-level a direct child).

So now everyone can add `assert(enclosing->fieldinit != fieldinit);` check in `Scope::pop` and test if the issue is triggered in his codebase, as in the worst case it may silently occur without visible effect but with code corruption.

(In reply to comment #6) > So now everyone can add `assert(enclosing->fieldinit != fieldinit);` check in > `Scope::pop` and test if the issue is triggered in his codebase, as in the > worst case it may silently occur without visible effect but with code > corruption. Denis, can you test this patch? src/scope.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/scope.c b/src/scope.c index f51a30c..c22b585 100644 --- a/src/scope.c +++ b/src/scope.c @@ -188,14 +188,17 @@ Scope *Scope::pop() enclosing->callSuper |= callSuper; if (enclosing->fieldinit && fieldinit) { + assert(enclosing->fieldinit != fieldinit); + size_t dim = fieldinit_dim; for (size_t i = 0; i < dim; i++) enclosing->fieldinit[i] |= fieldinit[i]; - /* Workaround regression @@@BUG11777@@@. - Probably this memory is used in future. - mem.free(fieldinit); - */ - fieldinit = NULL; + + if (!nofree) + { + mem.free(fieldinit); + fieldinit = NULL; + } } }

(In reply to comment #7) > (In reply to comment #6) > > So now everyone can add `assert(enclosing->fieldinit != fieldinit);` check in > > `Scope::pop` and test if the issue is triggered in his codebase, as in the > > worst case it may silently occur without visible effect but with code > > corruption. > > Denis, can you test this patch? I don't understand. The `assert` will definitely fail as I wrote. Did you mean this: --- assert(nofree || enclosing->fieldinit != fieldinit); --- ? Anyway this `assert` fails too.

Since https://github.com/D-Programming-Language/dmd/pull/2990 was pulled, I'm going to mark this as resolved.

Let's add `assert(fieldinit != enclosing->fieldinit);` in `Scope::pop` before we `free(fieldinit)`. This code fails the assertion (and `nofree` is `false`): --- void f(void delegate(int)) { } class C { int i; this() { f((a) {}); } } ---

And again I was incorrect. Failing `enclosing->fieldinit != fieldinit` assertion is just another random victim in case we `free(fieldinit)`. If we don't free, it doesn't happen. So I provided a test-case but has no idea what is going on there. And do not forget workaround dmd pull 2990 is assumed to be reverted while testing.

(In reply to comment #10) > Let's add `assert(fieldinit != enclosing->fieldinit);` in `Scope::pop` before > we `free(fieldinit)`. This code fails the assertion (and `nofree` is `false`): > --- > void f(void delegate(int)) { } > > class C > { > int i; > this() { f((a) {}); } > } > --- Thanks for your work and shrunken test case! It was much helpful for me. https://github.com/D-Programming-Language/dmd/pull/3423

Commits pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/684b84bc3e1635ec8805b9322a440776a50807d0 fix Issue 11777 - [ICE] dmd memory corruption as `Scope::pop` `free`s `fieldinit` used also in `enclosing` https://github.com/D-Programming-Language/dmd/commit/fe52711238588d01b996135aff5f49712c5f2756 Merge pull request #3423 from 9rnsr/fix11777 [REG2.065] Issue 11777 - [ICE] dmd memory corruption as `Scope::pop` `free`s `fieldinit` used also in `enclosing`

Commits pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/dfab94cab4888bbbfbc50f3811c8cc00eeb4a9c9 Copying scope object should fill filedinit to NULL, due to avoid issue 11777 https://github.com/D-Programming-Language/dmd/commit/c876355b1c67d30ccc37f94239950d662082378b Merge pull request #3816 from 9rnsr/fix_sc Copying scope object should fill filedinit to NULL, due to avoid issue 11777

Commit pushed to 2.066 at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/615300eb37465a0ed58ddd86010c8a0d57a2143b Merge pull request #3816 from 9rnsr/fix_sc Copying scope object should fill filedinit to NULL, due to avoid issue 11777

Commit pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/615300eb37465a0ed58ddd86010c8a0d57a2143b Merge pull request #3816 from 9rnsr/fix_sc