parse_keyboard_layout() passes a possibly null @filename to
trace_keymap_parse().  Trace backend log then formats it with %s,
which crashes on some systems.

Fix by moving the null check before the trace_keymap_parse().

While there, improve the error messages a bit.

Fixes: d3b787fa7dde (keymaps: add tracing)
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 ui/keymaps.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/ui/keymaps.c b/ui/keymaps.c
index 6ceaa97085..2359dbfe7e 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -86,19 +86,25 @@ static int parse_keyboard_layout(kbd_layout_t *k,
                                  const name2keysym_t *table,
                                  const char *language, Error **errp)
 {
+    g_autofree char *filename = NULL;
     int ret;
     FILE *f;
-    char * filename;
     char line[1024];
     char keyname[64];
     int len;
 
     filename = qemu_find_file(QEMU_FILE_TYPE_KEYMAP, language);
+    if (!filename) {
+        error_setg(errp, "could not find keymap file for language '%s'",
+                   language);
+        return -1;
+    }
+
     trace_keymap_parse(filename);
-    f = filename ? fopen(filename, "r") : NULL;
-    g_free(filename);
+
+    f = fopen(filename, "r");
     if (!f) {
-        error_setg(errp, "could not read keymap file: '%s'", language);
+        error_setg_file_open(errp, errno, filename);
         return -1;
     }
 
-- 
2.49.0

Markus Armbruster <armbru@redhat.com> writes:

> parse_keyboard_layout() passes a possibly null @filename to
> trace_keymap_parse().  Trace backend log then formats it with %s,
> which crashes on some systems.
>
> Fix by moving the null check before the trace_keymap_parse().
>
> While there, improve the error messages a bit.
>
> Fixes: d3b787fa7dde (keymaps: add tracing)
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Queued.  Thanks for the review!

On 23/7/25 15:15, Markus Armbruster wrote:
> parse_keyboard_layout() passes a possibly null @filename to
> trace_keymap_parse().  Trace backend log then formats it with %s,
> which crashes on some systems.
> 
> Fix by moving the null check before the trace_keymap_parse().
> 
> While there, improve the error messages a bit.
> 
> Fixes: d3b787fa7dde (keymaps: add tracing)
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   ui/keymaps.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

On Wed, Jul 23, 2025 at 5:15 PM Markus Armbruster <armbru@redhat.com> wrote:

> parse_keyboard_layout() passes a possibly null @filename to
> trace_keymap_parse().  Trace backend log then formats it with %s,
> which crashes on some systems.
>
> Fix by moving the null check before the trace_keymap_parse().
>
> While there, improve the error messages a bit.
>
> Fixes: d3b787fa7dde (keymaps: add tracing)
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>

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


> ---
>  ui/keymaps.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/ui/keymaps.c b/ui/keymaps.c
> index 6ceaa97085..2359dbfe7e 100644
> --- a/ui/keymaps.c
> +++ b/ui/keymaps.c
> @@ -86,19 +86,25 @@ static int parse_keyboard_layout(kbd_layout_t *k,
>                                   const name2keysym_t *table,
>                                   const char *language, Error **errp)
>  {
> +    g_autofree char *filename = NULL;
>      int ret;
>      FILE *f;
> -    char * filename;
>      char line[1024];
>      char keyname[64];
>      int len;
>
>      filename = qemu_find_file(QEMU_FILE_TYPE_KEYMAP, language);
> +    if (!filename) {
> +        error_setg(errp, "could not find keymap file for language '%s'",
> +                   language);
> +        return -1;
> +    }
> +
>      trace_keymap_parse(filename);
> -    f = filename ? fopen(filename, "r") : NULL;
> -    g_free(filename);
> +
> +    f = fopen(filename, "r");
>      if (!f) {
> -        error_setg(errp, "could not read keymap file: '%s'", language);
> +        error_setg_file_open(errp, errno, filename);
>          return -1;
>      }
>
> --
> 2.49.0
>
>