| Message ID | 1481732391-882-1-git-send-email-vlad.lungu@windriver.com |
|---|---|
| State | New |
| Headers | show |
----- Original Message ----- > From: "Eduardo Habkost" <ehabkost@redhat.com> > To: "Paolo Bonzini" <pbonzini@redhat.com> > Cc: "Vlad Lungu" <vlad.lungu@windriver.com>, qemu-devel@nongnu.org, rth@twiddle.net > Sent: Wednesday, December 14, 2016 6:51:44 PM > Subject: Re: [PATCH] multiboot: copy the cmdline verbatim > > 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. For filenames containing commas you're right, but... > 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. ... there is one case of comma escaping that wasn't broken: $ qemu-system-x86_64 -kernel foo -initrd '/tmp/one arg,,with,,commas,/tmp/another arg,,with,,commas' And presumably this is what Vlad was trying to do. Paolo
On Wed, Dec 14, 2016 at 04:38:07PM -0500, Paolo Bonzini wrote: > > > ----- Original Message ----- > > From: "Eduardo Habkost" <ehabkost@redhat.com> > > To: "Paolo Bonzini" <pbonzini@redhat.com> > > Cc: "Vlad Lungu" <vlad.lungu@windriver.com>, qemu-devel@nongnu.org, rth@twiddle.net > > Sent: Wednesday, December 14, 2016 6:51:44 PM > > Subject: Re: [PATCH] multiboot: copy the cmdline verbatim > > > > 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. > > For filenames containing commas you're right, but... > > > 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. > > ... there is one case of comma escaping that wasn't broken: > > $ qemu-system-x86_64 -kernel foo -initrd '/tmp/one arg,,with,,commas,/tmp/another arg,,with,,commas' > Oh, I didn't notice the whitespace-based split for initrd arguments. This is messier than I thought. Maybe the simplest solution is to inline mb_add_cmdline() at both callers, and change the kcmdline one to use memcpy(). > And presumably this is what Vlad was trying to do. I believe he was trying to fix -append, not -initrd. The patch fixes -append, but breaks comma-escaping on -initrd.
> > > 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. > > > > ... there is one case of comma escaping that wasn't broken: > > > > $ qemu-system-x86_64 -kernel foo -initrd '/tmp/one > > arg,,with,,commas,/tmp/another arg,,with,,commas' > > > > Oh, I didn't notice the whitespace-based split for initrd > arguments. > > This is messier than I thought. Maybe the simplest solution is to > inline mb_add_cmdline() at both callers, and change the kcmdline > one to use memcpy(). Yes, I agree. Paolo
On 12/15/2016 12:32 AM, Paolo Bonzini wrote: >>>> 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. >>> ... there is one case of comma escaping that wasn't broken: >>> >>> $ qemu-system-x86_64 -kernel foo -initrd '/tmp/one >>> arg,,with,,commas,/tmp/another arg,,with,,commas' >>> >> Oh, I didn't notice the whitespace-based split for initrd >> arguments. So that's how it works :-) >> This is messier than I thought. Maybe the simplest solution is to >> inline mb_add_cmdline() at both callers, and change the kcmdline >> one to use memcpy(). > OK, I have a new version that does with memcpy for the cmdline, with get_opt_value() for the modules and also unescapes the filename for get_image_size()/load_image() . You still can't have spaces in filenames, so maybe a new scheme should be devised for this. Regards, Vlad
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; }
get_opt_value() truncates the value at the first comma. Use memcpy() instead. Signed-off-by: Vlad Lungu <vlad.lungu@windriver.com> --- hw/i386/multiboot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)