On 12/09/2014 06:21 μμ, Luiz Capitulino wrote:
> On Fri, 12 Sep 2014 17:07:32 +0300
> Stratos Psomadakis <pso...@grnet.gr> 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 <pso...@grnet.gr>
>> Signed-off-by: Dimitris Aragiorgis <dim...@grnet.gr>
>> ---
>> 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
<pso...@grnet.gr>
signature.asc
Description: OpenPGP digital signature
