| Message ID | 1457420446-25276-9-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 8 March 2016 at 14:00, Peter Xu <peterx@redhat.com> wrote: > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > CC: Paolo Bonzini <pbonzini@redhat.com> > CC: Richard Henderson <rth@twiddle.net> > CC: Eduardo Habkost <ehabkost@redhat.com> > CC: "Michael S. Tsirkin" <mst@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > hw/i386/multiboot.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c > index 9e164e6..0eecb9a 100644 > --- a/hw/i386/multiboot.c > +++ b/hw/i386/multiboot.c > @@ -159,6 +159,12 @@ int load_multiboot(FWCfgState *fw_cfg, > uint8_t *mb_bootinfo_data; > uint32_t cmdline_len; > > +#define __KERN_FNAME_LEN (1024) > +#define __KERN_CMDLINE_LEN (4096) Where do these choices of length restriction come from? > + > + assert(strlen(kernel_filename) + 1 >= __KERN_FNAME_LEN); > + assert(strlen(kernel_cmdline) + 1 >= __KERN_CMDLINE_LEN); > + > /* Ok, let's see if it is a multiboot image. > The header is 12x32bit long, so the latest entry may be 8192 - 48. */ > for (i = 0; i < (8192 - 48); i += 4) { > @@ -324,7 +330,7 @@ int load_multiboot(FWCfgState *fw_cfg, > } > > /* Commandline support */ > - char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2]; > + char kcmdline[__KERN_FNAME_LEN + __KERN_CMDLINE_LEN]; > snprintf(kcmdline, sizeof(kcmdline), "%s %s", > kernel_filename, kernel_cmdline); Why can't we just dynamically allocate the command line (ie g_strdup_printf() it) ? > stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline)); > @@ -370,4 +376,6 @@ int load_multiboot(FWCfgState *fw_cfg, > nb_option_roms++; > > return 1; /* yes, we are multiboot */ > +#undef __KERN_FNAME_LEN > +#undef __KERN_CMDLINE_LEN thanks -- PMM
On 08/03/2016 08:00, Peter Xu wrote: > @@ -159,6 +159,12 @@ int load_multiboot(FWCfgState *fw_cfg, > uint8_t *mb_bootinfo_data; > uint32_t cmdline_len; > > +#define __KERN_FNAME_LEN (1024) > +#define __KERN_CMDLINE_LEN (4096) > + > + assert(strlen(kernel_filename) + 1 >= __KERN_FNAME_LEN); > + assert(strlen(kernel_cmdline) + 1 >= __KERN_CMDLINE_LEN); > + > /* Ok, let's see if it is a multiboot image. > The header is 12x32bit long, so the latest entry may be 8192 - 48. */ > for (i = 0; i < (8192 - 48); i += 4) { > @@ -324,7 +330,7 @@ int load_multiboot(FWCfgState *fw_cfg, > } > > /* Commandline support */ > - char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2]; > + char kcmdline[__KERN_FNAME_LEN + __KERN_CMDLINE_LEN]; > snprintf(kcmdline, sizeof(kcmdline), "%s %s", > kernel_filename, kernel_cmdline); > stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline)); > @@ -370,4 +376,6 @@ int load_multiboot(FWCfgState *fw_cfg, > nb_option_roms++; > > return 1; /* yes, we are multiboot */ > +#undef __KERN_FNAME_LEN > +#undef __KERN_CMDLINE_LEN Just put it in the heap using g_strdup_printf. Paolo
On Tue, Mar 08, 2016 at 01:29:21PM +0100, Paolo Bonzini wrote: > > > On 08/03/2016 08:00, Peter Xu wrote: > > @@ -159,6 +159,12 @@ int load_multiboot(FWCfgState *fw_cfg, > > uint8_t *mb_bootinfo_data; > > uint32_t cmdline_len; > > > > +#define __KERN_FNAME_LEN (1024) > > +#define __KERN_CMDLINE_LEN (4096) > > + > > + assert(strlen(kernel_filename) + 1 >= __KERN_FNAME_LEN); > > + assert(strlen(kernel_cmdline) + 1 >= __KERN_CMDLINE_LEN); > > + > > /* Ok, let's see if it is a multiboot image. > > The header is 12x32bit long, so the latest entry may be 8192 - 48. */ > > for (i = 0; i < (8192 - 48); i += 4) { > > @@ -324,7 +330,7 @@ int load_multiboot(FWCfgState *fw_cfg, > > } > > > > /* Commandline support */ > > - char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2]; > > + char kcmdline[__KERN_FNAME_LEN + __KERN_CMDLINE_LEN]; > > snprintf(kcmdline, sizeof(kcmdline), "%s %s", > > kernel_filename, kernel_cmdline); > > stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline)); > > @@ -370,4 +376,6 @@ int load_multiboot(FWCfgState *fw_cfg, > > nb_option_roms++; > > > > return 1; /* yes, we are multiboot */ > > +#undef __KERN_FNAME_LEN > > +#undef __KERN_CMDLINE_LEN > > Just put it in the heap using g_strdup_printf. Will fix and send standalone again. Thanks. Peter
diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c index 9e164e6..0eecb9a 100644 --- a/hw/i386/multiboot.c +++ b/hw/i386/multiboot.c @@ -159,6 +159,12 @@ int load_multiboot(FWCfgState *fw_cfg, uint8_t *mb_bootinfo_data; uint32_t cmdline_len; +#define __KERN_FNAME_LEN (1024) +#define __KERN_CMDLINE_LEN (4096) + + assert(strlen(kernel_filename) + 1 >= __KERN_FNAME_LEN); + assert(strlen(kernel_cmdline) + 1 >= __KERN_CMDLINE_LEN); + /* Ok, let's see if it is a multiboot image. The header is 12x32bit long, so the latest entry may be 8192 - 48. */ for (i = 0; i < (8192 - 48); i += 4) { @@ -324,7 +330,7 @@ int load_multiboot(FWCfgState *fw_cfg, } /* Commandline support */ - char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2]; + char kcmdline[__KERN_FNAME_LEN + __KERN_CMDLINE_LEN]; snprintf(kcmdline, sizeof(kcmdline), "%s %s", kernel_filename, kernel_cmdline); stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline)); @@ -370,4 +376,6 @@ int load_multiboot(FWCfgState *fw_cfg, nb_option_roms++; return 1; /* yes, we are multiboot */ +#undef __KERN_FNAME_LEN +#undef __KERN_CMDLINE_LEN }
Suggested-by: Paolo Bonzini <pbonzini@redhat.com> CC: Paolo Bonzini <pbonzini@redhat.com> CC: Richard Henderson <rth@twiddle.net> CC: Eduardo Habkost <ehabkost@redhat.com> CC: "Michael S. Tsirkin" <mst@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/i386/multiboot.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)