Fix issue 14137: std.socket.getAddressInfo breaks @safe

[quickfur]

Remove abuse of @trusted in template function getAddressInfo that cannot guarantee that the incoming type argument T is actually @safe. Localize @trusted to the single call to getAddressInfoImpl, so that any @system code in T will be caught by the type system.

Add unittest to ensure such examples of T will be rejected at compile-time.

Mark normal unittest for getAddressInfo as @safe to ensure that the function body itself does not introduce any non-@safe code.

The rest of this module has frightening amounts of @trusted functions, which need to be properly vetted (and probably the scope of @trusted reduced), but let's leave that for another battle.

[dlang-bot]

Fix	Bugzilla	Description
✓	14137	std.socket.getAddressInfo breaks @safety

[JakobOvrum]

An alternative would be to check isArray!(typeof(option)) && is(typeof(option) : const(char)[]), but I guess that would be a gratuitous breaking change.

---

I've been thinking about what to do with Socket.receive:

auto buffer = new Object[](1);
socket.receive(buffer);

It clearly doesn't have a memory safe interface. We could of course start thinking about type-aware wrappers, but considering the ubiquity of void[] for I/O, I thought it might be a good idea to instead disallow T[] -> void[] in @safe code when hasIndirections!T. Conversion to const(void)[] and immutable(void)[] might still be supported.

---

Anyway, that's for another time. LGTM.

[quickfur]

Well, technically the problem isn't with T[] -> void[] conversion in @safe code, because after that conversion you can't really do much with the void[] as long as you remain in @safe code. IMO the real issue is that functions that take void[] should not be marked @trusted (unless it's const(void[]), i.e., read-only, of course), because you're effectively claiming that the function has a memory-safe interface even though it takes in an array of arbitrary type, including types with indirection, and may do things with it that are completely unsafe.

[quickfur]

Raised a new issue here: https://issues.dlang.org/show_bug.cgi?id=15702

[JakobOvrum]

My point is that if conversion from T[] -> void[] was disallowed in @safe for hasIndirections!T, functions taking void[] would be safely callable from @safe, no matter what they cast to internally. This makes void[] (mutable) usable in @trusted interfaces.

Note that @safe functions don't make any memory safety guarantees when called from @system functions:

void foo(Object o) @safe {}

void main() @system {
    size_t i = void;
    foo(cast(Object)cast(void*)i);
}

[yebblies]

Conversion of T[] to void[] is not @safe.

int*[] x = [new int(0), new int(1), new int(2)];
size_t[] y = [0x12345678, 0x12345678, 0x12345678];
void[] v = x[];
v[] = y[];

And the same thing happens if you replace void[] with ubyte[].

So it looks like we need to make T[] -> *[] unsafe in general for any T with indirections. T[] -> const(*)[] should still be fine.

[quickfur]

What about copying void[] to void[]? Even though the current rules say that this is safe (copying arrays of the same type from one to another), it's actually not safe, because void[] is a type-erased array, and we don't know what its original type was. Allowing copying from void[] to void[] seems to be recipe for disaster, since we can't even check at runtime whether the destination array has the same type as the source.

Having said that, though, if @safe code cannot construct a void[] out of an array with indirections in the first place, perhaps this point will be moot.

[yebblies]

It's true, we could and maybe should tighten up void[] assignment as well, and then we could keep the T[] to void[] conversion.

[jmdavis]

Conversion of T[] to void[] is not @safe.

    int*[] x = [new int(0), new int(1), new int(2)];
    size_t[] y = [0x12345678, 0x12345678, 0x12345678];
    void[] v = x[];
    v[] = y[];

But the problem there is that you're mutating v. Wouldn't

    int*[] x = [new int(0), new int(1), new int(2)];
    void[] v = x[];

be perfectly @safe? I fail to see how simply converting an array to void[] is a problem. It's once you start mucking around with that void[] that things go south - which would include copying an array on top of the void[] like in your example.

[JakobOvrum]

Yeah, writing to void[] must be disallowed in @safe. My proposal is specifically to enable void[] in memory safe interfaces, allowing us to integrate the void[]-for-I/O idiom with @safe.

[jmdavis]

Ah. I responded too quickly and missed this comment:

It's true, we could and maybe should tighten up void[] assignment as well, and then we could keep the T[] to void[] conversion.

Yes. Conversion to void[] should be fine, but anything which interprets what the void[] contains can't be @safe, because whether it's @safe or not depends on what it contains. Reassigning the void[] should be fine, but the vectorized assignment definitely would not be.

[jmdavis]

Yeah, writing to void[] must be disallowed in @safe. My proposal is specifically to enable void[] in memory safe interfaces, allowing us to integrate the void[]-for-I/O idiom with @safe.

The question is whether it's @safe to convert void[] to const(ubyte)[]. I would have thought that it would be, since it's just treating it as bytes, and what the data actually means shouldn't matter - in which case, having a socket function which accepted void[] for writing to the socket (in which case, presumably it could be const(void[])) should be fine, but doing something like receiving data on a socket and writing it to void[] definitely wouldn't be. But there could be something subtle I'm missing that makes it so that converting void[] to const(ubyte)[] would be a problem.

[JakobOvrum]

I'll say it a third time. Receiving data on a socket and writing it to void[] would be @safe if T[] -> void[] was disallowed for hasIndirections!T. Socket.receive would remain @trusted as recv doesn't have a memory safe interface even with the amended rule.

---

As a completely separate issue, T[] -> const(void)[] and immutable(T)[] -> immutable(void)[] should be allowed in @safe unless someone can demonstrate how this could be used to corrupt memory.

[jmdavis]

I'll say it a third time. Receiving data on a socket and writing it to void[] would be @safe if T[] -> void[] was disallowed for hasIndirections!T. Socket.receive would remain @trusted as recv doesn't have a memory safe interface even with the amended rule.

Well, since the conversion to void[] would have been done elsewhere, it's not like Socket.receive can know that the data it's operating on is safe to mutate. So, we either have to always treat writing to void[] as @system, or we have to treat converting any T[] for which hasIndirections!T is true as @system.

But the simple fact that Socket.receive is calling C functions is going to force it to be @trusted no matter what we do with void[] (that, and the fact that it has to use the ptr member on the void[] to pass it to the C function).

[JakobOvrum]

Well, since the conversion to void[] would have been done elsewhere, it's not like Socket.receive can know that the data it's operating on is safe to mutate.

It's always safe to mutate with the amended rule.

So, we either have to always treat writing to void[] as @system, or we have to treat converting any T[] for which hasIndirections!T is true as @system.

I suggest we do both, although with the latter instated, the former becomes just a safeguard (casting before mutating is more visible than no casting required), not a mechanic of supporting @safe.

But the simple fact that Socket.receive is calling C functions is going to force it to be @trusted no matter what we do with void[] (that, and the fact that it has to use the ptr member on the void[] to pass it to the C function).

Whether a function is a C function or D function is entirely orthogonal to @safe.

[jmdavis]

Whether a function is a C function or D function is entirely orthogonal to @safe.

Well, in theory, but to mark a C function with @safe, you'd have to actually know its implementation. They're effectively @system given that the compiler can't verify them, so it's up to the programmer to determine whether what they're doing is @trust-able or not. But they're @system by default regardless, and as it stands, none of the socket functions in druntime are marked as @trusted. So, that combined with the fact that they tend to take require that you use the ptr from an array means that any function using them is going to be @system regardless of what's going on with void[].

[JakobOvrum]

getAddressInfoImpl looks like it has a memory safe interface. Memory safety attributes should be handled there instead.

[quickfur]

So you're saying it should be getAddressInfoImpl that gets marked @trusted?

[JakobOvrum]

Nevermind, the hints parameter has possible unsafe configurations, such as invalid ai_addrlen.

[JakobOvrum]

Well, in theory, but to mark a C function with @safe, you'd have to actually know its implementation. They're effectively @system given that the compiler can't verify them, so it's up to the programmer to determine whether what they're doing is @Trust-Able or not.

C functions can have available body when implemented in D, and D functions can also have unavailable body. Whether a function is a C function or not is a red herring.

But they're @system by default regardless, and as it stands, none of the socket functions in druntime are marked as @trusted.

D functions are also @system by default. Socket functions in druntime's bindings with memory safe interfaces can and should be marked @trusted. You don't need to know the implementation to make this decision - just the interface, which of course includes documentation of behaviour.

So, that combined with the fact that they tend to take require that you use the ptr from an array means that any function using them is going to be @system regardless of what's going on with void[].

The slice ptr property is allowed in @safe and that's not a mistake. The yielded pointer is either null or points to a valid memory address containing an initialized object of infinite lifetime. The fixed-length array ptr property is currently also accepted in @safe, but this is a mistake.

The reason that recv's interface is not memory safe is because it treats the pointer parameter as an array where the length is passed separately.

[jmdavis]

Hmmm. I thought that ptr was @system, but that's probably because it usually gets used either in conjunction with pointer arithmetic or passing it to something that takes the length as a separate argument. But you're right, if all you're doing is looking at the first element in the array, it would actually be @safe.

[dnadlinger]

The slice ptr property is allowed in @safe and that's not a mistake.

Are you sure about that?

int main() @safe {
    auto a = [0, 1, 2];
    auto b = a[$..$];
    return *b.ptr;
}

[JakobOvrum]

@klickverbot, no, I'm not. I guess your example would deref a[3], one element beyond the bounds of a? There are a number of ways we can deal with that, and I'm not sure which is the best one.

[dnadlinger]

@JakobOvrum: Yep, it accesses the beyond-the-last pointer. I knew reported this somewhere already: https://issues.dlang.org/show_bug.cgi?id=11176

[JakobOvrum]

@klickverbot, definitely something we need to fix.

It doesn't have any bearing on the use of recv though, which needs to be manually vetted for memory safety regardless.

[dnadlinger]

@JakobOvrum: Agreed, it is unrelated to recv.

[DmitryOlshansky]

Tons of unrelated discussion. The pull looks good to me

[DmitryOlshansky]

Auto-merge toggled on