[PATCH v2 1/2] ui: fix crash when there are no active_console

[PATCH v2 1/2] ui: fix crash when there are no active_console

[marcandre.lureau]

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ui/console.c b/ui/console.c
index 90ae4be602..0fbec4d0bd 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;
 }
-- 
2.41.0

[PATCH v2 2/2] ui: add precondition for dpy_get_ui_info()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Ensure that it only get called when dpy_ui_info_supported(). The
function should always return a result. There should be a non-null
console or active_console.

Modify the argument to be const as well.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/console.h | 2 +-
 ui/console.c         | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index 031e5d5194..08c0f0dc70 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -329,7 +329,7 @@ void update_displaychangelistener(DisplayChangeListener *dcl,
                                   uint64_t interval);
 void unregister_displaychangelistener(DisplayChangeListener *dcl);
 
-bool dpy_ui_info_supported(QemuConsole *con);
+bool dpy_ui_info_supported(const QemuConsole *con);
 const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con);
 int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info, bool delay);
 
diff --git a/ui/console.c b/ui/console.c
index 0fbec4d0bd..1c710a6d5e 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -803,7 +803,7 @@ static void dpy_set_ui_info_timer(void *opaque)
     con->hw_ops->ui_info(con->hw, head, &con->ui_info);
 }
 
-bool dpy_ui_info_supported(QemuConsole *con)
+bool dpy_ui_info_supported(const QemuConsole *con)
 {
     if (con == NULL) {
         con = active_console;
@@ -817,6 +817,8 @@ bool dpy_ui_info_supported(QemuConsole *con)
 
 const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con)
 {
+    assert(dpy_ui_info_supported(con));
+
     if (con == NULL) {
         con = active_console;
     }
-- 
2.41.0

Re: [PATCH v2 1/2] ui: fix crash when there are no active_console

[Albert Esteve]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

On Tue, Sep 12, 2023 at 8:28 AM <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 | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/ui/console.c b/ui/console.c
> index 90ae4be602..0fbec4d0bd 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;
>  }
> --
> 2.41.0
>
> Reviewed-by: Albert Esteve <aesteve@redhat.com>

[-- Attachment #2: Type: text/html, Size: 2185 bytes --]

Re: [PATCH v2 2/2] ui: add precondition for dpy_get_ui_info()

[Albert Esteve]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

On Tue, Sep 12, 2023 at 8:28 AM <marcandre.lureau@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Ensure that it only get called when dpy_ui_info_supported(). The
> function should always return a result. There should be a non-null
> console or active_console.
>
> Modify the argument to be const as well.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/ui/console.h | 2 +-
>  ui/console.c         | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 031e5d5194..08c0f0dc70 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -329,7 +329,7 @@ void
> update_displaychangelistener(DisplayChangeListener *dcl,
>                                    uint64_t interval);
>  void unregister_displaychangelistener(DisplayChangeListener *dcl);
>
> -bool dpy_ui_info_supported(QemuConsole *con);
> +bool dpy_ui_info_supported(const QemuConsole *con);
>  const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con);
>  int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info, bool delay);
>
> diff --git a/ui/console.c b/ui/console.c
> index 0fbec4d0bd..1c710a6d5e 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -803,7 +803,7 @@ static void dpy_set_ui_info_timer(void *opaque)
>      con->hw_ops->ui_info(con->hw, head, &con->ui_info);
>  }
>
> -bool dpy_ui_info_supported(QemuConsole *con)
> +bool dpy_ui_info_supported(const QemuConsole *con)
>  {
>      if (con == NULL) {
>          con = active_console;
> @@ -817,6 +817,8 @@ bool dpy_ui_info_supported(QemuConsole *con)
>
>  const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con)
>  {
> +    assert(dpy_ui_info_supported(con));
> +
>

I wonder if it would be better to avoid the assertion and return NULL
if not supported, and let the caller handle (logging an error for example).

But there are many other similar assertions in this file, so it is probably
good as it is...

Reviewed-by: Albert Esteve <aesteve@redhat.com>


>      if (con == NULL) {
>          con = active_console;
>      }
> --
> 2.41.0
>
>

[-- Attachment #2: Type: text/html, Size: 3326 bytes --]