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

Safer socket #336

Closed
wants to merge 3 commits into from
Closed

Conversation

kazu-yamamoto
Copy link
Collaborator

socket closes the file descriptor on exception.
This should fix #166 in master.

@kazu-yamamoto kazu-yamamoto requested a review from eborden June 26, 2018 07:18
s <- mkSocket fd
unsetIPv6Only s `E.onException` close s
return s
where
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the improvement in readability of moving the ifdefs to where.

c_socket (packFamily family) c_stype protocol
setNonBlock fd `E.onException` c_close fd
s <- mkSocket fd
unsetIPv6Only s `E.onException` close s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these be closing and throwing? Otherwise socket will return a Socket that has already been closed and file descriptor re-use by the OS could lead to us accidentally sending to an unknown socket.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually reuse is not an issue here since close changes it to -1, but is above with using vanilla c_close.

c_stype <- modifyFlag <$> packSocketTypeOrThrow "socket" stype
fd <- throwSocketErrorIfMinus1Retry "Network.Socket.socket" $
c_socket (packFamily family) c_stype protocol
setNonBlock fd `E.onException` c_close fd
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this result in a Socket with a closed file descriptor within its IORef?

Copy link
Collaborator

@eborden eborden left a comment

Choose a reason for hiding this comment

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

Woops, ignore the issues above. onException is fine here, since it isn't catching anything, but just a flavor of bracket.

Copy link
Collaborator

@eborden eborden left a comment

Choose a reason for hiding this comment

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

Actually I'm not sure. onException is defined as:

onException :: IO a -> IO b -> IO a
onException io what = io `catch` \e -> do _ <- what
                                          throwIO (e :: SomeException)

Is that sufficient to prevent async exceptions from causing file descriptors leaking? It seems we might want to utilize bracketOnError to ensure masking and do something like:

    bracketOnError create c_close $ \fd -> do
        setNonBlock fd
        s <- mkSocket fd
        unsetIPv6Only s
        return s
  where
    create =
        c_stype <- modifyFlag <$> packSocketTypeOrThrow "socket" stype
        throwSocketErrorIfMinus1Retry "Network.Socket.socket" $
                  c_socket (packFamily family) c_stype protocol

@kazu-yamamoto
Copy link
Collaborator Author

@eborden My code is safe only for synchronous exceptions while your code is safe even for asynchronous exceptions! Also, close is not necessary to close Socket since it is not managed by the IO manager yet. No deadlock here.

OK. I will adopt your code. Thank you for your suggestion!

kazu-yamamoto added a commit to kazu-yamamoto/network that referenced this pull request Jul 9, 2018
@kazu-yamamoto
Copy link
Collaborator Author

Rebased and merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

socket is not asynchronous exception safe
2 participants