D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 9352 - Memory corruption in delegate called by struct dtor
Summary: Memory corruption in delegate called by struct dtor
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: All All
: P2 major
Assignee: No Owner
URL:
Keywords:
: 9911 (view as issue list)
Depends on:
Blocks:
 
Reported: 2013-01-18 12:07 UTC by hsteoh
Modified: 2022-11-29 14:55 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description hsteoh 2013-01-18 12:07:19 UTC
Code:

------------SNIP-----------
import std.stdio;

static ubyte canary[32] = [
        0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
        20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31
];

struct S {
        ubyte[32] t;
        void delegate()[] destructor;

        this(int dummy) {
                t[] = canary[];
                writefln("ctor: %s", t);

                destructor ~= {
                        writefln("deleg: %s", t);
                };
        }

        ~this() {
                writefln("dtor: %s", t);

                // we're just undoing everything the constructor did, in
                // reverse order, same criteria
                foreach_reverse(d; destructor)
                        d();
        }
}

auto abc(int dummy) {
        return S(0);
}

void main() {
        auto input = abc(0);
        writefln("main: %s", input.t);
}
------------SNIP-----------

Output:
------------SNIP-----------
ctor: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31]
main: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31]
dtor: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31]
deleg: [0, 0, 0, 0, 0, 0, 0, 0, 24, 25, 26, 27, 28, 29, 30, 31, 240, 239, 168, 189, 184, 127, 0, 0, 152, 34, 64, 0, 0, 0, 0, 0]
------------SNIP-----------

This code was minimized from Adam Ruppe's terminal.d. The function abc(int dummy) is necessary; if S is constructed in main, the problem does not occur.
Comment 1 Adam D. Ruppe 2013-01-18 12:15:10 UTC
I think what's happening here is the delegate stores a pointed to the struct made in the ctor, which is on the stack. It gets moved when it returns from the function, but the delegate still points at the old memory, which gets overwritten by whatever.

I figure the best fix would be for the struct copy to update the delegate pointer (if I'm right about what's going on).

OR, we could ban it, since that is a (hidden) internal pointer which i think is banned by the D spec.. probably for exactly this reason.
Comment 2 hsteoh 2013-01-18 12:22:26 UTC
You're right, I added some writeln's to print the address of S.t, and here's what I got:

ctor: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31]
canary address: 7FFF8F541110
main: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31]
canary address: 7FFF8F541190
dtor: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31]
canary address: 7FFF8F541190
deleg: [0, 0, 0, 0, 0, 0, 0, 0, 160, 111, 66, 0, 0, 0, 0, 0, 240, 79, 99, 213, 136, 127, 0, 0, 176, 34, 64, 0, 0, 0, 0, 0]
canary address: 7FFF8F541110

Notice that both in main and in the dtor, a different address from the original address in the ctor is used. However, the delegate is using the original address instead of the new address. So the problem is indeed that the delegate is pointing to the invalidated copy of the struct.
Comment 3 hsteoh 2013-01-18 12:25:33 UTC
Rather than banning this outright, I think a better approach may be to detect when a delegate is referencing the struct on the stack, and create the struct on the heap instead. Sorta like how local variables will be allocated on the heap instead of the stack if the function returns a delegate that references them.

(But I'm not sure how feasible it is to do this in a struct ctor, though!)
Comment 4 hsteoh 2020-07-13 17:31:18 UTC
With -dip1000 and @safe, this code no longer compiles, and triggers the appropriate error messages about a reference to `this` being stored in a non-scope field.  IMO this is a good-enough resolution.
Comment 5 RazvanN 2022-11-29 14:55:45 UTC
*** Issue 9911 has been marked as a duplicate of this issue. ***