| Message ID | 1410530852-13631-1-git-send-email-psomas@grnet.gr |
|---|---|
| State | New |
| Headers | show |
On Fri, 12 Sep 2014 17:07:32 +0300 Stratos Psomadakis <psomas@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? Are you using the mux feature? We also reset it in CHR_EVENT_OPENED if the mux feature is not used, why isn't that good enough? > > Signed-off-by: Stratos Psomadakis <psomas@grnet.gr> > Signed-off-by: Dimitris Aragiorgis <dimara@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;
On 12/09/2014 06:21 μμ, Luiz Capitulino wrote: > On Fri, 12 Sep 2014 17:07:32 +0300 > Stratos Psomadakis <psomas@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 <psomas@grnet.gr> >> Signed-off-by: Dimitris Aragiorgis <dimara@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;
On Fri, 12 Sep 2014 20:01:04 +0300 Stratos Psomadakis <psomas@grnet.gr> wrote: > On 12/09/2014 06:21 μμ, Luiz Capitulino wrote: > > On Fri, 12 Sep 2014 17:07:32 +0300 > > Stratos Psomadakis <psomas@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 <psomas@grnet.gr> > >> Signed-off-by: Dimitris Aragiorgis <dimara@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; > >
On 12/09/2014 08:19 μμ, Luiz Capitulino wrote: > On Fri, 12 Sep 2014 20:01:04 +0300 > Stratos Psomadakis <psomas@grnet.gr> wrote: > >> On 12/09/2014 06:21 μμ, Luiz Capitulino wrote: >>> On Fri, 12 Sep 2014 17:07:32 +0300 >>> Stratos Psomadakis <psomas@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? I did some quick tests with a mux chardev (I tried two mux'ed HMP monitors and a serial and an HMP). Calling readline_restart() in CHR_EVENT_CLOSED didn't seem to affect mux support (as far as I could tell). However, calling readline_restart() in CHR_EVENT_OPENED, just before readline_show_prompt(), resolves the issue too, and I think it makes more sense to be called at that point. If you agree, I can resend the modified patch. > >> Thanks, >> Stratos >> >>>> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr> >>>> Signed-off-by: Dimitris Aragiorgis <dimara@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; >>
On Sat, 13 Sep 2014 19:27:46 +0300 Stratos Psomadakis <psomas@grnet.gr> wrote: > On 12/09/2014 08:19 μμ, Luiz Capitulino wrote: > > On Fri, 12 Sep 2014 20:01:04 +0300 > > Stratos Psomadakis <psomas@grnet.gr> wrote: > > > >> On 12/09/2014 06:21 μμ, Luiz Capitulino wrote: > >>> On Fri, 12 Sep 2014 17:07:32 +0300 > >>> Stratos Psomadakis <psomas@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? > > I did some quick tests with a mux chardev (I tried two mux'ed HMP > monitors and a serial and an HMP). Calling readline_restart() in > CHR_EVENT_CLOSED didn't seem to affect mux support (as far as I could > tell). However, calling readline_restart() in CHR_EVENT_OPENED, just > before readline_show_prompt(), resolves the issue too, and I think it > makes more sense to be called at that point. If you agree, I can resend > the modified patch. Yes, I think that's the best. I'll just apply your respin. > > > > >> Thanks, > >> Stratos > >> > >>>> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr> > >>>> Signed-off-by: Dimitris Aragiorgis <dimara@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; > >> > >
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;
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. Signed-off-by: Stratos Psomadakis <psomas@grnet.gr> Signed-off-by: Dimitris Aragiorgis <dimara@grnet.gr> --- monitor.c | 1 + 1 file changed, 1 insertion(+)