Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 10/29/19 3:26 AM, Dr. David Alan Gilbert wrote:
> * Laszlo Ersek (lersek@redhat.com) wrote:
>> On 10/25/19 18:57, Dr. David Alan Gilbert (git) wrote:
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> 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 <dgilbert@redhat.com>
>>> ---
>>>   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);
>>>           }
>>>       }
> 
> I think I'm fine with that as well; want to add a signed off and post?
> 
>> (
>>
>> 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:
> 
> It's rather scary how much we rely on the grubby depths of the
> implementations of our conversion routines.
> 
>>> 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.
> 
> I'm happy either way.

Thanks, queued with Laszlo's changes.

Regards,

Phil.