std.socket.Socket.receive breaks @safe

[dlangBugzillaToGithub]

hsteoh reported this on 2016-02-18T16:08:15Z

Transfered from https://issues.dlang.org/show_bug.cgi?id=15702

CC List

• issues.dlang (jmdavis)

Description

std.socket.Socket.receive is marked @trusted; however, this is unjustified, because it receives a void[] as buffer and overwrites the buffer with incoming socket data. If the buffer is an array of elements with indirection, this will break @safe-ty:

----
void main() @safe {
    Socket sock = ...;
    Object[] buf = new Object[1];

    // This overwrites the Object ptr with arbitrary data:
    sock.receive(buf, SocketFlags.init);
}
----

This raises the question of whether *any* function that takes in a non-const void[] can reasonably be marked @trusted, since the original type of the array has been erased and the function has no way to guarantee that writing to the void[] won't overwrite pointers with illegal values.

[dlangBugzillaToGithub]

hsteoh commented on 2016-02-18T16:20:18Z

This problem is made much worse by https://issues.dlang.org/show_bug.cgi?id=15672, because that allows the following truly evil code:

------
void readData(void[] buffer) @safe
{
        ubyte[] buf = cast(ubyte[]) buffer; // why does this compile?!
        buf[0] = 0xFF;
}
void main() @safe
{
        auto buffer = new Object[1];
        readData(buffer);
}
------

Thus, the @safe annotations here guarantee nothing at all.

[dlangBugzillaToGithub]

issues.dlang commented on 2016-02-18T18:35:05Z

I would think that converting from T[] to void[] would be @safe. That conversion won't actually corrupt anything. It's doing anything with the void[] which is the problem. Anything and everything which would involve interpreting what void[] is should definitely be @system.

[dlangBugzillaToGithub]

hsteoh commented on 2016-02-18T18:38:36Z

Yes, I agree. Converting T[] to void[] is @safe, but doing basically anything with the void[] other than reading it must be @system. Which means std.socket.Socket.receive should be @system, not @trusted. At least, it cannot be @trusted unless it verifies via sig constraints that hasIndirections!T is false.

Unfortunately changing this will probably break existing code, and it ain't gonna be pretty.

[dlangBugzillaToGithub]

hsteoh commented on 2016-02-18T18:51:20Z

Here's another example that shows why @safe/@trusted on any function that takes (a non-const) void[] must be considered suspect (credit: Steven Schveighoffer, from the forum thread):

----
void foo(void[] arr) @safe
{
    void[] arr2 = [123, 456, 789]; // this is clearly @safe
    arr[] = arr2[0 .. arr.length]; // so is this, under the current definition
}
----

[dlangBugzillaToGithub]

hsteoh commented on 2016-02-19T18:50:01Z

https://github.com/D-Programming-Language/phobos/pull/4011

[dlangBugzillaToGithub]

hsteoh commented on 2016-02-20T01:56:13Z

Changing std.socket.Socket.receive to use templates to check for array indirections will break too much code, and does not play nice with inheritance.

Proposed alternative solution is to make it illegal to implicitly convert T[] to void[] in @safe code if T has indirections. As a compromise, continue to allow explicit cast to void[]. This will plug this particular hole as well as highlight potentially dangerous implicit conversions to void[], but still continue to allow it if the user explicitly casts to void[]. Seems like a reasonable compromise.

[dlangBugzillaToGithub]

hsteoh commented on 2016-02-20T02:13:33Z

Alternative fix in the compiler:

https://github.com/D-Programming-Language/dmd/pull/5468