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

[Marc-André Lureau]

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