Re: [PATCH] monitor/qmp: fix race with clients disconnecting early

[Markus Armbruster <armbru@redhat.com>]

Stefan Reiter <s.reiter@proxmox.com> writes:

> On 26/08/2021 15:50, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>> [...]
>> 
>>> Let me re-explain the bug in my own words, to make sure I understand.
>>>
>>> A QMP monitor normally runs in the monitor I/O thread.
>>>
>>> A QMP monitor can serve only one client at a time.
>>>
>>> It executes out-of-band commands right as it reads them.  In-band
>>> commands are queued, and executed one after the other in the main loop.
>>>
>>> Command output is buffered.  We write it out as fast as the character
>>> device can take it.  If a write fails, we throw away the entire buffer
>>> contents.
>>>
>>> A client can disconnect at any time.  This throws away the queue.  An
>>> in-band command may be executing in the main loop.  An out-of-band
>>> command may be executing in the monitor's thread.
>>>
>>> Such commands (if any) are not affected by the disconnect.  Their output
>>> gets buffered, but write out fails, so it's thrown away.
>>>
>>> *Except* when another client connects quickly enough.  Then we send it
>>> output meant for the previous client.  This is wrong.  I suspect this
>>> could even send invalid JSON.
>>>
>
> I'm not sure this is the case. In all testing I have *never*
> encountered the case of broken JS or any other indication that partial
> output was received.

We buffer monitor output without bounds, and try to write it out as
quickly as the client will take it.  I think short writes are possible
when the client is slow to read.  Such short writes can write partial
JSON expressions; the operating system doesn't know or care.  If the
client disconnects right then, the buffer starts with the remainder of
the JSON expression.  If the buffer is sent to the next client...

> I think the issue is just between starting to execute the command in
> the BH and the new client connecting... can the CHR_EVENTs even be
> triggered when the main thread is busy with the BH?

Good question.  We better find out.

>>> Special case: if in-band command qmp_capabilities is executing when the
>>> client disconnects, and another client connects before the command flips
>>> the monitor from capabilities negotiation mode to command mode, that
>>> client starts in the wrong mode.
>>
>> What the cases have in common: disconnect + connect in monitor I/O
>> thread and the command executing in the main thread change the same
>> monitor state.
>>
>> You observed two issues: one involving the output buffer (new client
>> receives old client's output), and one involving monitor mode (new
>> client has its mode flipped by the old client's qmp_capabilities
>> command).
>>
>> Any monitor state accessed by commands could cause issues.  Right now, I
>> can see only one more: file descriptors.  Cleaning them up on disconnect
>> could mess with the command.
>
> Right, that would make sense, but also only an issue if the reconnect
> can happen in the middle of the command itself.
>
> Maybe we can acquire some kind of lock during actual in-band QMP
> command execution?

Yes, the root cause is insufficient synchronization between in-band
command running in the main loop and disconnect / connect running in the
monitor I/O thread, and synchronizing them properly feels like the best
chance for a complete and reliable fix.

Before the OOB work, everything was in the main thread.  I figure the
misbehavior you found was not possible then.

Synchronization so that disconnect is delayed until after the in-band
command in flight completes should get us back to that state.  But I'm
afraid it could regress the OOB feature.

The point of OOB commands is "exec-oob executes right away no matter
what".  To make that possible OOB-capable commands are severely
restricted in what they can do, and the client needs to limit the number
of commands in flight.

"Right away" should be possible even when the client has to connect
first.  I'm not sure that's actually the case now.  Delaying until after
in-band completes would definitely kill it, though.

I'm afraid the synchronization needs to be more involved.

>> [...]