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

[Paolo Bonzini <pbonzini@redhat.com>]

----- 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