Accept may leak file descriptor when thrown an asynchronous exception

[andrewthad]

This library's implementation of accept can leak file descriptors if the thread calling accept receives an asynchronous (throwTo) exception during the execution of accept. The current version, with CPP removed to show only the codepath for Linux, looks like this:

accept :: SocketAddress sa => Socket -> IO (Socket, sa)
accept listing_sock = withNewSocketAddress $ \new_sa sz ->
    withFdSocket listing_sock $ \listing_fd -> do
        new_sock <- callAccept listing_fd new_sa sz >>= mkSocket
        new_addr <- peekSocketAddress new_sa
        return (new_sock, new_addr)
  where
     callAccept fd sa sz = with (fromIntegral sz) $ \ ptr_len -> do
       throwSocketErrorWaitRead listing_sock "Network.Socket.accept"
                        (c_accept4 fd sa ptr_len (sockNonBlock .|. sockCloexec))

After callAccept but before mkSocket, an asynchronous exception could be received. One simple solution is to mask exceptions:

new_sock <- mask_ (callAccept listing_fd new_sa sz >>= mkSocket)

Now, even if an exception is delivered, the finalizer associated with the socket should close it. Note: I am assuming that mkSocket is not interruptible, and if this assumption is wrong, none of this will work. It is defined as

mkSocket :: CInt -> IO Socket
mkSocket fd = do
    ref <- newIORef fd
    let s = Socket ref fd
    void $ mkWeakIORef ref $ close s
    return s

And the documentation in Control.Exception makes it clear that newIORef is not interruptible. I believe that mkWeakIORef is also not interruptible.

[andrewthad]

I've realized that throwSocketErrorWaitRead is almost certainly interruptible, but I believe that the suggested solution still works. It's slightly more clear to enter a masked state after calling threadWaitRead, but that would require abandoning the convenience function throwSocketErrorWaitRead.

[kazu-yamamoto]

@andrewthad Thank you for reporting this issue. Can bracketOnError solve this problem?

[andrewthad]

I don't think so. The docs say:

bracketOnError
    :: IO a         -- ^ computation to run first ("acquire resource")
    -> (a -> IO b)  -- ^ computation to run last ("release resource")
    -> (a -> IO c)  -- ^ computation to run in-between
    -> IO c         -- returns the value from the in-between computation

But we do not want to unconditionally close the socket ("release"). So I don't think this can be used.

[kazu-yamamoto]

Like bracket, but only performs the final action if there was an exception raised by the in-between computation.

As the doc says, bracketOnError conditionally release the resource.

[andrewthad]

You're right. I don't think I've ever realized that bracketOnError existed. So, something like this should work:

new_sock <- bracketOnError (callAccept listing_fd new_sa sz) close mkSocket

I've examined everything that callAccept calls (in the linux code path), and it looks like no interruptible operations occur after the socket is created, so this should completely protect against socket leaks.

[kazu-yamamoto]

@andrewthad Would you send a PR to record your credit?