Re: [Qemu-devel] [synnefo-devel] Re: [PATCH resend 1/2] monitor: Reset HMP mon->rs on CHR_EVENT_CLOSED

[Stratos Psomadakis]

On 12/09/2014 06:21 μμ, Luiz Capitulino wrote:
> On Fri, 12 Sep 2014 17:07:32 +0300
> Stratos Psomadakis <address@hidden> wrote:
>
>> Commit cdaa86a54 ("Add G_IO_HUP handler for socket chardev") exposed a bug in
>> the way the HMP monitor handles its command buffer. When a client closes the
>> connection to the monitor, tcp_chr_read() will detect the G_IO_HUP condition
>> and call tcp_chr_disconnect() to close the server-side connection too. Due to
>> the fact that monitor reads 1 byte at a time (for each tcp_chr_read()), the
>> monitor readline state / buffers might contain junk (i.e. a half-finished
>> command).  Thus, without calling readline_restart() on mon->rs upon
>> CHR_EVENT_CLOSED, future HMP commands will fail.
> What's your reproducer?

We have a script that opens a connection to the HMP socket and starts
sending 'info version' commands to the monitor in a loop. If we kill the
script (in the middle of the loop) and re-run it, we get "unknown
command" errors from the HMP ("unknown command: 'infinfo'" for example).

> Are you using the mux feature?

Nope (on the cli we use '-monitor unix:<path>.mon,server,nowait' for the
HMP).

> We also reset it
> in CHR_EVENT_OPENED if the mux feature is not used, why isn't that
> good enough?

I checked the code and on CHR_EVENT_OPENED the monitor calls
readline_show_prompt (when not using mux). This resets the
last_cmd_index/size readline variables, but the cmd_buf_index/size
remains intact. I think that readline_restart() is necessary in order to
cleanup the readline cmd buf (either in CHR_EVENT_OPENED or in
CHR_EVENT_CLOSED).

Thanks,
Stratos

>
>> Signed-off-by: Stratos Psomadakis <address@hidden>
>> Signed-off-by: Dimitris Aragiorgis <address@hidden>
>> ---
>>  monitor.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 34cee74..7857300 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -5252,6 +5252,7 @@ static void monitor_event(void *opaque, int event)
>>          break;
>>  
>>      case CHR_EVENT_CLOSED:
>> +        readline_restart(mon->rs);
>>          mon_refcount--;
>>          monitor_fdsets_cleanup();
>>          break;


-- 
Stratos Psomadakis
<address@hidden>

signature.asc
Description: OpenPGP digital signature