[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
- [Qemu-devel] [PATCH] multiboot: copy the cmdline verbatim, Vlad Lungu, 2016/12/14
- Re: [Qemu-devel] [PATCH] multiboot: copy the cmdline verbatim, Paolo Bonzini, 2016/12/14
- Re: [Qemu-devel] [PATCH] multiboot: copy the cmdline verbatim, Eduardo Habkost, 2016/12/14
- Re: [Qemu-devel] [PATCH] multiboot: copy the cmdline verbatim, Paolo Bonzini, 2016/12/14
- Re: [Qemu-devel] [PATCH] multiboot: copy the cmdline verbatim,
Eduardo Habkost <=
- Re: [Qemu-devel] [PATCH] multiboot: copy the cmdline verbatim, Paolo Bonzini, 2016/12/14
- Re: [Qemu-devel] [PATCH] multiboot: copy the cmdline verbatim, Eduardo Habkost, 2016/12/14
- Re: [Qemu-devel] [PATCH] multiboot: copy the cmdline verbatim, Paolo Bonzini, 2016/12/14
- Re: [Qemu-devel] [PATCH] multiboot: copy the cmdline verbatim, Vlad Lungu, 2016/12/15