1. Patchew
  2. QEMU
  3. View series

[PATCH] ui: fix crash when there are no active_console

marcandre.lureau@redhat.com posted 1 patch 7 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230911140638.1458156-1-marcandre.lureau@redhat.com
Maintainers: Gerd Hoffmann <kraxel@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>
ui/console.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
[PATCH] ui: fix crash when there are no active_console
Posted by marcandre.lureau@redhat.com 7 months, 2 weeks ago 
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
0x0000555555888630 in dpy_ui_info_supported (con=0x0) at ../ui/console.c:812
812	    return con->hw_ops->ui_info != NULL;
(gdb) bt
#0  0x0000555555888630 in dpy_ui_info_supported (con=0x0) at ../ui/console.c:812
#1  0x00005555558a44b1 in protocol_client_msg (vs=0x5555578c76c0, data=0x5555581e93f0 <incomplete sequence \373>, len=24) at ../ui/vnc.c:2585
#2  0x00005555558a19ac in vnc_client_read (vs=0x5555578c76c0) at ../ui/vnc.c:1607
#3  0x00005555558a1ac2 in vnc_client_io (ioc=0x5555581eb0e0, condition=G_IO_IN, opaque=0x5555578c76c0) at ../ui/vnc.c:1635

Fixes:
https://issues.redhat.com/browse/RHEL-2600

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/ui/console.c b/ui/console.c
index 90ae4be602..0f31ecece6 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -808,6 +808,9 @@ bool dpy_ui_info_supported(QemuConsole *con)
     if (con == NULL) {
         con = active_console;
     }
+    if (con == NULL) {
+        return false;
+    }
 
     return con->hw_ops->ui_info != NULL;
 }
@@ -817,6 +820,9 @@ const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con)
     if (con == NULL) {
         con = active_console;
     }
+    if (con == NULL) {
+        return NULL;
+    }
 
     return &con->ui_info;
 }
@@ -826,6 +832,9 @@ int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info, bool delay)
     if (con == NULL) {
         con = active_console;
     }
+    if (con == NULL) {
+        return -1;
+    }
 
     if (!dpy_ui_info_supported(con)) {
         return -1;
@@ -1401,6 +1410,10 @@ QEMUCursor *qemu_console_get_cursor(QemuConsole *con)
     if (con == NULL) {
         con = active_console;
     }
+    if (con == NULL) {
+        return NULL;
+    }
+
     return QEMU_IS_GRAPHIC_CONSOLE(con) ? QEMU_GRAPHIC_CONSOLE(con)->cursor : NULL;
 }
 
@@ -1414,6 +1427,10 @@ bool qemu_console_is_graphic(QemuConsole *con)
     if (con == NULL) {
         con = active_console;
     }
+    if (con == NULL) {
+        return false;
+    }
+
     return con && QEMU_IS_GRAPHIC_CONSOLE(con);
 }
 
@@ -1422,6 +1439,10 @@ bool qemu_console_is_fixedsize(QemuConsole *con)
     if (con == NULL) {
         con = active_console;
     }
+    if (con == NULL) {
+        return false;
+    }
+
     return con && (QEMU_IS_GRAPHIC_CONSOLE(con) || QEMU_IS_FIXED_TEXT_CONSOLE(con));
 }
 
@@ -1493,6 +1514,10 @@ int qemu_console_get_index(QemuConsole *con)
     if (con == NULL) {
         con = active_console;
     }
+    if (con == NULL) {
+        return -1;
+    }
+
     return con ? con->index : -1;
 }
 
-- 
2.41.0
Re: [PATCH] ui: fix crash when there are no active_console
Posted by Albert Esteve 7 months, 2 weeks ago 
On Mon, Sep 11, 2023 at 4:08 PM <marcandre.lureau@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> 0x0000555555888630 in dpy_ui_info_supported (con=0x0) at
> ../ui/console.c:812
> 812         return con->hw_ops->ui_info != NULL;
> (gdb) bt
> #0  0x0000555555888630 in dpy_ui_info_supported (con=0x0) at
> ../ui/console.c:812
> #1  0x00005555558a44b1 in protocol_client_msg (vs=0x5555578c76c0,
> data=0x5555581e93f0 <incomplete sequence \373>, len=24) at ../ui/vnc.c:2585
> #2  0x00005555558a19ac in vnc_client_read (vs=0x5555578c76c0) at
> ../ui/vnc.c:1607
> #3  0x00005555558a1ac2 in vnc_client_io (ioc=0x5555581eb0e0,
> condition=G_IO_IN, opaque=0x5555578c76c0) at ../ui/vnc.c:1635
>
> Fixes:
> https://issues.redhat.com/browse/RHEL-2600
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/console.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/ui/console.c b/ui/console.c
> index 90ae4be602..0f31ecece6 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -808,6 +808,9 @@ bool dpy_ui_info_supported(QemuConsole *con)
>      if (con == NULL) {
>          con = active_console;
>      }
> +    if (con == NULL) {
> +        return false;
> +    }
>
>      return con->hw_ops->ui_info != NULL;
>  }
> @@ -817,6 +820,9 @@ const QemuUIInfo *dpy_get_ui_info(const QemuConsole
> *con)
>      if (con == NULL) {
>          con = active_console;
>      }
> +    if (con == NULL) {
> +        return NULL;
> +    }
>
>      return &con->ui_info;
>  }
> @@ -826,6 +832,9 @@ int dpy_set_ui_info(QemuConsole *con, QemuUIInfo
> *info, bool delay)
>      if (con == NULL) {
>          con = active_console;
>      }
> +    if (con == NULL) {
> +        return -1;
> +    }
>
>      if (!dpy_ui_info_supported(con)) {
>          return -1;
> @@ -1401,6 +1410,10 @@ QEMUCursor *qemu_console_get_cursor(QemuConsole
> *con)
>      if (con == NULL) {
>          con = active_console;
>      }
> +    if (con == NULL) {
> +        return NULL;
> +    }
> +
>      return QEMU_IS_GRAPHIC_CONSOLE(con) ?
> QEMU_GRAPHIC_CONSOLE(con)->cursor : NULL;
>  }
>
> @@ -1414,6 +1427,10 @@ bool qemu_console_is_graphic(QemuConsole *con)
>      if (con == NULL) {
>          con = active_console;
>      }
> +    if (con == NULL) {
> +        return false;
> +    }
> +
>      return con && QEMU_IS_GRAPHIC_CONSOLE(con);
>  }
>
> @@ -1422,6 +1439,10 @@ bool qemu_console_is_fixedsize(QemuConsole *con)
>      if (con == NULL) {
>          con = active_console;
>      }
> +    if (con == NULL) {
> +        return false;
> +    }
> +
>

The "con" initialization condition is already checked in the line below
(now unnecessarily).
If the if block is preferred for consistency with other functions, we could
remove the con check from
the condition below:
```
return QEMU_IS_GRAPHIC_CONSOLE(con) || QEMU_IS_FIXED_TEXT_CONSOLE(con);
```


>      return con && (QEMU_IS_GRAPHIC_CONSOLE(con) ||
> QEMU_IS_FIXED_TEXT_CONSOLE(con));
>  }
>
> @@ -1493,6 +1514,10 @@ int qemu_console_get_index(QemuConsole *con)
>      if (con == NULL) {
>          con = active_console;
>      }
> +    if (con == NULL) {
> +        return -1;
> +    }
> +
>

Same as before, here we could simply "return con->index;"


>      return con ? con->index : -1;

 }
>
> --
> 2.41.0
>
>
>
Re: [PATCH] ui: fix crash when there are no active_console
Posted by Marc-André Lureau 7 months, 2 weeks ago 
Hi

On Mon, Sep 11, 2023 at 6:44 PM Albert Esteve <aesteve@redhat.com> wrote:
>
>
>
> On Mon, Sep 11, 2023 at 4:08 PM <marcandre.lureau@redhat.com> wrote:
>>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
>> 0x0000555555888630 in dpy_ui_info_supported (con=0x0) at ../ui/console.c:812
>> 812         return con->hw_ops->ui_info != NULL;
>> (gdb) bt
>> #0  0x0000555555888630 in dpy_ui_info_supported (con=0x0) at ../ui/console.c:812
>> #1  0x00005555558a44b1 in protocol_client_msg (vs=0x5555578c76c0, data=0x5555581e93f0 <incomplete sequence \373>, len=24) at ../ui/vnc.c:2585
>> #2  0x00005555558a19ac in vnc_client_read (vs=0x5555578c76c0) at ../ui/vnc.c:1607
>> #3  0x00005555558a1ac2 in vnc_client_io (ioc=0x5555581eb0e0, condition=G_IO_IN, opaque=0x5555578c76c0) at ../ui/vnc.c:1635
>>
>> Fixes:
>> https://issues.redhat.com/browse/RHEL-2600
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  ui/console.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/ui/console.c b/ui/console.c
>> index 90ae4be602..0f31ecece6 100644
>> --- a/ui/console.c
>> +++ b/ui/console.c
>> @@ -808,6 +808,9 @@ bool dpy_ui_info_supported(QemuConsole *con)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return false;
>> +    }
>>
>>      return con->hw_ops->ui_info != NULL;
>>  }
>> @@ -817,6 +820,9 @@ const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return NULL;
>> +    }
>>
>>      return &con->ui_info;
>>  }
>> @@ -826,6 +832,9 @@ int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info, bool delay)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return -1;
>> +    }
>>
>>      if (!dpy_ui_info_supported(con)) {
>>          return -1;
>> @@ -1401,6 +1410,10 @@ QEMUCursor *qemu_console_get_cursor(QemuConsole *con)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return NULL;
>> +    }
>> +
>>      return QEMU_IS_GRAPHIC_CONSOLE(con) ? QEMU_GRAPHIC_CONSOLE(con)->cursor : NULL;
>>  }
>>
>> @@ -1414,6 +1427,10 @@ bool qemu_console_is_graphic(QemuConsole *con)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return false;
>> +    }
>> +
>>      return con && QEMU_IS_GRAPHIC_CONSOLE(con);
>>  }
>>
>> @@ -1422,6 +1439,10 @@ bool qemu_console_is_fixedsize(QemuConsole *con)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return false;
>> +    }
>> +
>
>
> The "con" initialization condition is already checked in the line below (now unnecessarily).
> If the if block is preferred for consistency with other functions, we could remove the con check from
> the condition below:
> ```
> return QEMU_IS_GRAPHIC_CONSOLE(con) || QEMU_IS_FIXED_TEXT_CONSOLE(con);
> ```
>
>>
>>      return con && (QEMU_IS_GRAPHIC_CONSOLE(con) || QEMU_IS_FIXED_TEXT_CONSOLE(con));
>>  }
>>
>> @@ -1493,6 +1514,10 @@ int qemu_console_get_index(QemuConsole *con)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return -1;
>> +    }
>> +
>
>
> Same as before, here we could simply "return con->index;"
>
>>
>>      return con ? con->index : -1;
>>
>>  }
>>

You are right, the added checks are mostly redundant. I'll make the
patch touch only dpy_ui_info_supported(). And another patch for the
related precondition when calling dpy_get_ui_info().


-- 
Marc-André Lureau
Re: [PATCH] ui: fix crash when there are no active_console
Posted by Albert Esteve 7 months, 2 weeks ago 
On Mon, Sep 11, 2023 at 4:42 PM Albert Esteve <aesteve@redhat.com> wrote:

>
>
> On Mon, Sep 11, 2023 at 4:08 PM <marcandre.lureau@redhat.com> wrote:
>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
>> 0x0000555555888630 in dpy_ui_info_supported (con=0x0) at
>> ../ui/console.c:812
>> 812         return con->hw_ops->ui_info != NULL;
>> (gdb) bt
>> #0  0x0000555555888630 in dpy_ui_info_supported (con=0x0) at
>> ../ui/console.c:812
>> #1  0x00005555558a44b1 in protocol_client_msg (vs=0x5555578c76c0,
>> data=0x5555581e93f0 <incomplete sequence \373>, len=24) at ../ui/vnc.c:2585
>> #2  0x00005555558a19ac in vnc_client_read (vs=0x5555578c76c0) at
>> ../ui/vnc.c:1607
>> #3  0x00005555558a1ac2 in vnc_client_io (ioc=0x5555581eb0e0,
>> condition=G_IO_IN, opaque=0x5555578c76c0) at ../ui/vnc.c:1635
>>
>> Fixes:
>> https://issues.redhat.com/browse/RHEL-2600
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  ui/console.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/ui/console.c b/ui/console.c
>> index 90ae4be602..0f31ecece6 100644
>> --- a/ui/console.c
>> +++ b/ui/console.c
>> @@ -808,6 +808,9 @@ bool dpy_ui_info_supported(QemuConsole *con)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return false;
>> +    }
>>
>>      return con->hw_ops->ui_info != NULL;
>>  }
>> @@ -817,6 +820,9 @@ const QemuUIInfo *dpy_get_ui_info(const QemuConsole
>> *con)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return NULL;
>> +    }
>>
>>      return &con->ui_info;
>>  }
>> @@ -826,6 +832,9 @@ int dpy_set_ui_info(QemuConsole *con, QemuUIInfo
>> *info, bool delay)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return -1;
>> +    }
>>
>>      if (!dpy_ui_info_supported(con)) {
>>          return -1;
>> @@ -1401,6 +1410,10 @@ QEMUCursor *qemu_console_get_cursor(QemuConsole
>> *con)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return NULL;
>> +    }
>> +
>>      return QEMU_IS_GRAPHIC_CONSOLE(con) ?
>> QEMU_GRAPHIC_CONSOLE(con)->cursor : NULL;
>>  }
>>
>> @@ -1414,6 +1427,10 @@ bool qemu_console_is_graphic(QemuConsole *con)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return false;
>> +    }
>> +
>>
>
I had miss this one before:
```
return QEMU_IS_GRAPHIC_CONSOLE(con);
```

Regards,
Albert


>      return con && QEMU_IS_GRAPHIC_CONSOLE(con);
>>  }
>>
>> @@ -1422,6 +1439,10 @@ bool qemu_console_is_fixedsize(QemuConsole *con)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return false;
>> +    }
>> +
>>
>
> The "con" initialization condition is already checked in the line below
> (now unnecessarily).
> If the if block is preferred for consistency with other functions, we
> could remove the con check from
> the condition below:
> ```
> return QEMU_IS_GRAPHIC_CONSOLE(con) || QEMU_IS_FIXED_TEXT_CONSOLE(con);
> ```
>
>
>>      return con && (QEMU_IS_GRAPHIC_CONSOLE(con) ||
>> QEMU_IS_FIXED_TEXT_CONSOLE(con));
>>  }
>>
>> @@ -1493,6 +1514,10 @@ int qemu_console_get_index(QemuConsole *con)
>>      if (con == NULL) {
>>          con = active_console;
>>      }
>> +    if (con == NULL) {
>> +        return -1;
>> +    }
>> +
>>
>
> Same as before, here we could simply "return con->index;"
>
>
>>      return con ? con->index : -1;
>
>  }
>>
>> --
>> 2.41.0
>>
>>
>>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.