Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unix domain #306

Merged
merged 13 commits into from
Feb 16, 2018
31 changes: 10 additions & 21 deletions Network/Socket/Types.hsc
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

f ptr 128

------------------------------------------------------------------------
-- Socket addresses
Expand Down Expand Up @@ -893,14 +895,11 @@ type CSaFamily = (#type sa_family_t)
-- in that the value of the argument /is/ used.
sizeOfSockAddr :: SockAddr -> Int
#if defined(DOMAIN_SOCKET_SUPPORT)
sizeOfSockAddr (SockAddrUnix path) =
case path of
'\0':_ -> (#const sizeof(sa_family_t)) + length path
_ -> #const sizeof(struct sockaddr_un)
sizeOfSockAddr SockAddrUnix{} = #const sizeof(struct sockaddr_un)
#else
sizeOfSockAddr SockAddrUnix{} = error "sizeOfSockAddr: not supported"
sizeOfSockAddr SockAddrUnix{} = error "sizeOfSockAddr: not supported"
#endif
sizeOfSockAddr SockAddrInet{} = #const sizeof(struct sockaddr_in)
sizeOfSockAddr SockAddrInet{} = #const sizeof(struct sockaddr_in)
sizeOfSockAddr SockAddrInet6{} = #const sizeof(struct sockaddr_in6)

-- | Use a 'SockAddr' with a function requiring a pointer to a
Expand All @@ -920,38 +919,28 @@ withSockAddr addr f = do
-- | Write the given 'SockAddr' to the given memory location.
pokeSockAddr :: Ptr a -> SockAddr -> IO ()
#if defined(DOMAIN_SOCKET_SUPPORT)
pokeSockAddr p (SockAddrUnix path) = do
# if defined(darwin_HOST_OS)
zeroMemory p (#const sizeof(struct sockaddr_un))
# else
case path of
('\0':_) -> zeroMemory p (#const sizeof(struct sockaddr_un))
_ -> return ()
# endif
pokeSockAddr p sa@(SockAddrUnix path) = do
zeroMemory p $ fromIntegral $ sizeOfSockAddr sa
# if defined(HAVE_STRUCT_SOCKADDR_SA_LEN)
(#poke struct sockaddr_un, sun_len) p ((#const sizeof(struct sockaddr_un)) :: Word8)
# endif
(#poke struct sockaddr_un, sun_family) p ((#const AF_UNIX) :: CSaFamily)
let pathC = map castCharToCChar path
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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, throw an error.

#else
pokeSockAddr _ SockAddrUnix{} = error "pokeSockAddr: not supported"
#endif
pokeSockAddr p (SockAddrInet (PortNum port) addr) = do
#if defined(darwin_HOST_OS)
zeroMemory p (#const sizeof(struct sockaddr_in))
#endif
#if defined(HAVE_STRUCT_SOCKADDR_SA_LEN)
(#poke struct sockaddr_in, sin_len) p ((#const sizeof(struct sockaddr_in)) :: Word8)
#endif
(#poke struct sockaddr_in, sin_family) p ((#const AF_INET) :: CSaFamily)
(#poke struct sockaddr_in, sin_port) p port
(#poke struct sockaddr_in, sin_addr) p addr
pokeSockAddr p (SockAddrInet6 (PortNum port) flow addr scope) = do
# if defined(darwin_HOST_OS)
zeroMemory p (#const sizeof(struct sockaddr_in6))
# endif
# if defined(HAVE_STRUCT_SOCKADDR_SA_LEN)
(#poke struct sockaddr_in6, sin6_len) p ((#const sizeof(struct sockaddr_in6)) :: Word8)
# endif
Expand Down
1 change: 1 addition & 0 deletions network.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ test-suite spec
build-depends:
base < 5,
bytestring,
directory,
HUnit,
network,
hspec
Expand Down
41 changes: 41 additions & 0 deletions tests/SimpleSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import qualified Data.ByteString as S
import qualified Data.ByteString.Char8 as C
import Network.Socket
import Network.Socket.ByteString
import System.Directory
import System.IO.Error
import System.Timeout (timeout)

import Test.Hspec
Expand Down Expand Up @@ -147,6 +149,15 @@ spec = do
`shouldBe` (0x2001, 0x0db8, 0x85a3, 0x0000, 0x0000, 0x8a2e, 0x0370, 0x7334)
#endif

describe "unix sockets" $ do
it "basic unix sockets end-to-end" $ do
when isUnixDomainSocketAvailable $ do
let client sock = send sock testMsg
server (sock, addr) = do
recv sock 1024 `shouldReturn` testMsg
addr `shouldBe` (SockAddrUnix "")
unixTest client server

------------------------------------------------------------------------

serverAddr :: String
Expand All @@ -155,9 +166,39 @@ serverAddr = "127.0.0.1"
testMsg :: ByteString
testMsg = "This is a test message."

unixAddr :: String
unixAddr = "/tmp/network-test"

------------------------------------------------------------------------
-- Test helpers

-- | Establish a connection between client and server and then run
-- 'clientAct' and 'serverAct', in different threads. Both actions
-- get passed a connected 'Socket', used for communicating between
-- client and server. 'unixTest' makes sure that the 'Socket' is
-- closed after the actions have run.
unixTest :: (Socket -> IO a) -> ((Socket, SockAddr) -> IO b) -> IO ()
unixTest clientAct serverAct = do
test clientSetup clientAct serverSetup server
where
clientSetup = do
sock <- socket AF_UNIX Stream defaultProtocol
connect sock (SockAddrUnix unixAddr)
return sock

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing unlink before bind

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unlink is good. But is it available on Windows?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done just in case.

listen sock 1
return sock

server sock = E.bracket (accept sock) (killClientSock . fst) serverAct

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


-- | Establish a connection between client and server and then run
-- 'clientAct' and 'serverAct', in different threads. Both actions
-- get passed a connected 'Socket', used for communicating between
Expand Down