On Fri, 12 Sep 2014 20:01:04 +0300
Stratos Psomadakis <pso...@grnet.gr> wrote:
> 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).
I'm wondering if calling readline_restart() in the CHR_EVENT_CLOSED
can break mux support. But I won't have time to check it today. Maybe
moving the readline_restart() call to right before the
readline_show_prompt() call in the OPENED event is the best thing to do?
>
> 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;
>
>
