-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Race condition in asyncdispatch.closeSocket on Unix #12652
Comments
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? |
I can see two ways:
|
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. |
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? |
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
|
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
andsend
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
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.
The text was updated successfully, but these errors were encountered: