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

[Eduardo Habkost]

On Wed, Dec 14, 2016 at 04:38:07PM -0500, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
> > From: "Eduardo Habkost" <address@hidden>
> > To: "Paolo Bonzini" <address@hidden>
> > Cc: "Vlad Lungu" <address@hidden>, address@hidden, address@hidden
> > 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.

-- 
Eduardo