On 10/25/19 18:57, Dr. David Alan Gilbert (git) wrote:
From: "Dr. David Alan Gilbert" <address@hidden>
Commit ee5d0f89de3e53cdb0dc added range checking on reboot-timeout
to only allow the range 0..65535; however both qemu and libvirt document
the special value -1 to mean don't reboot.
Allow it again.
Fixes: ee5d0f89de3e53cdb0dc ("fw_cfg: Fix -boot reboot-timeout error checking")
RH bz: https://bugzilla.redhat.com/show_bug.cgi?id=1765443
Signed-off-by: Dr. David Alan Gilbert <address@hidden>
---
hw/nvram/fw_cfg.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 7dc3ac378e..1a9ec44232 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -247,10 +247,11 @@ static void fw_cfg_reboot(FWCfgState *s)
if (reboot_timeout) {
rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
+
/* validate the input */
- if (rt_val < 0 || rt_val > 0xffff) {
+ if (rt_val < -1 || rt_val > 0xffff) {
error_report("reboot timeout is invalid,"
- "it should be a value between 0 and 65535");
+ "it should be a value between -1 and 65535");
exit(1);
}
}
Ouch.
Here's the prototype of qemu_opt_get_number():
uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
So, when we call it, here's what we actually do:
rt_val = (int64_t)qemu_opt_get_number(opts, "reboot-timeout",
(uint64_t)-1);
^^^^^^^^^
^^^^^^^^^^
The conversion to uint64_t is fine.
The conversion to int64_t is not great:
Otherwise, the new type is signed and the value cannot be represented
in it; either the result is implementation-defined or an
implementation-defined signal is raised.
I guess we're exploiting two's complement, as the implementation-defined
result. Not great. :)
Here's what I'd prefer:
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 7dc3ac378ee0..16413550a1da 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -237,7 +237,7 @@ static void fw_cfg_bootsplash(FWCfgState *s)
static void fw_cfg_reboot(FWCfgState *s)
{
const char *reboot_timeout = NULL;
- int64_t rt_val = -1;
+ uint64_t rt_val = -1;
uint32_t rt_le32;
/* get user configuration */
@@ -248,9 +248,9 @@ static void fw_cfg_reboot(FWCfgState *s)
if (reboot_timeout) {
rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
/* validate the input */
- if (rt_val < 0 || rt_val > 0xffff) {
+ if (rt_val > 0xffff && rt_val != (uint64_t)-1) {
error_report("reboot timeout is invalid,"
- "it should be a value between 0 and 65535");
+ "it should be a value between -1 and 65535");
exit(1);
}
}
(
The trick is that strtoull(), in
qemu_opt_get_number()
qemu_opt_get_number_helper()
parse_option_number()
qemu_strtou64()
strtoull()
turns "-1" into (uint64_t)-1, which counts as a valid conversion, per
spec:
If the subject sequence has the expected form and the value of /base/
is zero, the sequence of characters starting with the first digit is
interpreted as an integer constant according to the rules of 6.4.4.1.
If the subject sequence has the expected form and the value of /base/
is between 2 and 36, it is used as the base for conversion, ascribing
to each letter its value as given above. If the subject sequence
begins with a minus sign, the value resulting from the conversion is
negated (in the return type). A pointer to the final string is stored
in the object pointed to by /endptr/, provided that /endptr/ is not a
null pointer.
)
I don't insist though; if Phil is OK with the posted patch, I won't try
to block it.