Race condition in asyncdispatch.closeSocket on Unix

[endragor]

closeSocket's unix implementation first closes the socket handle, making it available for OS to allocate to the process for other purposes, then invokes callbacks, which call recv and send on the same handle. This makes it possible for recv and send to read/write data from/to another socket or file or whatever is re-bound to the same handle, if there are other threads opening files/sockets. This opens up a way for unexpected behavior and security vulnerabilities.

The code is:

Nim/lib/pure/asyncdispatch.nim

Lines 1269 to 1276 in f22d3c7

	sock.SocketHandle.close()
	# We need to unblock the read and write callbacks which could still be
	# waiting for the socket to become readable and/or writeable.
	for cb in data.readList & data.writeList:
	if not cb(sock):
	raise newException(
	ValueError, "Expecting async operations to stop when fd has closed."
	)

As a side note, if read cb throws an exception, which is possible, since it calls user code, then the subsequent callbacks in the list will be leaked, i.e. if something awaits a send on the same socket, it will await forever. I don't think that is the desired behavior here.

[dom96]

As far as the primary issue is concerned I'm not sure what we can do to resolve it off the top of my head. Any suggestions?

[endragor]

I can see two ways:

1. Store the flags and futures in addition to callbacks and directly fail or complete them depending on SafeDisconn flag within closeSocket() after closing the socket.

2. Call shutdown(socket, SHUT_RDWR), then callbacks, then close(). recv() and send() syscalls must fail after invoking shutdown(). Notice the user may have called shutdown on their own before that and I'm not sure what the behavior is if you call shutdown twice on the same socket.

[endragor]

Actually, since shutdown() is only applicable to established TCP connections and not to server sockets or UDP sockets, I don't think (2) is a solution.

[dom96]

Store the flags and futures in addition to callbacks and directly fail or complete them depending on SafeDisconn flag within closeSocket() after closing the socket.

Okay, we might be able to do that. Although it won't be easy and I'm still not 100% sure it's possible.

I think as a short-term fix we should put a lock around this code to make sure no weirdness occurs for parallel code. What do you think?

[endragor]

The problem may actually occur even for single-threaded code - in case callbacks open sockets. In my fork of Nim (I have to use an older version) I fixed this by adding a bool parameter to callbacks. If it's true, the callbacks assume the socket is closed and react accordingly.

addEvent, which is an exported procedure, uses the same callback though and it has to be changed to the original signature (without the bool argument).