* [Qemu-devel] [PATCHv3] multiboot: copy the cmdline verbatim, unescape module strings
@ 2016-12-15 12:32 Vlad Lungu
2016-12-18 20:25 ` Eduardo Habkost
0 siblings, 1 reply; 4+ messages in thread
From: Vlad Lungu @ 2016-12-15 12:32 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, ehabkost, rth, Vlad Lungu
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 <vlad.lungu@windriver.com>
---
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);
not_last = *next_initrd;
- *next_initrd = '\0';
/* if a space comes after the module filename, treat everything
after that as parameters */
- hwaddr c = mb_add_cmdline(&mbs, initrd_filename);
- if ((next_space = strchr(initrd_filename, ' ')))
+ hwaddr c = mb_add_cmdline(&mbs, tmpbuf);
+ if ((next_space = strchr(tmpbuf, ' ')))
*next_space = '\0';
- mb_debug("multiboot loading module: %s\n", initrd_filename);
- mb_mod_length = get_image_size(initrd_filename);
+ mb_debug("multiboot loading module: %s\n", tmpbuf);
+ mb_mod_length = get_image_size(tmpbuf);
if (mb_mod_length < 0) {
- fprintf(stderr, "Failed to open file '%s'\n", initrd_filename);
+ fprintf(stderr, "Failed to open file '%s'\n", tmpbuf);
exit(1);
}
mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_mod_length + mbs.mb_buf_size);
mbs.mb_buf = g_realloc(mbs.mb_buf, mbs.mb_buf_size);
- load_image(initrd_filename, (unsigned char *)mbs.mb_buf + offs);
+ load_image(tmpbuf, (unsigned char *)mbs.mb_buf + offs);
mb_add_mod(&mbs, mbs.mb_buf_phys + offs,
mbs.mb_buf_phys + offs + mb_mod_length, c);
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCHv3] multiboot: copy the cmdline verbatim, unescape module strings
2016-12-15 12:32 [Qemu-devel] [PATCHv3] multiboot: copy the cmdline verbatim, unescape module strings Vlad Lungu
@ 2016-12-18 20:25 ` Eduardo Habkost
2016-12-19 9:35 ` Vlad Lungu
0 siblings, 1 reply; 4+ messages in thread
From: Eduardo Habkost @ 2016-12-18 20:25 UTC (permalink / raw)
To: Vlad Lungu; +Cc: qemu-devel, pbonzini, rth
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 <vlad.lungu@windriver.com>
> ---
> 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.
The rest of the patch looks good.
> not_last = *next_initrd;
> - *next_initrd = '\0';
> /* if a space comes after the module filename, treat everything
> after that as parameters */
> - hwaddr c = mb_add_cmdline(&mbs, initrd_filename);
> - if ((next_space = strchr(initrd_filename, ' ')))
> + hwaddr c = mb_add_cmdline(&mbs, tmpbuf);
> + if ((next_space = strchr(tmpbuf, ' ')))
> *next_space = '\0';
> - mb_debug("multiboot loading module: %s\n", initrd_filename);
> - mb_mod_length = get_image_size(initrd_filename);
> + mb_debug("multiboot loading module: %s\n", tmpbuf);
> + mb_mod_length = get_image_size(tmpbuf);
> if (mb_mod_length < 0) {
> - fprintf(stderr, "Failed to open file '%s'\n", initrd_filename);
> + fprintf(stderr, "Failed to open file '%s'\n", tmpbuf);
> exit(1);
> }
>
> mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_mod_length + mbs.mb_buf_size);
> mbs.mb_buf = g_realloc(mbs.mb_buf, mbs.mb_buf_size);
>
> - load_image(initrd_filename, (unsigned char *)mbs.mb_buf + offs);
> + load_image(tmpbuf, (unsigned char *)mbs.mb_buf + offs);
> mb_add_mod(&mbs, mbs.mb_buf_phys + offs,
> mbs.mb_buf_phys + offs + mb_mod_length, c);
>
> --
> 1.9.1
>
--
Eduardo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCHv3] multiboot: copy the cmdline verbatim, unescape module strings
2016-12-18 20:25 ` Eduardo Habkost
@ 2016-12-19 9:35 ` Vlad Lungu
2016-12-19 21:01 ` Eduardo Habkost
0 siblings, 1 reply; 4+ messages in thread
From: Vlad Lungu @ 2016-12-19 9:35 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: qemu-devel, pbonzini, rth
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 <vlad.lungu@windriver.com>
>> ---
>> 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 :-)
Regards,
Vlad
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCHv3] multiboot: copy the cmdline verbatim, unescape module strings
2016-12-19 9:35 ` Vlad Lungu
@ 2016-12-19 21:01 ` Eduardo Habkost
0 siblings, 0 replies; 4+ messages in thread
From: Eduardo Habkost @ 2016-12-19 21:01 UTC (permalink / raw)
To: Vlad Lungu; +Cc: qemu-devel, pbonzini, rth
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 <vlad.lungu@windriver.com>
> >> ---
> >> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-12-19 21:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-15 12:32 [Qemu-devel] [PATCHv3] multiboot: copy the cmdline verbatim, unescape module strings Vlad Lungu
2016-12-18 20:25 ` Eduardo Habkost
2016-12-19 9:35 ` Vlad Lungu
2016-12-19 21:01 ` Eduardo Habkost
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).