IMHO, this is a lot of complex gymnastics at the Haskell layer for a problem that should likely be more easily and cleanly solved in the underlying C API.

The underlying C-functions called by Haskell's Network library that fill-in sockaddr data would need to arrange to handle unterminated sun_path on systems that lack a sun_len field in sockaddr_un, by arranging to ensure any missing null-termination.

I would strongly also recommend to always zero the buffer (in C) before calling system calls that fill-in a user-provided sockaddr buffer. Whether on Darwin or not.

If the output socket addresses used for AF_UNIX are always zero-filled before use, and are one-byte longer than the maximum size used by the platform, then null-termination is always assured. The C-wrapper can also use the addrlen output argument to append any missing NUL byte via ((unsigned char *)sa)[addrlen] = 0, if the addrlen initially passed to the system call is one less than the actual (too large by 1) allocation.

This is not only simpler but uniformly safer. I'd also like too see a lot less #ifdefs in the code.

Finally, I'm puzzled by the logic below. This type of thing deserves comments explaining what's going on. Why does an empty path get that particular length? Is it the right length on platforms that also have sun_len? ...

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

Why not just (#const sizeof(struct sockaddr_un)) + 1

If you have preferences about how this should be implemented, please let me know what you want to see where. My goal in this request is to fix a bug upstream that has halted work on a much larger project (I have already patched locally to unblock my team's progress). I am not interested in broader refactors on a repo that is already undergoing a lot of change.

• Almost all of the C-functions you reference are direct imports of syscalls. There are no Haskell Network Library level C-functions for accept, bind, close, etc with which to tap into.
• Zero buffering things needs a broad look across the entire code repo since none of the alloca calls have their contents zeroed out before use. I can zero out this particular call here, but it's a much bigger issue in the code.
• I too would like to see fewer #ifdefs as well as some cleaner logic around different parts. Both of those are well beyond the scope of what this pull request aims to achieve.

Indeed you're not obligated to take on substantial refactoring of the code. And my opinion is not dispositive, I am not a maintainer of the project, just an observer with some experience. So I'll leave to you and Kazu et. al. to figure how to move forward. My comments are intended to aid of the long-term quality of the code base. If you have cycles to refactor code that is starting to show wear and tear, super! If not, perhaps someone else can. The risk comes in when code is repeatedly patched in ways that minimize change, but increase long-term costs by adding complexity. When it is time to stop and clean up is a matter of judgement.

As to the specifics:

• Almost all of the C-functions you reference are direct imports of syscalls. There are no Haskell Network Library level C-functions for accept, bind, close, etc with which to tap into.

Indeed, and so one might need to add some such functions that present a saner, more uniform API to the upper layers of the system. Heimdal (where I am a maintainer) has "libroken" which deals with platform-specific quirks, so that the rest of the software sees a more uniform platform, and needs a lot fewer #ifdefs. Postfix likewise has various sane_somefunction() for various values of somefunction that have platform-specific idiosyncrasies. It may be appropriate to add some such wrappers here.

• Zero buffering things needs a broad look across the entire code repo since none of the alloca calls have their contents zeroed out before use. I can zero out this particular call here, but it's a much bigger issue in the code.

Perhaps so, but if sun_path has a known issue in this space, it may be a good place to start.

• I too would like to see fewer #ifdefs as well as some cleaner logic around different parts. Both of those are well beyond the scope of what this pull request aims to achieve.

This cuts to the core of the question of when it is time to stop patching and do some refactoring. I'll leave that call to Kazu et. al.

I created https://github.com/kazu-yamamoto/network/tree/unix-domain by copying your test case.
See CI results: https://travis-ci.org/kazu-yamamoto/network/builds

For the first commit, the test case fails on Linux. This is because you check a socket address returned from accept should be SockAddrUnix "". In my opinion, this is not correct. We cannot assume anything for a socket address returned from accept in the case of Unix domain.

So, I removed the shouldBe and the test passes. Now I don't understand what you are trying to solve.

In some cases, sun_path is not null-terminated. But this is a kind of application bugs. Linux kernel certainly null-terminated sun_path. So, null-termination related bugs should not occur on Linux.

This is not only simpler but uniformly safer. I'd also like too see a lot less #ifdefs in the code.

Finally, I'm puzzled by the logic below. This type of thing deserves comments explaining what's going on. Why does an empty path get that particular length? Is it the right length on platforms that also have sun_len?

This is due to the original code. I agree that refactoring is necessary.

Why not just (#const sizeof(struct sockaddr_un)) + 1

For abstract unix domain sockets on Linux, this does not work. I don't understand why yet.

I misunderstood. This is not related to abstract unix domain sockets. Simply + 1 does not work.

null-termination related bugs should not occur on Linux

Unfortunately they do. From the unix man page (http://man7.org/linux/man-pages/man7/unix.7.html) under Bugs:

   some implementations don't require a null terminator
   when binding a socket (the addrlen argument is used to determine the
   length of sun_path) and when the socket address is retrieved on these
   implementations, there is no null terminator in sun_path.

   Applications that retrieve socket addresses can (portably) code to
   handle the possibility that there is no null terminator in sun_path
   by respecting the fact that the number of valid bytes in the pathname
   is:

       strnlen(addr.sun_path, addrlen - offsetof(sockaddr_un, sun_path))

I was only able to consistently repro this on my centos setup. With the shouldBe check, I would get this failure:

spec: HUnitFailure (Just (SrcLoc {srcLocPackage = "main", srcLocModule = "SimpleSpec", srcLocFile = "tests/SimpleSpec.hs", srcLocStartLine = 158, srcLocStartCol = 23, srcLocEndLine = 158, srcLocEndCol = 56})) (ExpectedButGot Nothing "" "\EOT")

Note how the returned SockAddr was \EOT, which certainly isn't correct.

I'll attempt to be more concrete on exactly what I've been seeing.

An application I'm working on would intermittently get decoding exceptions coming out of a call to accept when a new connection came around. Wolf-fencing showed this exception to be thrown on the peekSockAddr addr line in accept ::. Connecting gdb to the repro, I looked at the contents of sun_path and saw gobbledygook. It appeared like sun_path was left completely untouched as uninitialized memory across the accept syscall. This would explain the intermittent decoding errors.

• A block of memory would be allocated with no structure
• The syscall would leave sun_path untouched
• The network code would immediately call peekCString on sun_path

This would then attempt to locale-decode (utf8 in our case) uninitialized memory. If we happened to get something null terminated early enough it would work fine. Other times we'd get invalid byte sequences.

It would be great if all linux distros filled sun_path to be null terminated, but I have empirical evidence of that not happening on some distros. Looking at the man page, this situation of non-null-terminated sun_path is explicitly called out and has a suggested remediation. That remediation is what I've implemented in this patch.

Connecting gdb to the repro, I looked at the contents of sun_path and saw gobbledygook. It appeared like sun_path was left completely untouched as uninitialized memory across the accept syscall. This would explain the intermittent decoding errors.

A block of memory would be allocated with no structure
The syscall would leave sun_path untouched

This is why the sun_path field should be zero-filled by the caller, before calling accept(), or any other socket function that fills-in a caller-provider sockaddr! If the syscall leaves sun_path unmodified, then it will be an empty address! Presumably "untouched" really means 0-length, and the OS writes 0 bytes (and does not write the terminating NUL). On platforms were sockets have a sun_len field. And of course the OS also returns a socklen_t overall length from which the length of sun_path can be deduced.

My view is all the details of this should be handled in a C-wrapper around the various syscalls, so that the Haskell sees uniform behaviour across all platforms. By the time the sockaddr is returned to Haskell it should have a sensible length, and the sun_path should already be NUL terminated.

The network code would immediately call peekCString on sun_path
This would then attempt to locale-decode (utf8 in our case) uninitialized memory. If we happened to get something null terminated early enough it would work fine. Other times we'd get invalid byte sequences.

Also sun_path is surely a ByteString, not a utf8 string. It should not be subject to locale-specific decoding.

It would be great if all linux distros filled sun_path to be null terminated, but I have empirical evidence of that not happening on some distros.

Sample C-code and output to demonstrate the behaviour you're seeing would really be helpful. Does zeroing the sockaddr before asking the OS to return the address solve the problem?

Looking at the man page, this situation of non-null-terminated sun_path is explicitly called out and has a suggested remediation. That remediation is what I've implemented in this patch.

I think that the Haskell code that allocates buffers for sockaddr results should allocate one more byte than sizeof(sockaddr_un) and always zero it before the call. At this point the returned sun_path is guaranteed nul-terminated whether the OS wrote sun_len bytes (without NUL-termination) or sun_len+1 bytes (with NUL-termination). What remains is reading the right number of bytes back from sun_path, which for sockaddr_un, on platforms that lack sun_len, means reading up to the NUL terminator.

I'd like to keep this pull request focused on fixing the bug at hand and not devolve into broader project maintenance ideas. The ideas are good, but I'd prefer they be discussed in separate Issues or subsequent pull requests that implement them, which I would defer to kazu to drive appropriately. There are plenty of more defensive ways to implement things at different layers, but they are out-of-scope of what I'm trying to do here.

Are there any concrete requests on how this patch should be implemented that differ from the current implementation?

On a purely "functional" level, the patch looks OK to me, other than the question about the "non-threaded" variant on Windows, and the missing unlink in the test.

@kazu-yamamoto Linux kernel certainly null-terminated sun_path. So, null-termination related bugs should not occur on Linux.

While modern Linux kernels do generally NUL-terminate sun_path there is one exception, when the socket name is exactly 108 bytes (the maximum size of sun_path in sockaddr_un) then sun_path may not be NUL-terminated. Applications can still allocate a sockaddr_un buffer that is one byte larger than strictly needed, and zero the entire buffer. Then when the kernel writes "only" 108 bytes, the next byte will still be zero.

So we can avoid running into the issue in every case by providing a buffer that is 109 bytes long and is zeroed, but this should only needed for getsockname() (on servers) and getpeername() on clients. With unix-domain sockets and accept() the client is almost always anonymous (has a zero-length sun_path).

FWIW, I just tested with a client that does bind() before connect and observed that on every OS that supports unix-domain sockets the client socket addr is only seen in the sockaddr returned by accept(2), while getpeername() always fails:

From accept():
sun_len = 106
sun_family = 1
strlen(sun_path) = 18
sun_path = '/tmp/client-socket'

From getsockname():
sun_len = 106
sun_family = 1
strlen(sun_path) = 11
sun_path = '/tmp/socket'

getpeername: Socket is not connected

This is a apparently a long-standing well-known (by a few people) problem. So the only time you can learn the name of a non-anonymous client unix-domain socket is via accept().

Looks like #306 will be the blessed fix

@JonCoens Looks like #306 will be the blessed fix

Yes, I think so. If you're willing to review that, more eyeballs are likely better. And of course test it against the code that caused you file this PR... Thanks for all the work so far.

Superceded by #306