Issue 5270 - Using a scope delegate allows memory corruption in safe mode
Summary: Using a scope delegate allows memory corruption in safe mode
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: accepts-invalid, pull, safe, spec
: 13085 (view as issue list)
Depends on:
Blocks:
 
Reported: 2010-11-24 09:04 UTC by Lars T. Kyllingstad
Modified: 2018-03-04 12:44 UTC (History)
7 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Lars T. Kyllingstad 2010-11-24 09:04:42 UTC
The following program compiles and runs without error.  Memory corruption happens in bar() because the context for the delegate stored in globalDg never gets copied to the heap, due to the 'scope' storage class being used in call().


@safe:

    void delegate() globalDg;
    void call(scope void delegate() @safe dg)
    {
        dg();

        // Don't tell anyone, but I'm saving this for later ;)
        globalDg = dg;
    }


    void foo()
    {
        int i;
        void izero() { i = 0; }
        call(&izero);
        assert (i == 0);
    }


    void bar()
    {
        int x = 123;

        // Simply calling some function cannot possibly
        // do anything to x...
        globalDg();

        // ...or can it?
        assert (x == 0);
    }


    void main()
    {
        foo();
        bar();
    }
Comment 1 Walter Bright 2012-01-23 23:48:02 UTC
This now compiles & runs successfully.
Comment 2 timon.gehr 2012-01-24 07:38:16 UTC
The issue is that "it compiles and runs without error". (the second assertion asserts that there is memory corruption) The compiler has to either:

- enforce the 'scope' storage class in @safe mode by flow-analysis.
- not perform the scope delegate optimization in @safe mode.

Change the second assertion to 'assert (x == 123);' to see the error.
Comment 3 hsteoh 2014-07-09 22:21:26 UTC
*** Issue 13085 has been marked as a duplicate of this issue. ***
Comment 4 hsteoh 2014-07-09 22:22:41 UTC
Another failing case that illustrates the problem:
------
int delegate() globDg;

void func(scope int delegate() dg) {
        globDg = dg; // should be rejected but isn't
        globDg();
}

void sub() {
        int x;
        func(() { return ++x; });
}

void trashme() {
        import std.stdio;
        writeln(globDg()); // prints garbage
}

void main() {
        sub();
        trashme();
}
------
Comment 5 hsteoh 2014-07-09 22:25:26 UTC
Removing 'scope' from func's parameter fixes the problem, since the compiler will then allocate x on the heap instead of the stack.
Comment 6 Walter Bright 2016-08-25 07:07:57 UTC
(In reply to hsteoh from comment #4)
> Another failing case that illustrates the problem:
> ------
> int delegate() globDg;
> 
> void func(scope int delegate() dg) {
>         globDg = dg; // should be rejected but isn't
>         globDg();
> }

Annotating func() with @safe results in:

  test3.d(4): Error: scope variable dg assigned to non-scope globDg

with Pull Request:

  https://github.com/dlang/dmd/pull/5972

This check is limited to @safe code to avoid breaking too much existing code.
Comment 7 Walter Bright 2016-08-25 07:09:40 UTC
(In reply to timon.gehr from comment #2)
> The issue is that "it compiles and runs without error". (the second
> assertion asserts that there is memory corruption) The compiler has to
> either:
> 
> - enforce the 'scope' storage class in @safe mode by flow-analysis.
> - not perform the scope delegate optimization in @safe mode.
> 
> Change the second assertion to 'assert (x == 123);' to see the error.

Pull https://github.com/dlang/dmd/pull/5972 now causes:

test3.d(9): Error: scope variable dg assigned to non-scope globalDg
Comment 8 Walter Bright 2016-08-25 07:10:13 UTC
This can be closed once 5972 is pulled.
Comment 9 Walter Bright 2018-03-04 01:33:03 UTC
(In reply to Walter Bright from comment #8)
> This can be closed once 5972 is pulled.

It was pulled.
Comment 10 anonymous4 2018-03-04 12:44:15 UTC
(In reply to Walter Bright from comment #6)
> This check is limited to @safe code to avoid breaking too much existing code.
For the record: escaping scoped data should stay allowed is unsafe code, it's a useful pattern, the code would just respect scoping manually.