qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] multiboot: copy the cmdline verbatim


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH] multiboot: copy the cmdline verbatim
Date: Wed, 14 Dec 2016 15:51:44 -0200
User-agent: Mutt/1.7.1 (2016-10-04)

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 <address@hidden>
> >>> ---
> >>>  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]