Wrongly trusted return statement in automem.vector.range() allows obtaining a reference to expired stack frame

[PetarKirov]

This @trusted block of code:

automem/source/automem/vector.d

Lines 313 to 316 in 98d7b1b

	// FIXME - why isn't &this @safe?
	return Range(() @trusted { return &this; }(),
	start,
	end);

allows a pointer to the vector to escape:

/+ dub.sdl:
dependency "automem" version="==0.6.0"
dflags "-preview=dip1000" "-preview=dip25"
+/

import automem.vector;

@safe:

typeof(vector(1).range()) global;

void main()
{
    auto vec1 = vector(1, 2, 3);
    global = vec1.range; // compiles, but it shouldn't
}

Stack corruption PoC:

/+ dub.sdl:
dependency "automem" version="==0.6.0"
dflags "-preview=dip1000" "-preview=dip25"
+/

import std.stdio : writeln;
import automem.vector;

@safe:

typeof(vector(1).range()) global;

void main()
{
    escape;
    global.length.writeln; // 0
    stackSmash;
    global.length.writeln; // 42
}

void escape()
{
    auto vec1 = vector(1, 2, 3);
    global = vec1.range;
}

void stackSmash()
{
    long[4096] arr = 42;
}

$DC --version
DMD64 D Compiler v2.085.0
Copyright (C) 1999-2019 by The D Language Foundation, All Rights Reserved written by Walter Bright

dub automem_test.d
0
42

[atilaneves]

Nice catch, taking a look. Thanks for reporting!

[atilaneves]

So: using @trusted obviously opens a can of worms. But: why isn't return scope enough for dmd to think it's safe to return a range with a pointer to this? It should track the lifetime of the returned range as being shorter than the vector that returned it (because of return scope) then disallow assigning it to a global. Am I missing something that would make this an automem versus a dmd bug if @trusted is removed?

[PetarKirov]

I tried a couple of things with the current layout, but I couldn't figure out how to make it @safe.

So far, I'm thinking that only a layout like this could be easily made @safe:

struct Vector(T)
{
    Impl* impl;
    long start, end;

    struct Impl
    {
        long refs;
        long capacity;
        T[0] data;
    }
    auto range(long start, long end) scope return
    {
        return Vector!T(impl, start, end);
    }
}

(Disclaimer: untested, typing on the phone.)

[PetarKirov]

It should track the lifetime of the returned range as being shorter than the vector that returned it (because of return scope) then disallow assigning it to a global.

Agreed, though at least dmd doesn't allow this code without the use of @trusted. I think this a good candidate for a bug report.

Am I missing something that would make this an automem versus a dmd bug if @trusted is removed?

It's a deficiency of dmd that it doesn't support @safe alternative for your use case.
Perhaps there's a way to make it work, I'll try to think more about that, without changing the object layout.

[atilaneves]

Weirdly enough, this fails to compile as expected with dip1000:

typeof(Container.range()) global;

void main() @safe {
    auto c = Container();
    global = c.range;
}

struct Container {
    auto range() @safe return scope {
        static struct Range {
            Container *self;
        }

        return Range(&this);
    }
}

$ dmd -preview=dip1000 bug.d
bug.d(5): Error: address of variable c assigned to global with longer lifetime

[atilaneves]

Found out why: it's the _elements member variable. Add it to the example above and dmd goes "nope".

[atilaneves]

https://issues.dlang.org/show_bug.cgi?id=19752