ui/gtk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
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)
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;
}
--
2.34.1
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; >> } >
© 2016 - 2024 Red Hat, Inc.