| Message ID | 1542272663-6619-1-git-send-email-liq3ea@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | keymaps: detect recursive keyboard layout file | expand |
On Thu, Nov 15, 2018 at 01:04:23AM -0800, Li Qiang wrote: > When the parse_keyboard_layout() find a "include " line > in the keyboard layout file, it will call parse_keyboard_layout() > to perform a recursive parse. If the keyboard layout is malformed > by adding a line include itself, this can cause an infinite parse. > Thus cause qemu a segv. This patch avoid this. Hmm. Most keymap files are generated by qemu-keymap these days and do not use includes in the first place. Three are left over: nl-be, sl, sv. Looking at them it seems like nl-be is not functional, it just includes "common" and doesn't define any mappings. For sl and sv I have no clue what keymap they represent. So I'd suggest to just remove support for "include", drop the nl-be map, fix the sl and sv maps that they don't need "include" any more. Either just replace the "include" statement with the content of the "common" file. Or, if someone has a clue what keyboard layout these keymaps are for, add rules to the Makefile and update them using then using qemu-keymap. cheers, Gerd
Gerd Hoffmann <kraxel@redhat.com> 于2018年11月15日周四 下午6:15写道： > On Thu, Nov 15, 2018 at 01:04:23AM -0800, Li Qiang wrote: > > When the parse_keyboard_layout() find a "include " line > > in the keyboard layout file, it will call parse_keyboard_layout() > > to perform a recursive parse. If the keyboard layout is malformed > > by adding a line include itself, this can cause an infinite parse. > > Thus cause qemu a segv. This patch avoid this. > > Hmm. Most keymap files are generated by qemu-keymap these days and do > not use includes in the first place. Three are left over: nl-be, sl, > sv. > > Looking at them it seems like nl-be is not functional, it just includes > "common" and doesn't define any mappings. For sl and sv I have no clue > what keymap they represent. > > So I'd suggest to just remove support for "include", drop the nl-be map, > fix the sl and sv maps that they don't need "include" any more. Either > just replace the "include" statement with the content of the "common" > file. Or, if someone has a clue what keyboard layout these keymaps are > for, add rules to the Makefile and update them using then using > qemu-keymap. > Hello Gerd, Thanks for your advice. Here my consideration is for the '-k' option can specific any file. Though it seems too radical, I consider every input can't be trusted. Anyway, it's ok you ingore this patch. Thanks, Li Qiang > > cheers, > Gerd > >
Li Qiang <liq3ea@gmail.com> writes: > When the parse_keyboard_layout() find a "include " line > in the keyboard layout file, it will call parse_keyboard_layout() > to perform a recursive parse. If the keyboard layout is malformed > by adding a line include itself, this can cause an infinite parse. > Thus cause qemu a segv. This patch avoid this. > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > --- > ui/keymaps.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/ui/keymaps.c b/ui/keymaps.c > index 085889b555..564893a9f3 100644 > --- a/ui/keymaps.c > +++ b/ui/keymaps.c > @@ -38,6 +38,8 @@ struct kbd_layout_t { > GHashTable *hash; > }; > > +GList *keyboard_files; > + > static int get_keysym(const name2keysym_t *table, > const char *name) > { > @@ -80,6 +82,11 @@ static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k) > trace_keymap_add(keysym, keycode, line); > } > > +static gint compare_string(gconstpointer a, gconstpointer b) > +{ > + return g_strcmp0(a, b); > +} > + > static int parse_keyboard_layout(kbd_layout_t *k, > const name2keysym_t *table, > const char *language, Error **errp) > @@ -94,12 +101,18 @@ static int parse_keyboard_layout(kbd_layout_t *k, > filename = qemu_find_file(QEMU_FILE_TYPE_KEYMAP, language); > trace_keymap_parse(filename); > f = filename ? fopen(filename, "r") : NULL; > - g_free(filename); > if (!f) { > + g_free(filename); > error_setg(errp, "could not read keymap file: '%s'", language); > return -1; > } > > + if (g_list_find_custom(keyboard_files, filename, compare_string)) { > + error_setg(errp, "find recursive keyboard layout: %s'", filename); Suggest something like "Inclusion loop for %s". > + g_free(filename); > + return -1; > + } > + keyboard_files = g_list_append(keyboard_files, filename); > for(;;) { > if (fgets(line, 1024, f) == NULL) { > break; > @@ -168,6 +181,8 @@ static int parse_keyboard_layout(kbd_layout_t *k, > ret = 0; > out: > fclose(f); > + keyboard_files = g_list_remove(keyboard_files, filename); > + g_free(filename); > return ret; > } There's no real need to make @keyboard_files global. I'd make it local to init_keyboard_layout(), and pass it as argument to parse_keyboard_layout(). Matter of taste. Gerd is the maintainer, not me.
Markus Armbruster <armbru@redhat.com> 于2018年11月15日周四 下午9:29写道： > Li Qiang <liq3ea@gmail.com> writes: > > > When the parse_keyboard_layout() find a "include " line > > in the keyboard layout file, it will call parse_keyboard_layout() > > to perform a recursive parse. If the keyboard layout is malformed > > by adding a line include itself, this can cause an infinite parse. > > Thus cause qemu a segv. This patch avoid this. > > > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > --- > > ui/keymaps.c | 17 ++++++++++++++++- > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/ui/keymaps.c b/ui/keymaps.c > > index 085889b555..564893a9f3 100644 > > --- a/ui/keymaps.c > > +++ b/ui/keymaps.c > > @@ -38,6 +38,8 @@ struct kbd_layout_t { > > GHashTable *hash; > > }; > > > > +GList *keyboard_files; > > + > > static int get_keysym(const name2keysym_t *table, > > const char *name) > > { > > @@ -80,6 +82,11 @@ static void add_keysym(char *line, int keysym, int > keycode, kbd_layout_t *k) > > trace_keymap_add(keysym, keycode, line); > > } > > > > +static gint compare_string(gconstpointer a, gconstpointer b) > > +{ > > + return g_strcmp0(a, b); > > +} > > + > > static int parse_keyboard_layout(kbd_layout_t *k, > > const name2keysym_t *table, > > const char *language, Error **errp) > > @@ -94,12 +101,18 @@ static int parse_keyboard_layout(kbd_layout_t *k, > > filename = qemu_find_file(QEMU_FILE_TYPE_KEYMAP, language); > > trace_keymap_parse(filename); > > f = filename ? fopen(filename, "r") : NULL; > > - g_free(filename); > > if (!f) { > > + g_free(filename); > > error_setg(errp, "could not read keymap file: '%s'", language); > > return -1; > > } > > > > + if (g_list_find_custom(keyboard_files, filename, compare_string)) { > > + error_setg(errp, "find recursive keyboard layout: %s'", > filename); > > Suggest something like "Inclusion loop for %s". > > > + g_free(filename); > > + return -1; > > + } > > + keyboard_files = g_list_append(keyboard_files, filename); > > for(;;) { > > if (fgets(line, 1024, f) == NULL) { > > break; > > @@ -168,6 +181,8 @@ static int parse_keyboard_layout(kbd_layout_t *k, > > ret = 0; > > out: > > fclose(f); > > + keyboard_files = g_list_remove(keyboard_files, filename); > > + g_free(filename); > > return ret; > > } > > There's no real need to make @keyboard_files global. I'd make it local > to init_keyboard_layout(), and pass it as argument to > parse_keyboard_layout(). Thanks Markus, Finally, Gerd decide remove the support of "include" in keymaps. Thanks, Li Qiang > Matter of taste. Gerd is the maintainer, not > me. >
diff --git a/ui/keymaps.c b/ui/keymaps.c index 085889b555..564893a9f3 100644 --- a/ui/keymaps.c +++ b/ui/keymaps.c @@ -38,6 +38,8 @@ struct kbd_layout_t { GHashTable *hash; }; +GList *keyboard_files; + static int get_keysym(const name2keysym_t *table, const char *name) { @@ -80,6 +82,11 @@ static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k) trace_keymap_add(keysym, keycode, line); } +static gint compare_string(gconstpointer a, gconstpointer b) +{ + return g_strcmp0(a, b); +} + static int parse_keyboard_layout(kbd_layout_t *k, const name2keysym_t *table, const char *language, Error **errp) @@ -94,12 +101,18 @@ static int parse_keyboard_layout(kbd_layout_t *k, filename = qemu_find_file(QEMU_FILE_TYPE_KEYMAP, language); trace_keymap_parse(filename); f = filename ? fopen(filename, "r") : NULL; - g_free(filename); if (!f) { + g_free(filename); error_setg(errp, "could not read keymap file: '%s'", language); return -1; } + if (g_list_find_custom(keyboard_files, filename, compare_string)) { + error_setg(errp, "find recursive keyboard layout: %s'", filename); + g_free(filename); + return -1; + } + keyboard_files = g_list_append(keyboard_files, filename); for(;;) { if (fgets(line, 1024, f) == NULL) { break; @@ -168,6 +181,8 @@ static int parse_keyboard_layout(kbd_layout_t *k, ret = 0; out: fclose(f); + keyboard_files = g_list_remove(keyboard_files, filename); + g_free(filename); return ret; }
When the parse_keyboard_layout() find a "include " line in the keyboard layout file, it will call parse_keyboard_layout() to perform a recursive parse. If the keyboard layout is malformed by adding a line include itself, this can cause an infinite parse. Thus cause qemu a segv. This patch avoid this. Signed-off-by: Li Qiang <liq3ea@gmail.com> --- ui/keymaps.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)