digitalocean / digitalocean/go-libvirt
Possible deadlock between `requestStream` and `callback` functions in rpc.go
- Dominant language
- Go
- Stars
- 1.1k
- Forks
- 143
- PR merge metrics
- No merged PRs in 30d
Description
## Problem
There's a possible deadlock in the `requestStream` and `callback` functions that can be triggered if there's an error sending packet:
Relevant lines from `requestStream` function:
https://github.com/digitalocean/go-libvirt/blob/ab4e783fc40ffea87b7016abf550f5e739d37733/rpc.go#L271-L285
`callback` function:
https://github.com/digitalocean/go-libvirt/blob/ab4e783fc40ffea87b7016abf550f5e739d37733/rpc.go#L75-L85
Notice that the `requestStream` registers an unbuffered channel and the `callback` function holds the lock for the `callbacks` internal map until the send to the channel succeeds (that is because an unbuffered channel will block until there's a receiver).
A deadlock can be triggered in the following scenario:
1. We register the channel in the `requestStream` function on line 271
2. There's an error sending packet on line 279 so a defer function on lines 272-277 to deregister the callback channel should run before we return the error. This also means that we never read from the channel to get the response.
3. At the same time, the `callback` function was run in a different goroutine and is on line 84 sending response to the callback channel
4. The `callback` function holds a lock until the send to the channel succeeds which in this case will be forever because there's no channel receiver. This means the defer function to deregister the callback could not acquire a lock to close the callback channel and the program hangs indefinitely.
## Possible solutions
The issue here I think is the `callback` function holding a lock while sending to an unbuffered channel. If the send on the channel blocks, then it will keep the lock indefinitely.
So there are two possible solutions:
1. Use a buffered channel which shouldn't block sending responses so long as it's not full
2. Release the lock before sending to the channel. This I believe should be safe to do and will ensure that the channel will never lock the map indefinitely (even with the buffered channel it could potentially be possible).
Contributor guide
Assessment
This issue has not been assessed yet.