Fixed double mutex acquisition deadlock when recreating UDP session #372
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| @@ -55,13 +55,15 @@ func (h *udpHandler) ReceiveTo(tunConn lwip.UDPConn, data []byte, destAddr *net. | |||
|
|
|||
| h.mu.Lock() | |||
| reqSender, ok := h.senders[laddr] | |||
| h.mu.Unlock() | |||
| if !ok { | |||
| if reqSender, err = h.newSession(tunConn); err != nil { | |||
There was a problem hiding this comment.
I would suggest adding a h.mu.Unlock() before returning. The goal is to protect both h.senders[laddr] read and h.senders[laddr] = reqSender write.
There was a problem hiding this comment.
Unfortunately, this is not possible since newSession will do h.mu.Lock() as well, so if it's not unlocked by then, it will always deadlock. As I understand, this execution path will always cause deadlock in the current code base.
I understand there may be an issue regarding access of h.senders[laddr], but fixing it requires uprooting way more code that I understand at the moment. So I suggest to fix the hard deadlock issue now with minimal change - if there are any issues with UDP sessions access synchronization in the future, they may be taken care of separately. After all, this piece of code was essentially a dead end so there may be more bugs there.
There was a problem hiding this comment.
Perhaps I didn't understand you well and perhaps I didn't explain the issue well, so it's not possible to always do all h.senders[laddr] access inside the same mutex as they are interleaved with newSession() sometimes which requires mutex to be unlocked first.
minor: Opportunistically clean-up stale route rule in outline-cli closes Jigsaw-Code#342
|
@jyyi1 please re-review - I've addressed formatting issue and replied on comments. |
minor: Opportunistically clean-up stale route rule in outline-cli
closes #342