On Wed, Dec 14, 2016 at 06:20:46PM +0100, Paolo Bonzini wrote: > On 14/12/2016 18:00, Eduardo Habkost wrote: > > On Wed, Dec 14, 2016 at 05:55:07PM +0100, Paolo Bonzini wrote: > >> > >> > >> On 14/12/2016 17:19, Vlad Lungu wrote: > >>> get_opt_value() truncates the value at the first comma. > >>> Use memcpy() instead. > >> > >> Looks good since get_opt_value is already used by the caller. Have you > >> tested this with multiple initrd modules too? > > > > get_opt_value() is used by the caller, but with NULL buf > > argument. This means the caller doesn't handle ",," escaping. > > (See my reply to the previous submission of this patch) > > Hmm, wait. When NULL is passed, ",," escaping is handled correctly in > that next_initrd points to the next lone comma. The lone comma is > replaced with a '\0' by the caller.
It is handled correctly when splitting the string, but not when opening the file. > > So you need to use get_opt_value again in mb_add_cmdline to do the > unescaping, because mb_add_cmdline only receives double commas. > > This was actually my first reaction to the patch, and it was correct. > Then I overthought it. :) > > So the patch is wrong. Except that the caller is already broken when using ",," for a different reason: it calls get_image_size(initrd_filename) and load_image(initrd_filename, ...) directly. So comma-escaping never worked anyway: $ qemu-system-x86_64 -kernel ~/Downloads/gnumach -initrd /tmp/one,,file,/tmp/another,,file Failed to open file '/tmp/one,,file' The right fix for comma-escaping would require calling get_opt_value() with non-NULL buf outside mb_add_cmdline(), because the mb_add_cmdline(&mbs, kcmdline) call do NOT need get_opt_value() to be called. In other words: this fixes the mb_add_cmdline(kcmdline) case, and doesn't break comma escaping on the initrd case (because it was already broken). I don't see a problem with this patch. > > Paolo > > > However this only means the caller is buggy, not > > mb_add_cmdline(). So the change still looks good to me. > > > >> > >> Paolo > >> > >>> Signed-off-by: Vlad Lungu <vlad.lu...@windriver.com> > >>> --- > >>> hw/i386/multiboot.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c > >>> index 387caa6..b4495ad 100644 > >>> --- a/hw/i386/multiboot.c > >>> +++ b/hw/i386/multiboot.c > >>> @@ -109,7 +109,7 @@ static uint32_t mb_add_cmdline(MultibootState *s, > >>> const char *cmdline) > >>> hwaddr p = s->offset_cmdlines; > >>> char *b = (char *)s->mb_buf + p; > >>> > >>> - get_opt_value(b, strlen(cmdline) + 1, cmdline); > >>> + memcpy(b, cmdline, strlen(cmdline) + 1); > >>> s->offset_cmdlines += strlen(b) + 1; > >>> return s->mb_buf_phys + p; > >>> } > >>> > > -- Eduardo
