Re: [Qemu-devel] [PATCHv3] multiboot: copy the cmdline verbatim, unescape module strings

[Eduardo Habkost]

On Mon, Dec 19, 2016 at 11:35:34AM +0200, Vlad Lungu wrote:
> On 12/18/2016 10:25 PM, Eduardo Habkost wrote:
> > On Thu, Dec 15, 2016 at 02:32:04PM +0200, Vlad Lungu wrote:
> >> get_opt_value() truncates the value at the first comma
> >> Use memcpy() instead
> >> Unescape the module filename and parameters with get_opt_value()
> >> before calling mb_add_cmdline()
> >>
> >> Signed-off-by: Vlad Lungu <address@hidden>
> >> ---
> >>  hw/i386/multiboot.c | 19 +++++++++----------
> >>  1 file changed, 9 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> >> index 387caa6..6b7b5a9 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;
> >>  }
> >> @@ -287,7 +287,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> >>      mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len;
> >>  
> >>      if (initrd_filename) {
> >> -        char *next_initrd, not_last;
> >> +        char *next_initrd, not_last, tmpbuf[strlen(initrd_filename) + 1];
> >>  
> >>          mbs.offset_mods = mbs.mb_buf_size;
> >>  
> >> @@ -296,25 +296,24 @@ int load_multiboot(FWCfgState *fw_cfg,
> >>              int mb_mod_length;
> >>              uint32_t offs = mbs.mb_buf_size;
> >>  
> >> -            next_initrd = (char *)get_opt_value(NULL, 0, initrd_filename);
> >> +            next_initrd = (char *)get_opt_value(tmpbuf, 
> >> strlen(initrd_filename) + 1, initrd_filename);
> > I would prefer to use sizeof(initrd_filename) like Paolo
> > suggested.
> sizeof(initrd_filename) is 8 (on my machine, x86_64). Maybe sizeof(tmpbuf) 
> would be a better idea :-)

Oops, yes. I meant sizeof(tmpbuf).  :)

-- 
Eduardo