Bug 23440 – closure over typesafe variadic or scope array passes safe though leads to stack corruption

Status
NEW
Severity
critical
Priority
P3
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2022-10-27T15:41:13Z
Last change time
2024-12-13T19:25:15Z
Keywords
accepts-invalid, safe
Assigned to
No Owner
Creator
Adam D. Ruppe
See also
https://issues.dlang.org/show_bug.cgi?id=23445
Moved to GitHub: dmd#20171 →

Comments

Comment #0 by destructionator — 2022-10-27T15:41:13Z
Consider the following: ``` void delegate() @safe foo(int[] stuff...) @safe { // adding `scope` to `stuff` makes no difference return () { assert(stuff == [1,2,3]); foreach(item; stuff) {} }; } void delegate() @safe helper() @safe { return foo(1, 2, 3); } void bar() @safe { int[2048] pwned; // just to smash the stack } void main() @safe { auto dg = helper(); bar(); dg(); } ``` Note how everything is `@safe`, though a dmd master build (from mid October), including the -dip1000 switch, fails to catch the problem here. The call to `foo` does a stack allocation, which makes enough sense, it normally does for a typesafe variadic and that's fine. The receiving function doesn't know the pedigree of the array. So it should assume `scope`. This PR merged recently does this: https://github.com/dlang/dmd/pull/14324 which is probably why adding the keyword here doesn't change anything; it is already implicitly scope. However, the delegate is able to capture it anyway, without any error being reported. The `stuff` variable itself is captured on the heap closure, but the `stuff.ptr` is not, which is why the assert is liable to fail. In some cases, you do pass an infinite lifetime array to a typesafe variadic, but the function just doesn't know, so it has to be more conservative. It should prohibit capturing it and extending the lifetime - somehow - without the programmer's intervention to pass the safe checks. bugzilla suggested this: https://issues.dlang.org/show_bug.cgi?id=5212 as a duplicate and they have a similar root cause but not quite the same, as my case launders it through a closure instead of a direct assignment.
Comment #1 by kinke — 2023-03-28T14:35:12Z
Just hit this too. Bumping priority to critical.
Comment #2 by schveiguy — 2023-03-28T14:51:12Z
The closure should fail to allocate, since it has to point at scope data from the heap. It can be reduced to this case: ```d @safe: auto foo(scope int *x) { return { return x; }; } auto bar() { int x = 5; return foo(&x); } void smash() { ubyte[1024] data; } void main() { import std.stdio; auto dg = bar(); smash(); writeln(*dg()); } ```
Comment #3 by Ajieskola — 2023-03-28T15:18:06Z
This isn't due to any bug in variadic arrays. They are (as far as I tested) properly treated as `scope` arrays with the `-preview=dip1000` flag. The real failure is in creation of the closure. This compiles and behaves exactly like the original example: --- void delegate() @safe foo(scope int[] stuff) @safe { return () { assert(stuff == [1,2,3]); foreach(item; stuff) {} }; } void delegate() @safe helper() @safe { int[3] stArray = [1, 2, 3]; return foo(stArray); } void bar() @safe { int[2048] pwned; // just to smash the stack } void main() @safe { auto dg = helper(); bar(); dg(); } ---
Comment #4 by razvan.nitu1305 — 2023-03-29T07:54:19Z
*** Issue 23813 has been marked as a duplicate of this issue. ***
Comment #5 by destructionator — 2023-06-27T21:31:37Z
user on the discord posted this which also seems kinda similar import std; auto makeClosure(ref int n) @safe { return ref () => n; } auto escapeClosure() @safe { int n = 123; return makeClosure(n); } void clobberStack() @safe { ubyte[4096] a; } void main() @safe { auto dg = escapeClosure(); pragma(msg,typeof(dg)); clobberStack(); assert(dg() == 123); // kaboom }
Comment #6 by robert.schadek — 2024-12-13T19:25:15Z
THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/dmd/issues/20171 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB