sociomantic-tsunami / swarm

Asynchronous client/node framework library
Boost Software License 1.0
14 stars 26 forks source link

Neo requests escape a reference to a stack variable. #417

Closed don-clugston-sociomantic closed 3 years ago

don-clugston-sociomantic commented 4 years ago

When compiled with -dip25, dmd give an alarming warning:

./submodules/swarm/src/swarm/neo/connection/RequestOnConnBase.d(416): Deprecation: copying (cast(const(void*))&elem)[0..4] into allocated memory escapes a reference to parameter variable elem

The code is:

            public void add ( T ) ( ref T elem )
            {
                static assert(!hasIndirections!(T));
                this.outer.outer.send_payload ~= (cast(const(void*))&elem)[0..T.sizeof];
            }

And actually I'm horrified that such a nasty bit of code has no comments. Now look at what this is doing. Suppose T is an int. This creates a void[] slice of the raw bytes of elem, and appends them to send_payload. That's a hacky way of serializing but OK.

But here's the problem. send_payload is not an array of bytes! It is actually:

         private const(void)[][] send_payload;

So though it looks as though it appends the bytes, that's not what it's doing. It's appending the slices. Ie it is saving pointers to stack variables.

Now it looks as though in practice, the send_payload array is used immediately after being set, and then the array is cleared. However, in most cases, the parent class, and hence the array itself, has a lifetime which is longer than the stack variables which it points to. It's possible that all extant uses are correct in the sense that they avoid memory corruption. But it all seems rather fragile.

I am not sure how to deal with this.

don-clugston-sociomantic commented 4 years ago

There is a second escaping reference in appendSlices in src/swarm/neo/util/Util.d

It is basically the same code, though it has code comments which indicates that's it's hacky. However, it appears that this function is never actually used anywhere in swarm or dmq/dls/dhtproto.

Geod24 commented 3 years ago

Ie it is saving pointers to stack variables.

Little correction: The argument is ref, so it's saving a pointer to the caller's frame. It is indeed very fragile. There is addCopy with takes a value and will perform a "deep" copy for integer types.

I think the issue here was that they wanted an array of disjoint types with no overhead.

Geod24 commented 3 years ago

The deprecation is fixed. The usage, not so much, but that's for another issue.