Incorrect handling of `address` argument in recvfrom() about network HOT 10 OPEN

Let's introduce a breaking change:

peekSocketAddress :: Ptr sa -> Int -> IO sa

And let's release it with a new major version.

from network.

@Philonous As you think, SockAddrUnix "" would be the best workaround.
Would you kindly send a PR?

from network.

@Philonous Gentle ping.

from network.

I'm working on it. This isn't as obvious as I first thought.

• Since 3.0 Socket doesn't container the address family any more, so I can't check it to return the correct address type when addrlen is 0.
• I'm thinking that just returning SockAddrUnix "" whenever addrlen is 0 regardless of the socket type might work, it's at least not worse than the situation before.

However, there's another problem:

• POSIX doesn't specify that sun_path is null-terminated (though at least on Linux and *BSD apparently it is)
• On Linux, the terminating null byte can sometimes be truncated [1] even for correctly sized buffers (sizeof(sockaddr_un))
  • This doesn't affect the existing code because it allocates and zeroes sizeof(sockaddr_storage) = 128 bytes[2][3], so there's always a null byte after sun_path. But SocketAddress' is publicly exported and can be implemented for a new type, e.g. one that passes a buffer of sizesizeof(sockaddr_un) bytes, which should work.
• Abstract paths can contain null characters, so the only way to handle them properly is to know the length of sun_path
  • This is handled correctly in other places[4], so it's surprising that peekSocketAddress completely fails

As far as I can see, the interface peekSocketAddress :: Ptr sa -> IO sa can't work, because it needs addrlen to correctly decode sockaddr_un. It might also be a good idea to handle the case where addlen = 0 there.

• [1] https://man7.org/linux/man-pages/man7/unix.7.html see section BUGS:

However, there is one
case where confusing behavior can result: if 108 non-null bytes
are supplied when a socket is bound, then the addition of the
null terminator takes the length of the pathname beyond
sizeof(sun_path). Consequently, when retrieving the socket
address (for example, via accept(2)), if the input addrlen
argument for the retrieving call is specified as sizeof(struct
sockaddr_un), then the returned address structure won't have a
null terminator in sun_path.

• [2]

  network/Network/Socket/Types.hsc

  Lines 1021 to 1023 in 09f30fa

  	withNewSocketAddress f = allocaBytes sockaddrStorageLen $ \ptr -> do
  	zeroMemory ptr $ fromIntegral sockaddrStorageLen
  	f ptr sockaddrStorageLen

• [3]

  network/Network/Socket/Types.hsc

  Line 1010 in 09f30fa

  sockaddrStorageLen = 128

• [4]

  network/Network/Socket/Types.hsc

  Lines 1114 to 1117 in 09f30fa

  	sizeOfSockAddr (SockAddrUnix path) =
  	case path of
  	'\0':_ -> (#const sizeof(sa_family_t)) + length path
  	_ -> #const sizeof(struct sockaddr_un)

from network.

Since 3.0 Socket doesn't container the address family any more, so I can't check it to return the correct address type when addrlen is 0.

Why don't you use getSocketName to get its SocketAddress type?

from network.

As far as I can see, the interface peekSocketAddress :: Ptr sa -> IO sa can't work, because it needs addrlen to correctly decode sockaddr_un. It might also be a good idea to handle the case where addlen = 0 there.

Can't we use sizeOfSocketAddress or sizeOfSockAddr to get addrlen?

from network.

Why don't you use getSocketName to get its SocketAddress type?

• AFAIK Windows doesn't allow getsocketname() on unbound sockets. [1] Windows doesn't have AF_UNIX either, so the code could be conditionally included.
• POSIX explicitly mentions that getsocketname() on unbound sockets returns unspecified data [2]. Although on Linux it seems to at least return the correct address family.

Can't we use sizeOfSocketAddress or sizeOfSockAddr to get addrlen?

We already have addrlen, recvfrom() etc. are passing it back to us. The problem is that we need it inside peekSocketAddress to correctly decode sockaddr_un.
sizeOfSocketAddress needs an already decoded address, so we can't use it inside peekSocketAddress.

• [1] https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-getsockname

WSAEINVAL | The socket has not been bound to an address with bind, or ADDR_ANY is specified in bind but connection has not yet occurred.

• [2] https://pubs.opengroup.org/onlinepubs/9699919799/

If the socket has not been bound to a local name, the value stored in the object pointed to by address is unspecified.

from network.

I've been working on this, but I've been stuck on

I think the idea here is that for connected sockets, recvfrom() and recvmsg() don't return the remote address, so the getpeername() is there for convenience.

This assumes that recvfrom() will only not return an address when the socket is connected, which is false for AF_UNIX (this is what this ticket is about in the first place). In general, this would be false whenever we receive a packet from an unnamed socket. So I've been trying to figure out if this can happen besides with AF_UNIX. It could in theory happen with socketpair(), but none of the platforms I checked (Linux, FreeBSD, OpenBSD, NetBSD, Solaris) support socketpair() with anything but AF_UNIX, windows doesn't have socketpair().

Next I tried to figure out if getsockname() is guaranteed to return sensible data even for unbound sockets at least on *nix, since POSIX specifically states that it is "unspecified". On Windows it seems to be forbidden anyway, so it would have to be #ifdef-ed out. Linux just returns an empty address, which is fine, FreeBSD returns the correct address family and a bunch of garbage, but it looked like it should at least be safe to read, but I'm not entirely certain. I did not check on the other platforms for lack of time and easy access

I think this is brittle. A slightly better idea (in my opinion) might be to store the address family in the Socket value like before 3.0.

But I think the morally correct approach would be to change recvFrom's etc type signature from

recvFrom :: SocketAddress sa => Socket -> Int -> IO (ByteString, sa)

to

recvFrom :: SocketAddress sa => Socket -> Int -> IO (ByteString, Maybe sa)

and remove the call to getpeername().

It would remove the possibility of failure and more closely resemble the the underlying API.
Unfortunately, it would be a breaking change and presumably affect a lot of existing code.

from network.

I would suggest to keep recvFrom as is and provide a new safeRecvFrom which satisfies you.

from network.