Re: [PATCH] ui/gtk: Negative Page number is not valid

[Kim, Dongwon]

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;
> >       }