diff mbox series

ui: fix crash when there are no active_console

Message ID 20230911140638.1458156-1-marcandre.lureau@redhat.com
State New
Headers show
Series ui: fix crash when there are no active_console | expand

Commit Message

Marc-André Lureau Sept. 11, 2023, 2:06 p.m. UTC 
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(+)

Comments

Albert Esteve Sept. 11, 2023, 2:42 p.m. UTC | #1 
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
>
>
>
Albert Esteve Sept. 11, 2023, 3:02 p.m. UTC | #2 
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
>>
>>
>>
Marc-André Lureau Sept. 12, 2023, 6:27 a.m. UTC | #3 
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().
diff mbox series

Patch

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