Bug 13116 – Should not be able to return ref to 'this'

Status
RESOLVED
Resolution
FIXED
Severity
critical
Priority
P1
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2014-07-13T04:48:57Z
Last change time
2023-07-05T07:10:03Z
Keywords
accepts-invalid, pull, safe, wrong-code
Assigned to
No Owner
Creator
hsteoh
See also
https://issues.dlang.org/show_bug.cgi?id=24024

Comments

Comment #0 by hsteoh — 2014-07-13T04:48:57Z
This code compiles, but should not: ----- import std.stdio; class C { int x; this(int _x) { x = _x; } ref C evil() { return this; // <-- should not compile but does } } void hmm(int x, int y, ref C c) { c = null; // corrupt memory writefln("%d %d", x, y); // prints "0 2" } void main() { auto c = new C(1); auto d = new C(2); hmm(1, 2, c.evil()); // N.B., we passed 1 and 2 to hmm() } ----- Explanation: C.evil() returns a dangling pointer to an out-of-scope local variable (i.e., 'this'), which is passed into hmm() which overwrites that memory location. On my system (Debian/Linux amd64) it just so happens that this memory location coincides with the address of the parameter 'x', thus causing x to get overwritten. Cause of bug: it should be illegal to return 'this' in a ref function, because it's a local variable (albeit implicit).
Comment #1 by hsteoh — 2014-07-13T04:54:37Z
Better yet, the following variation shows @safe breakage: ---- import std.stdio; class C { int x; this(int _x) { x = _x; } ref C evil() @safe { return this; // <-- should not compile but does } } void hmm(int x, int y, ref C c) { () @safe { c = null; // corrupt memory -- in @safe block }(); writefln("%d %d", x, y); // prints "0 2" } void main() { auto c = new C(1); auto d = new C(2); hmm(1, 2, c.evil()); // N.B., we passed 1 and 2 to hmm() } ----
Comment #2 by hsteoh — 2014-07-13T04:58:20Z
Attempting to do the same with returning a local variable produces a compile error: test.d(10): Error: escaping reference to local variable c which is correct. So looks like 'this' was overlooked as a local variable (albeit an implicit one) in the check for escaping references.
Comment #3 by hsteoh — 2014-07-14T14:41:47Z
Note that it's OK to do this with structs because the address of the original struct is returned, not the address of the `this` implicit variable.
Comment #4 by hsteoh — 2014-07-14T14:42:01Z
Comment #5 by hsteoh — 2014-07-14T17:20:09Z
As Kenji points out, this is part of a more general problem where `super` and `this` in class methods are lvalues, so they are liable to illegal rebindings: ---- class Base { int x; } void rebind(ref Base b) { b = new Base; } void rebind(ref Derived d) { d = new Derived; } class Derived : Base { int y; void evil() { rebind(super); x = 123; // this modifies a different copy of Base.x than this one! rebind(this); y = 123; // this modifies a different copy of this.y ! } } ---- This problem can be solved if we make `this` and `super` rvalues in class methods. Note that in struct methods, `this` is OK to be an lvalue, because structs are by-value types so no rebinding is involved in the struct analog of the above code; it modifies the struct in-place.
Comment #6 by github-bugzilla — 2014-08-05T14:59:13Z
Commit pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/24e8347f1fa4ccbb1cc6a27c40eedef1324e6c13 Merge pull request #3761 from quickfur/issue13116 Issue 13116: should reject returning `this` from a ref class member function
Comment #7 by github-bugzilla — 2015-03-09T11:22:28Z
Commit pushed to revert-3761-issue13116 at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/084c8f4168c37543ddabd9ba6b666152a600f129 Revert "Issue 13116: should reject returning `this` from a ref class member function"
Comment #8 by github-bugzilla — 2015-03-09T23:48:40Z
Commit pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/942dffe2da43c22ea64b6dc6ae4423f921c7838c Merge pull request #4463 from MartinNowak/revert3761 Revert "Merge pull request #3761 from quickfur/issue13116"
Comment #9 by k.hara.pg — 2015-03-10T00:21:13Z
(In reply to github-bugzilla from comment #8) > Commit pushed to master at https://github.com/D-Programming-Language/dmd > > https://github.com/D-Programming-Language/dmd/commit/ > 942dffe2da43c22ea64b6dc6ae4423f921c7838c > Merge pull request #4463 from MartinNowak/revert3761 > > Revert "Merge pull request #3761 from quickfur/issue13116" By that, the wrong-code bug is re-introduced.
Comment #10 by github-bugzilla — 2015-03-10T11:53:54Z
Commit pushed to 2.067 at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/4d7040a5296a3a5e155b1dd37c0fbad480a2061b Merge pull request #4463 from MartinNowak/revert3761 Revert "Merge pull request #3761 from quickfur/issue13116"
Comment #11 by github-bugzilla — 2015-04-11T12:24:53Z
Comment #12 by github-bugzilla — 2015-06-17T21:01:44Z
Comment #13 by k.hara.pg — 2016-03-28T13:35:34Z
Comment #14 by github-bugzilla — 2016-03-31T10:03:14Z
Commits pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/b0f45b08924511891473c7a2cb18dc8446a9ad30 fix Issue 13116 - Should not be able to return ref to 'this' https://github.com/D-Programming-Language/dmd/commit/2d59f2e43e59b1a3c66745c6d9ac9039ff09eed3 Merge pull request #5591 from 9rnsr/fix13116 Issue 13116 - Should not be able to return ref to 'this'
Comment #15 by github-bugzilla — 2016-10-01T11:45:36Z
Commits pushed to stable at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/b0f45b08924511891473c7a2cb18dc8446a9ad30 fix Issue 13116 - Should not be able to return ref to 'this' https://github.com/dlang/dmd/commit/2d59f2e43e59b1a3c66745c6d9ac9039ff09eed3 Merge pull request #5591 from 9rnsr/fix13116
Comment #16 by github-bugzilla — 2018-01-03T22:31:58Z
Commit pushed to dmd-cxx at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/ad0a23cccb67f7876f8831ef653882e97eaec8e7 fix Issue 13116 - Should not be able to return ref to 'this'