| Message ID | 20181110034115.103335-3-liq3ea@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | refactor fw_cfg_bootsplash() and fw_cfg_reboot() | expand |
Li Qiang <liq3ea@163.com> writes: > Currently the user can set a negative reboot_timeout. > Also it is wrong to parse 'reboot-timeout' with qemu_opt_get() and then > convert it to number. Again, it's not wrong per se, just needlessly complicated and error-prone. What makes it wrong is ... > convert it to number. This patch refactor this function by following: > 1. ensure reboot_timeout is in 0~0xffff > 2. use qemu_opt_get_number() to parse reboot_timeout > 3. simlify code > > Signed-off-by: Li Qiang <liq3ea@163.com> > --- > hw/nvram/fw_cfg.c | 23 +++++++++++------------ > vl.c | 2 +- > 2 files changed, 12 insertions(+), 13 deletions(-) > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 78f43dad93..6aca80846a 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -178,24 +178,23 @@ static void fw_cfg_bootsplash(FWCfgState *s) > > static void fw_cfg_reboot(FWCfgState *s) > { > - int reboot_timeout = -1; > - char *p; > - const char *temp; > + const char *reboot_timeout = NULL; > > /* get user configuration */ > QemuOptsList *plist = qemu_find_opts("boot-opts"); > QemuOpts *opts = QTAILQ_FIRST(&plist->head); > - if (opts != NULL) { > - temp = qemu_opt_get(opts, "reboot-timeout"); > - if (temp != NULL) { > - p = (char *)temp; > - reboot_timeout = strtol(p, &p, 10); ... the total lack of error checking here. Same in PATCH 1. Here's my attempt at a clearer commit message: fw_cfg: Fix -boot reboot-timeout error checking fw_cfg_reboot() gets option parameter "reboot-timeout" with qemu_opt_get(), then converts it to an integer by hand. It neglects to check that conversion for errors, and fails to reject negative values. Positive values above the limit get reported and replaced by the limit. Check for conversion errors properly, and reject all values outside 0..0xffff. PATCH 1's commit message could be improved the same way. > - } > + reboot_timeout = qemu_opt_get(opts, "reboot-timeout"); > + > + if (reboot_timeout == NULL) { > + return; > } > + int64_t rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1); > + > /* validate the input */ > - if (reboot_timeout > 0xffff) { > - error_report("reboot timeout is larger than 65535, force it to 65535."); > - reboot_timeout = 0xffff; > + if (rt_val < 0 || rt_val > 0xffff) { > + error_report("reboot timeout is invalid," > + "it should be a value between 0 and 65535"); > + exit(1); > } > fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&reboot_timeout, 4), 4); > } Change in behavior when "reboot-timeout" isn't specified. Before your patch, we fw_cfg_add_file() with a value of -1. After your patch, we don't fw_cfg_add_file(). Why is that okay? > diff --git a/vl.c b/vl.c > index be37da46f0..086127ff0b 100644 > --- a/vl.c > +++ b/vl.c > @@ -339,7 +339,7 @@ static QemuOptsList qemu_boot_opts = { > .type = QEMU_OPT_NUMBER, > }, { > .name = "reboot-timeout", > - .type = QEMU_OPT_STRING, > + .type = QEMU_OPT_NUMBER, > }, { > .name = "strict", > .type = QEMU_OPT_BOOL,
At 2018-11-17 00:52:58, "Markus Armbruster" <armbru@redhat.com> wrote: >Li Qiang <liq3ea@163.com> writes: > >> Currently the user can set a negative reboot_timeout. >> Also it is wrong to parse 'reboot-timeout' with qemu_opt_get() and then >> convert it to number. > >Again, it's not wrong per se, just needlessly complicated and >error-prone. What makes it wrong is ... > >> convert it to number. This patch refactor this function by following: >> 1. ensure reboot_timeout is in 0~0xffff >> 2. use qemu_opt_get_number() to parse reboot_timeout >> 3. simlify code >> >> Signed-off-by: Li Qiang <liq3ea@163.com> >> --- >> hw/nvram/fw_cfg.c | 23 +++++++++++------------ >> vl.c | 2 +- >> 2 files changed, 12 insertions(+), 13 deletions(-) >> >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >> index 78f43dad93..6aca80846a 100644 >> --- a/hw/nvram/fw_cfg.c >> +++ b/hw/nvram/fw_cfg.c >> @@ -178,24 +178,23 @@ static void fw_cfg_bootsplash(FWCfgState *s) >> >> static void fw_cfg_reboot(FWCfgState *s) >> { >> - int reboot_timeout = -1; >> - char *p; >> - const char *temp; >> + const char *reboot_timeout = NULL; >> >> /* get user configuration */ >> QemuOptsList *plist = qemu_find_opts("boot-opts"); >> QemuOpts *opts = QTAILQ_FIRST(&plist->head); >> - if (opts != NULL) { >> - temp = qemu_opt_get(opts, "reboot-timeout"); >> - if (temp != NULL) { >> - p = (char *)temp; >> - reboot_timeout = strtol(p, &p, 10); > >... the total lack of error checking here. Same in PATCH 1. > Got. >Here's my attempt at a clearer commit message: > > fw_cfg: Fix -boot reboot-timeout error checking > > fw_cfg_reboot() gets option parameter "reboot-timeout" with > qemu_opt_get(), then converts it to an integer by hand. It neglects > to check that conversion for errors, and fails to reject negative > values. Positive values above the limit get reported and replaced > by the limit. > > Check for conversion errors properly, and reject all values outside > 0..0xffff. > Thanks for your advice, I appreciate it and will change in the revision version. >PATCH 1's commit message could be improved the same way. > >> - } >> + reboot_timeout = qemu_opt_get(opts, "reboot-timeout"); >> + >> + if (reboot_timeout == NULL) { >> + return; >> } >> + int64_t rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1); >> + >> /* validate the input */ >> - if (reboot_timeout > 0xffff) { >> - error_report("reboot timeout is larger than 65535, force it to 65535."); >> - reboot_timeout = 0xffff; >> + if (rt_val < 0 || rt_val > 0xffff) { >> + error_report("reboot timeout is invalid," >> + "it should be a value between 0 and 65535"); >> + exit(1); >> } >> fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&reboot_timeout, 4), 4); >> } > >Change in behavior when "reboot-timeout" isn't specified. > >Before your patch, we fw_cfg_add_file() with a value of -1. > >After your patch, we don't fw_cfg_add_file(). > >Why is that okay? > Here I following Gerd's advice. For values >0xffff or < 0, report and exit. -->http://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00551.html Thanks, Li Qiang >> diff --git a/vl.c b/vl.c >> index be37da46f0..086127ff0b 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -339,7 +339,7 @@ static QemuOptsList qemu_boot_opts = { >> .type = QEMU_OPT_NUMBER, >> }, { >> .name = "reboot-timeout", >> - .type = QEMU_OPT_STRING, >> + .type = QEMU_OPT_NUMBER, >> }, { >> .name = "strict", >> .type = QEMU_OPT_BOOL,
ÀîÇ¿ <liq3ea@163.com> writes: > At 2018-11-17 00:52:58, "Markus Armbruster" <armbru@redhat.com> wrote: >>Li Qiang <liq3ea@163.com> writes: >> >>> Currently the user can set a negative reboot_timeout. >>> Also it is wrong to parse 'reboot-timeout' with qemu_opt_get() and then >>> convert it to number. >> >>Again, it's not wrong per se, just needlessly complicated and >>error-prone. What makes it wrong is ... >> >>> convert it to number. This patch refactor this function by following: >>> 1. ensure reboot_timeout is in 0~0xffff >>> 2. use qemu_opt_get_number() to parse reboot_timeout >>> 3. simlify code >>> >>> Signed-off-by: Li Qiang <liq3ea@163.com> >>> --- >>> hw/nvram/fw_cfg.c | 23 +++++++++++------------ >>> vl.c | 2 +- >>> 2 files changed, 12 insertions(+), 13 deletions(-) >>> >>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >>> index 78f43dad93..6aca80846a 100644 >>> --- a/hw/nvram/fw_cfg.c >>> +++ b/hw/nvram/fw_cfg.c >>> @@ -178,24 +178,23 @@ static void fw_cfg_bootsplash(FWCfgState *s) >>> >>> static void fw_cfg_reboot(FWCfgState *s) >>> { >>> - int reboot_timeout = -1; >>> - char *p; >>> - const char *temp; >>> + const char *reboot_timeout = NULL; >>> >>> /* get user configuration */ >>> QemuOptsList *plist = qemu_find_opts("boot-opts"); >>> QemuOpts *opts = QTAILQ_FIRST(&plist->head); >>> - if (opts != NULL) { >>> - temp = qemu_opt_get(opts, "reboot-timeout"); >>> - if (temp != NULL) { >>> - p = (char *)temp; >>> - reboot_timeout = strtol(p, &p, 10); >> >>... the total lack of error checking here. Same in PATCH 1. > >> > > > Got. > > >>Here's my attempt at a clearer commit message: >> >> fw_cfg: Fix -boot reboot-timeout error checking >> >> fw_cfg_reboot() gets option parameter "reboot-timeout" with >> qemu_opt_get(), then converts it to an integer by hand. It neglects >> to check that conversion for errors, and fails to reject negative >> values. Positive values above the limit get reported and replaced >> by the limit. >> >> Check for conversion errors properly, and reject all values outside >> 0..0xffff. > >> > > > Thanks for your advice, I appreciate it and will change in the revision version. > > >>PATCH 1's commit message could be improved the same way. >> >>> - } >>> + reboot_timeout = qemu_opt_get(opts, "reboot-timeout"); >>> + >>> + if (reboot_timeout == NULL) { >>> + return; >>> } >>> + int64_t rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1); >>> + >>> /* validate the input */ >>> - if (reboot_timeout > 0xffff) { >>> - error_report("reboot timeout is larger than 65535, force it to 65535."); >>> - reboot_timeout = 0xffff; >>> + if (rt_val < 0 || rt_val > 0xffff) { >>> + error_report("reboot timeout is invalid," >>> + "it should be a value between 0 and 65535"); >>> + exit(1); >>> } >>> fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&reboot_timeout, 4), 4); >>> } >> >>Change in behavior when "reboot-timeout" isn't specified. >> >>Before your patch, we fw_cfg_add_file() with a value of -1. >> >>After your patch, we don't fw_cfg_add_file(). >> >>Why is that okay? > >> > > > Here I following Gerd's advice. > For values >0xffff or < 0, report and exit. > -->http://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00551.html Cases: 0. "reboot-timeout" not specified (e.g. no -boot option given) 1. "reboot-timeout" specified, value out of bounds 1.a. < 0 1.b. > 0xffff 2. "reboot-timeout" specified, value okay Gerd's advice is about case 1. Your patch implements it. My question is about case 0. Do you understand my question now? [...]
Markus Armbruster <armbru@redhat.com> 于2018年11月19日周一 下午3:01写道： > ÀîÇ¿ <liq3ea@163.com> writes: > > > At 2018-11-17 00:52:58, "Markus Armbruster" <armbru@redhat.com> wrote: > >>Li Qiang <liq3ea@163.com> writes: > >> > >>> Currently the user can set a negative reboot_timeout. > >>> Also it is wrong to parse 'reboot-timeout' with qemu_opt_get() and then > >>> convert it to number. > >> > >>Again, it's not wrong per se, just needlessly complicated and > >>error-prone. What makes it wrong is ... > >> > >>> convert it to number. This patch refactor this function by following: > >>> 1. ensure reboot_timeout is in 0~0xffff > >>> 2. use qemu_opt_get_number() to parse reboot_timeout > >>> 3. simlify code > >>> > >>> Signed-off-by: Li Qiang <liq3ea@163.com> > >>> --- > >>> hw/nvram/fw_cfg.c | 23 +++++++++++------------ > >>> vl.c | 2 +- > >>> 2 files changed, 12 insertions(+), 13 deletions(-) > >>> > >>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > >>> index 78f43dad93..6aca80846a 100644 > >>> --- a/hw/nvram/fw_cfg.c > >>> +++ b/hw/nvram/fw_cfg.c > >>> @@ -178,24 +178,23 @@ static void fw_cfg_bootsplash(FWCfgState *s) > >>> > >>> static void fw_cfg_reboot(FWCfgState *s) > >>> { > >>> - int reboot_timeout = -1; > >>> - char *p; > >>> - const char *temp; > >>> + const char *reboot_timeout = NULL; > >>> > >>> /* get user configuration */ > >>> QemuOptsList *plist = qemu_find_opts("boot-opts"); > >>> QemuOpts *opts = QTAILQ_FIRST(&plist->head); > >>> - if (opts != NULL) { > >>> - temp = qemu_opt_get(opts, "reboot-timeout"); > >>> - if (temp != NULL) { > >>> - p = (char *)temp; > >>> - reboot_timeout = strtol(p, &p, 10); > >> > >>... the total lack of error checking here. Same in PATCH 1. > > > >> > > > > > > Got. > > > > > >>Here's my attempt at a clearer commit message: > >> > >> fw_cfg: Fix -boot reboot-timeout error checking > >> > >> fw_cfg_reboot() gets option parameter "reboot-timeout" with > >> qemu_opt_get(), then converts it to an integer by hand. It neglects > >> to check that conversion for errors, and fails to reject negative > >> values. Positive values above the limit get reported and replaced > >> by the limit. > >> > >> Check for conversion errors properly, and reject all values outside > >> 0..0xffff. > > > >> > > > > > > Thanks for your advice, I appreciate it and will change in the revision > version. > > > > > >>PATCH 1's commit message could be improved the same way. > >> > >>> - } > >>> + reboot_timeout = qemu_opt_get(opts, "reboot-timeout"); > >>> + > >>> + if (reboot_timeout == NULL) { > >>> + return; > >>> } > >>> + int64_t rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1); > >>> + > >>> /* validate the input */ > >>> - if (reboot_timeout > 0xffff) { > >>> - error_report("reboot timeout is larger than 65535, force it > to 65535."); > >>> - reboot_timeout = 0xffff; > >>> + if (rt_val < 0 || rt_val > 0xffff) { > >>> + error_report("reboot timeout is invalid," > >>> + "it should be a value between 0 and 65535"); > >>> + exit(1); > >>> } > >>> fw_cfg_add_file(s, "etc/boot-fail-wait", > g_memdup(&reboot_timeout, 4), 4); > >>> } > >> > >>Change in behavior when "reboot-timeout" isn't specified. > >> > >>Before your patch, we fw_cfg_add_file() with a value of -1. > >> > >>After your patch, we don't fw_cfg_add_file(). > >> > >>Why is that okay? > > > >> > > > > > > Here I following Gerd's advice. > > For values >0xffff or < 0, report and exit. > > -->http://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00551.html > > Cases: > > 0. "reboot-timeout" not specified (e.g. no -boot option given) > > 1. "reboot-timeout" specified, value out of bounds > 1.a. < 0 > 1.b. > 0xffff > > 2. "reboot-timeout" specified, value okay > > Gerd's advice is about case 1. Your patch implements it. > > My question is about case 0. > > Do you understand my question now? > OK got. Once I think the 'reboot_timeout' can't be -1(as the user can't set this), but seems it's ok to be -1(the default value as no 'reboot-timeout' specified). I will prepare another patchset later. Thanks, Li Qiang > > [...] >
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 78f43dad93..6aca80846a 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -178,24 +178,23 @@ static void fw_cfg_bootsplash(FWCfgState *s) static void fw_cfg_reboot(FWCfgState *s) { - int reboot_timeout = -1; - char *p; - const char *temp; + const char *reboot_timeout = NULL; /* get user configuration */ QemuOptsList *plist = qemu_find_opts("boot-opts"); QemuOpts *opts = QTAILQ_FIRST(&plist->head); - if (opts != NULL) { - temp = qemu_opt_get(opts, "reboot-timeout"); - if (temp != NULL) { - p = (char *)temp; - reboot_timeout = strtol(p, &p, 10); - } + reboot_timeout = qemu_opt_get(opts, "reboot-timeout"); + + if (reboot_timeout == NULL) { + return; } + int64_t rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1); + /* validate the input */ - if (reboot_timeout > 0xffff) { - error_report("reboot timeout is larger than 65535, force it to 65535."); - reboot_timeout = 0xffff; + if (rt_val < 0 || rt_val > 0xffff) { + error_report("reboot timeout is invalid," + "it should be a value between 0 and 65535"); + exit(1); } fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&reboot_timeout, 4), 4); } diff --git a/vl.c b/vl.c index be37da46f0..086127ff0b 100644 --- a/vl.c +++ b/vl.c @@ -339,7 +339,7 @@ static QemuOptsList qemu_boot_opts = { .type = QEMU_OPT_NUMBER, }, { .name = "reboot-timeout", - .type = QEMU_OPT_STRING, + .type = QEMU_OPT_NUMBER, }, { .name = "strict", .type = QEMU_OPT_BOOL,
Currently the user can set a negative reboot_timeout. Also it is wrong to parse 'reboot-timeout' with qemu_opt_get() and then convert it to number. This patch refactor this function by following: 1. ensure reboot_timeout is in 0~0xffff 2. use qemu_opt_get_number() to parse reboot_timeout 3. simlify code Signed-off-by: Li Qiang <liq3ea@163.com> --- hw/nvram/fw_cfg.c | 23 +++++++++++------------ vl.c | 2 +- 2 files changed, 12 insertions(+), 13 deletions(-)