Unix domain #306

kazu-yamamoto · 2018-02-07T03:27:00Z

Fix: accept should return SockAddrUnix "" for unix domain sockets.

4e257c5 is a test case created by @JonCoens. It shows that the bug exists.
adc9f8b is a fix for the bug.
My concern is #const sizeof(struct sockaddr_un). If we add 1 to this size, the tests fail.

author: JonCoens in haskell#305.

vdukhovni · 2018-02-07T03:33:52Z

Two comments:

The accept() system call generally returns "" for sun_path, but not always. That's only true for anonymous clients that don't bind their own AF_UNIX socket name first. However, in the test in question, the client will be anonymous, so in this test sun_path should indeed come out empty.
As for adding 1 I was likely doing it too globally, what we should ideally do is add 1 only when allocating a socket buffer for the output and zeroing it. After that, and everywhere else we should use the natural size.

The idea of adding 1 is to ensure that there's always room for the terminating NUL (that we provide) even if the bound socket path has the maximum possible length.

vdukhovni · 2018-02-07T03:38:23Z

Network/Socket/Types.hsc

@@ -820,7 +820,9 @@ withSocketAddress addr f = do
    allocaBytes sz $ \p -> pokeSocketAddress p addr >> f (castPtr p) sz

 withNewSocketAddress :: SocketAddress sa => (Ptr sa -> Int -> IO a) -> IO a
-withNewSocketAddress f = allocaBytes 128 $ \ptr -> f ptr 128
+withNewSocketAddress f = allocaBytes 128 $ \ptr -> do
+    zeroMemory ptr 128


The constant 128 should probably be computed from max(sizeof(sockaddr_storage), sizeof(sockaddr_un)) or similar (some systems don't have a sockaddr_storage, and sockaddr_in and sockaddr_in6 might need to be used instead). And it is here that we might want to add and zero one more byte.

Currently, memory is cheap. So, let's just use 128 which is the size of struct sockaddr_strage. +1 is not necessary for 128.

I see, so the idea is that 128 is large enough for sockaddr_un on all platforms, and naturally also for AF_INET and AF_INET6` socket addresses. Perhaps a comment to that effect and a named constant at the top of the file? I am a bit averse to unexplained magic numbers in code.

vdukhovni · 2018-02-07T03:53:31Z

Network/Socket/Types.hsc

-        poker = case path of ('\0':_) -> pokeArray; _ -> pokeArray0 0
-    poker ((#ptr struct sockaddr_un, sun_path) p) pathC
+    -- the buffer is already filled with nulls.
+    pokeArray ((#ptr struct sockaddr_un, sun_path) p) pathC


This might not be safe when path is unexpectedly long. We now have sizeOfSockAddr for unix-domain sockets as a constant sizeof(sockaddr_un), and it seems possible that trying to use a longer path will corrupt memory?

I tested this with a short program that calls allocaBytes 128 and then pokeArray p with an array of > 128 C chars. The compiler and run-time did not object, and dutifully dumped core when the array was large enough. So here, we need to fail if SockAddrUnix path contains a path that is too long (which is a bit shorter than 128 bytes, since we have to account for the sun_family structure member. It may be unwise to assume that it is the standard 2 bytes, so we could either compute the overhead explicitly, or just cap the size at 120 (again as a named constant, properly commented at the top of the file).

Now, throw an error.

vdukhovni · 2018-02-07T03:54:36Z

tests/SimpleSpec.hs

+
+    serverSetup = do
+        sock <- socket AF_UNIX Stream defaultProtocol
+        bind sock (SockAddrUnix unixAddr)


missing unlink before bind

A more careful test might check whether the directory entry of that name (if it exists) is a socket (not a regular file or directory) and only unlink in that case, but "/tmp/network-socket" should be safe enough. We should probably also unlink when the test is done.

unlink is good. But is it available on Windows?

It would be the same as the existing cleanup after the test.

E.tryJust (guard . isDoesNotExistError) $ removeFile unixAddr

Note also that Windows has no unix-domain sockets, so both the test and the unlink can be skipped on Windows.

Done just in case.

vdukhovni · 2018-02-07T03:57:01Z

tests/SimpleSpec.hs

+    killClientSock sock = do
+        shutdown sock ShutdownBoth
+        close sock
+        E.tryJust (guard . isDoesNotExistError) $ removeFile unixAddr


Ah, there's the unlink after the test...

vdukhovni · 2018-02-07T16:58:10Z

Network/Socket/Types.hsc

@@ -910,6 +914,9 @@ withSockAddr addr f = do
    let sz = sizeOfSockAddr addr
    allocaBytes sz $ \p -> pokeSockAddr p addr >> f (castPtr p) sz

+unixPathMax :: Int
+unixPathMax = 108
+


Note that on NetBSD, FreeBSD and Darwin the size of sun_path in sockaddr_un is 104 bytes. However, allocating larger sockaddr buffers (than sizeof(sockaddr_un)) allows use of longer (NUL-terminated) pathnames. So we can in fact bind and connect to sockets with names that are 108 (or more) bytes in length on these systems, provided the sockaddr buffer is long enough. The only issue is that not all clients will be able to interoperate with sockets whose names that exceed the default sizeof(sun_path) of sockaddr_un, but that's the user's problem if he/she chooses to create such sockets.

If there are still Linux systems that fail to NUL-terminate returned socket addresses, we should however promise to the OS one less byte than we allocated and zeroed:

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

This is likely needed everywhere, since maximal length pathnames might otherwise not get a NUL terminator even on BSD and Darwin.

That said, note that this pull request uses sockaddrStorageLen = 128 only to allocate storage for socket addresses we get back from the OS in withNewSocketAddress but still leaves

sizeOfSockAddr SockAddrUnix{} = #const sizeof(struct sockaddr_un)

as the size used in withSocketAddress to allocate socket addresses for passing to the OS. Therefore, on BSD and Darwin pathnames of 108 bytes will overflow the allocated buffer when binding paths of 108 bytes.

Since we need to support initialization of the same size sockets as we're willing to receive (and in particular path of up 108 bytes on BSD and the like, where this requires socket paths longer than fit in the stock sockaddr_un) we need to also allocate space for sockaddrStorageLen bytes when initializing AF_UNIX sockaddrs with a user provided path.
Since we'll refuse to actually write more than 108 bytes of path in pokeSockAddr, the result will always be NUL-terminated.

sizeOfSockAddr SockAddrUnix{} = #const sizeof(struct sockaddr_un)

to

sizeOfSockAddr SockAddrUnix{path} = sockaddrStorageLen

Thank you for this investigation. I agree with your approach. So, I pushed the fix.

vdukhovni · 2018-02-07T17:09:25Z

One more thing: in peekSockAddr we should be using peekCAString not peekCString, socket path names are octet strings, not locale-specific filenames. Especially since pokeSockAddr is not doing locale conversions, peekSockAddr should also avoid these.

They should probably have been represented as ByteStrings, not Strings. If the API in 3.0 is different enough to allow that change, that might also make sense.

kazu-yamamoto · 2018-02-09T05:59:50Z

I'm busy now. I will come back to this issue on 15th Feb.

kazu-yamamoto · 2018-02-15T07:44:03Z

The accept() system call generally returns "" for sun_path, but not always. That's only true for anonymous clients that don't bind their own AF_UNIX socket name first. However, in the test in question, the client will be anonymous, so in this test sun_path should indeed come out empty.

Good point. But in this case, clients are anonymous. So, "" should be OK.

kazu-yamamoto · 2018-02-15T07:56:52Z

One more thing: in peekSockAddr we should be using peekCAString not peekCString, socket path names are octet strings, not locale-specific filenames. Especially since pokeSockAddr is not doing locale conversions, peekSockAddr should also avoid these.

Done.

They should probably have been represented as ByteStrings, not Strings. If the API in 3.0 is different enough to allow that change, that might also make sense.

I would like to discuss this kind of changes in a separate issue. If you wish, please record this issue.

kazu-yamamoto · 2018-02-15T08:12:07Z

I'm waiting for results from CI.

kazu-yamamoto · 2018-02-15T08:29:31Z

c3e2e2e does not work for Linux.

vdukhovni · 2018-02-15T18:31:53Z

Can you be more explicit about what breaks?

…gths

vdukhovni · 2018-02-15T20:22:25Z

See pull-request 1 against your unix-domain branch.

kazu-yamamoto · 2018-02-16T04:14:33Z

@vdukhovni OK. Thank you for your thorough review. I think we can merge this now.

@JonCoens I will merge this instead of #305 and try to backport this to 2.6. Thank you for reporting the bug.

kazu-yamamoto · 2018-02-16T04:15:37Z

@vdukhovni Oh, you are assigned as a reviewer. Please approve this!

vdukhovni

Separately there should probably be documentation that path in SockAddrUnix path, though defined as String, must hold only characters from 1-255, and is not subject to any encoding to/from String. Whether this warrants a type change to ByteString is less clear. That could do more harm than good.

kazu-yamamoto · 2018-02-16T06:43:23Z

Thanks. Merged.

I will take care of doc stuff.

kazu-yamamoto added 5 commits February 6, 2018 13:52

a test case for unix domain.

4e257c5

author: JonCoens in haskell#305.

fixing addr from accept.

adc9f8b

pokeSockAddr should always fill sockaddr for safety.

93076eb

just using pokeArray thanks to nulls.

0cd928b

refactoring.

3ee89f3

vdukhovni reviewed Feb 7, 2018

View reviewed changes

kazu-yamamoto added 2 commits February 7, 2018 21:07

explaining the magic number 128.

e877e1b

checking length of SockAddrUnix.

c8042c7

vdukhovni reviewed Feb 7, 2018

View reviewed changes

JonCoens mentioned this pull request Feb 7, 2018

Peek sockaddr for unix domain sockets according to non-null-terminating semantics #305

Closed

removing unix socket file just in case.

8771875

using peekCAString instead of peekCString.

831fac6

using sockaddrStorageLen for sizeOfSockAddr SockAddrUnix.

c3e2e2e

kazu-yamamoto mentioned this pull request Feb 15, 2018

2.6.3.4 and 2.7.0.0 #296

Closed

Fix unix-domain bind() failure with longer than standard sockaddr len…

1f3806f

…gths

vdukhovni mentioned this pull request Feb 15, 2018

Unix domain fixes kazu-yamamoto/network#1

Closed

kazu-yamamoto added 2 commits February 16, 2018 11:22

using NULL intead of 0.

d0b11fb

rescuing Windows.

b80dbeb

vdukhovni approved these changes Feb 16, 2018

View reviewed changes

kazu-yamamoto added a commit to kazu-yamamoto/network that referenced this pull request Feb 16, 2018

Merge PR haskell#306.

a4e86b5

kazu-yamamoto merged commit b80dbeb into haskell:master Feb 16, 2018

kazu-yamamoto deleted the unix-domain branch February 16, 2018 06:43

kazu-yamamoto mentioned this pull request Jan 30, 2019

Set correct sockaddr length for abstract addresses #374

Closed

kazu-yamamoto mentioned this pull request Apr 23, 2019

Fixing unix socket on 2.8 #400

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unix domain #306

Unix domain #306

kazu-yamamoto commented Feb 7, 2018 •

edited

Loading

vdukhovni commented Feb 7, 2018

vdukhovni Feb 7, 2018

kazu-yamamoto Feb 7, 2018

vdukhovni Feb 7, 2018

kazu-yamamoto Feb 7, 2018

vdukhovni Feb 7, 2018

vdukhovni Feb 7, 2018

kazu-yamamoto Feb 7, 2018

vdukhovni Feb 7, 2018

vdukhovni Feb 7, 2018

kazu-yamamoto Feb 7, 2018

vdukhovni Feb 7, 2018

kazu-yamamoto Feb 15, 2018

vdukhovni Feb 7, 2018

vdukhovni Feb 7, 2018 •

edited

Loading

kazu-yamamoto Feb 15, 2018

vdukhovni commented Feb 7, 2018

kazu-yamamoto commented Feb 9, 2018

kazu-yamamoto commented Feb 15, 2018

kazu-yamamoto commented Feb 15, 2018

kazu-yamamoto commented Feb 15, 2018

kazu-yamamoto commented Feb 15, 2018

vdukhovni commented Feb 15, 2018

vdukhovni commented Feb 15, 2018

kazu-yamamoto commented Feb 16, 2018

kazu-yamamoto commented Feb 16, 2018

vdukhovni left a comment •

edited

Loading

kazu-yamamoto commented Feb 16, 2018

Unix domain #306

Unix domain #306

Conversation

kazu-yamamoto commented Feb 7, 2018 • edited Loading

vdukhovni commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdukhovni Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdukhovni commented Feb 7, 2018

kazu-yamamoto commented Feb 9, 2018

kazu-yamamoto commented Feb 15, 2018

kazu-yamamoto commented Feb 15, 2018

kazu-yamamoto commented Feb 15, 2018

kazu-yamamoto commented Feb 15, 2018

vdukhovni commented Feb 15, 2018

vdukhovni commented Feb 15, 2018

kazu-yamamoto commented Feb 16, 2018

kazu-yamamoto commented Feb 16, 2018

vdukhovni left a comment • edited Loading

Choose a reason for hiding this comment

kazu-yamamoto commented Feb 16, 2018

kazu-yamamoto commented Feb 7, 2018 •

edited

Loading

vdukhovni Feb 7, 2018 •

edited

Loading

vdukhovni left a comment •

edited

Loading