Ticket #6 (closed defect: fixed)

Opened 6 years ago

Last modified 6 years ago

Network.Socket.sClose should change socket status

Reported by: tibbe Owned by: tibbe
Priority: major Milestone:
Component: network Version:
Keywords: Cc:

Description

Initially reported here: http://hackage.haskell.org/trac/ghc/ticket/2996


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.


Test case


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.


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.


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.


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


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!


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.


I pushed the closed-state.patch.

Change History

Changed 6 years ago by tibbe

  • owner set to tibbe
  • status changed from new to assigned

Changed 6 years ago by anonymous

Any particular reason why sClose is idempotent? I'd have expected it to throw an exception on an attempt to close a closed socket.

Changed 6 years ago by amthrax

I made sClose idempotent to mirror the interface of hClose, which is likewise idempotent.

Changed 6 years ago by tibbe

  • status changed from assigned to closed
  • resolution set to fixed

I'm closing this bug. If you believe we can add checks to all operations without degrading performance please open a new bug and we can discuss it there.

Note: See TracTickets for help on using tickets.