| Message ID | 20240626000835.248538-1-dongwon.kim@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ui/gtk: Negative Page number is not valid | expand |
Hi Dongwon, On 26/6/24 02:08, dongwon.kim@intel.com wrote: > From: Dongwon Kim <dongwon.kim@intel.com> > > Negative page number means the page with that number does not > belong to the notebook so it shouldn't be used as a valid page > number in gd_vc_find_by_page. This function should just return > null in such case. > > This change, however, will cause a segfault during detaching > /untabifying process in gtk_release_modifiers because the > current VC's page number suddenly becomes '-1' as soon as > the VC is detached, which makes gd_vc_find_by_page return > null. So gtk_release_modifiers should do the null check on > VC returned from gd_vc_find_by_page. > > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> > --- > ui/gtk.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/ui/gtk.c b/ui/gtk.c > index 93b13b7a30..1f8523fd81 100644 > --- a/ui/gtk.c > +++ b/ui/gtk.c > @@ -164,7 +164,7 @@ static VirtualConsole *gd_vc_find_by_page(GtkDisplayState *s, gint page) The caller should check gtk_notebook_get_current_page() != -1. We might assert(page >= 0) here. > for (i = 0; i < s->nb_vcs; i++) { > vc = &s->vc[i]; > p = gtk_notebook_page_num(GTK_NOTEBOOK(s->notebook), vc->tab_item); > - if (p == page) { > + if (p > -1 && p == page) { Then this is not necessary. > return vc; > } > } return NULL; I wonder about returning NULL, maybe just g_assert_not_reached(); > @@ -357,7 +357,7 @@ static void gtk_release_modifiers(GtkDisplayState *s) > { > VirtualConsole *vc = gd_vc_find_current(s); > > - if (vc->type != GD_VC_GFX || > + if (!vc || vc->type != GD_VC_GFX || Then this is not necessary. > !qemu_console_is_graphic(vc->gfx.dcl.con)) { > return; > }
On 6/26/2024 10:06 AM, Philippe Mathieu-Daudé wrote: > Hi Dongwon, > > On 26/6/24 02:08, dongwon.kim@intel.com wrote: >> From: Dongwon Kim <dongwon.kim@intel.com> >> >> Negative page number means the page with that number does not >> belong to the notebook so it shouldn't be used as a valid page >> number in gd_vc_find_by_page. This function should just return >> null in such case. >> >> This change, however, will cause a segfault during detaching >> /untabifying process in gtk_release_modifiers because the >> current VC's page number suddenly becomes '-1' as soon as >> the VC is detached, which makes gd_vc_find_by_page return >> null. So gtk_release_modifiers should do the null check on >> VC returned from gd_vc_find_by_page. >> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Cc: Marc-André Lureau <marcandre.lureau@redhat.com> >> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> >> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> >> --- >> ui/gtk.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/ui/gtk.c b/ui/gtk.c >> index 93b13b7a30..1f8523fd81 100644 >> --- a/ui/gtk.c >> +++ b/ui/gtk.c >> @@ -164,7 +164,7 @@ static VirtualConsole >> *gd_vc_find_by_page(GtkDisplayState *s, gint page) > > The caller should check gtk_notebook_get_current_page() != -1. > > We might assert(page >= 0) here. We could do that but it means there should be more checks in other functions where gd_vc_find_by_page is called, like gd_vc_find_current. And we just can't do assert in\ gd_vc_find_current because detached VC has the page number of -1. > >> for (i = 0; i < s->nb_vcs; i++) { >> vc = &s->vc[i]; >> p = gtk_notebook_page_num(GTK_NOTEBOOK(s->notebook), >> vc->tab_item); >> - if (p == page) { >> + if (p > -1 && p == page) { > > Then this is not necessary. > >> return vc; >> } >> } > return NULL; > > I wonder about returning NULL, maybe just > > g_assert_not_reached(); > >> @@ -357,7 +357,7 @@ static void gtk_release_modifiers(GtkDisplayState *s) >> { >> VirtualConsole *vc = gd_vc_find_current(s); >> - if (vc->type != GD_VC_GFX || >> + if (!vc || vc->type != GD_VC_GFX || > > Then this is not necessary. > >> !qemu_console_is_graphic(vc->gfx.dcl.con)) { >> return; >> } >
diff --git a/ui/gtk.c b/ui/gtk.c index 93b13b7a30..1f8523fd81 100644 --- a/ui/gtk.c +++ b/ui/gtk.c @@ -164,7 +164,7 @@ static VirtualConsole *gd_vc_find_by_page(GtkDisplayState *s, gint page) for (i = 0; i < s->nb_vcs; i++) { vc = &s->vc[i]; p = gtk_notebook_page_num(GTK_NOTEBOOK(s->notebook), vc->tab_item); - if (p == page) { + if (p > -1 && p == page) { return vc; } } @@ -357,7 +357,7 @@ static void gtk_release_modifiers(GtkDisplayState *s) { VirtualConsole *vc = gd_vc_find_current(s); - if (vc->type != GD_VC_GFX || + if (!vc || vc->type != GD_VC_GFX || !qemu_console_is_graphic(vc->gfx.dcl.con)) { return; }