[WhySoBad]

This pull request adds support for the sendto and recvfrom socket operations to the UnixSocketFileDescription trait.

Additionally, it also implements them for the TCP socket shim. Since TCP sockets are "connection-mode", they are mostly the same as recv and send.

[RalfJung]

Nice, thanks!
@rustbot author

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[WhySoBad]

@rustbot ready

[RalfJung]

My new tests pass in Miri but fail natively. Seems like the behavior is still not quite right?

[RalfJung]

@rustbot author

[WhySoBad]

I integrated your commit into mine such that the write_socket_address changes can be part of the recvfrom commit.

The write_socket_address now takes an optional socket address, and if it's None then we just set the address length to zero.

@rustbot ready

[RalfJung]

Now behavior is wrong if the address pointer is 0. ;) In that case it apparently does not update the length.
Looks like we need to cover a whole bunch of different combinations here.

Also strangely on my system I cannot reproduce the "length gets reset to 0" any more? I get this instead

thread '<unnamed>' (659654) panicked at tests/pass-dep/libc/socket-tcp.rs:448:14:
called `Result::unwrap()` on an `Err` value: Os { code: 14, kind: Uncategorized, message: "Bad address" }

[RalfJung]

Ah, it's not happy if the value addr-len-ptr points to is too big. So it definitely always reads that pointer? I assume that means it must not be null or dangling.

EDIT: No it's not the size. It's fine with 1338 or 1500. But not with 1337. ?!??

[RalfJung]

Oh wait the "bad address" happens later in a different test. 🤦

So anyway for recvfrom I think we should have a test where the addr pointers is null and the len pointer is dangling but non-null, and a test where the addr pointer is dangling-non-null and the len pointer valid, and it needs to be overwritten by 0.
The logic is -- if the addr pointer is null, we are done. Otherwise, store 0 in the len ptr.

For sendto, I think the logic is -- if the len or addr ptr are null, we are done, but otherwise we actually load the address? Or maybe we just always return "bad address"? Not sure here.

[WhySoBad]

For sendto, I think the logic is -- if the len or addr ptr are null, we are done, but otherwise we actually load the address? Or maybe we just always return "bad address"? Not sure here.

Linux seems to actually load the address as soon as neither argument is zero/null. However, this directly contradicts their man page (and also the POSIX man page) of sendto which state that "[...] If sendto() is used on a connection-mode (SOCK_STREAM, SOCK_SEQPACKET) socket, the arguments dest_addr and addrlen are ignored [...]". I'd thus suggest that we don't do what Linux does and instead let something like this pass:

libc_utils::write_all_generic(
    TEST_BYTES.as_ptr().cast(),
    TEST_BYTES.len(),
    libc_utils::NoRetry,
    |buf, count| {
        libc::sendto(
            peerfd,
            buf,
            count,
            0,
            ptr::without_provenance_mut::<libc::sockaddr>(16),
            16,
        )
    },
)

[WhySoBad]

Turns out there is more BSD-specific behavior:

• For recvfrom when the address pointer is a null pointer and the address length is an invalid pointer, the BSDs return EFAULT, however, they never specify that the address length pointer needs to be valid in that case.
• For sendto when the address length is zero and the address pointer is an invalid pointer, the BSDs return EINVAL but neither documents EINVAL for sendto.

We now skip those tests on native BSDs.

[RalfJung]

For recvfrom when the address pointer is a null pointer and the address length is an invalid pointer, the BSDs return EFAULT, however, they never specify that the address length pointer needs to be valid in that case.

Maybe that should actually be raise UB then. Given the POSIX docs and behavior of BSDs, I am inclined to say that if the address pointer is non-null, we always overwrite the length pointer with 0. I think POSIX allows this, and Linux being extra liberal is not something we have to model given that other Unixes apparently are closer to POSIX here.

[RalfJung]

@rustbot author

[WhySoBad]

I am inclined to say that if the address pointer is non-null, we always overwrite the length pointer with 0

This is exactly what we currently do. The problem with those BSD hosts is that they read the address length pointer even when the address pointer is null, and in our test case this address length pointer is a pointer without provenance.

@rustbot ready

[RalfJung]

Oh I see. The length ptr is documented by POSIX as

Either a null pointer, if address is a null pointer, or a pointer to a socklen_t object which on input specifies the length of the supplied sockaddr structure, and on output specifies the length of the sending address.

So POSIX seems to permit what the BSDs are doing.

[WhySoBad]

Yes, the question is whether we want to implement this edge case for BSD targets, or whether we accept an API difference there.

[RalfJung]

I added a commit that implements what seems like a reasonable check, that covers the recvfrom behavior we saw on BSD: if a length pointer is present, then the value it stores initially actually must be the size of the buffer that the address pointer points to, even if we are not going to write anything.

[RalfJung]

FWIW I cannot reproduce this, I think some results got mixed up. See #5576.