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

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

[dongwon.kim]

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

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

[Philippe Mathieu-Daudé]

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

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