Possible deadlock between requestStream and callback functions in rpc.go

[ishustava]

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:

go-libvirt/rpc.go

Lines 271 to 285 in ab4e783

	l.register(serial, c)
	defer func() {
	l.cmux.Lock()
	defer l.cmux.Unlock()
	l.deregister(serial)
	}()
	err := l.socket.SendPacket(serial, proc, program, payload, socket.Call,
	socket.StatusOK)
	if err != nil {
	return response{}, err
	}
	resp, err := l.getResponse(c)

callback function:

go-libvirt/rpc.go

Lines 75 to 85 in ab4e783

	func (l *Libvirt) callback(id int32, res response) {
	l.cmux.Lock()
	defer l.cmux.Unlock()
	c, ok := l.callbacks[id]
	if !ok {
	return
	}
	c <- res
	}

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).