Network.Socket.sClose should change socket status

[amthrax]

The current implementation of sClose in Network.Socket leaves the socket in a Connected state but frees the underlying OS resource. Thus, any further operations on the socket may apply to a different OS object if something else was assigned the same FD after the socket was closed. For example, sClose'ing the socket a second time can lead to some arbitrary other file or socket being closed.

The attached test case was run in ghci 6.8.2 on Debian Linux sid, though by inspection of the Network.Socket source in 6.10.1, this is still present and the test case should work. It takes advantage of the way *NIX allocates file descriptors to demonstrate specific misbehaviors, so may or may not misbehave in the intended way on other platforms.

[amthrax]

Test case

[amthrax]

Simple patch to add a Closed state and make sClose idempotent. Operations that perform state transitions already check the socket state and will refuse to operate on a Closed socket, but this does not add checks for other operations like send/recv.

[amthrax]

I should mention that closed-state.patch is against the 6.8.2 network library. It applies cleanly (with offsets) against the 6.10.1 library, but I can't test it.

[amthrax]

Proposed patch in addition to closed-state.patch that adds checks for a valid FD in all operations. This is a fairly simple approach. In particular, it punts on the problem of one thread closing the socket in the middle of a socket operation in another thread. Arguably, the semantics of this are sufficiently unclear (especially for blocking operations) that applications should be doing their own synchronization in such situations.

[igloo]

Thanks for the patches! We'll take a look.

[igloo]

tibbe, this looks related to things you've been working on recently, and I think you are considering becoming the network maintainer? Can you take a look please?

Thanks!

[tibbe]

Sorry for the late reply. closed-state.patch looks fine to me. As for the second patch I think we need to figure out the performance consequences of checking an MVar for every op before we make the change.

Perhaps it would be worthwhile to write down which guarantees the API currently gives and could give when used in multi-threaded code. I'm not even entirely sure what guarantees we give at the moment, without your patches. If nothing else that would serve as good documentation.

[simonmar]

I pushed the closed-state.patch.

[tibbe]

Moved to  http://trac.haskell.org/network/ticket/6