Laszlo Ersek <ler...@redhat.com> writes:
> On 06/06/13 18:27, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> hw/i386/smbios.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
>> index 68bd6d0..88a1360 100644
>> --- a/hw/i386/smbios.c
>> +++ b/hw/i386/smbios.c
>> @@ -140,7 +140,10 @@ static void smbios_build_type_0_fields(const char *t)
>> bios_release_date_str),
>> buf, strlen(buf) + 1);
>> if (get_param_value(buf, sizeof(buf), "release", t)) {
>> - sscanf(buf, "%hhd.%hhd", &major, &minor);
>> + if (sscanf(buf, "%hhd.%hhd", &major, &minor) != 2) {
>> + error_report("Invalid release");
>> + exit(1);
>> + }
>> smbios_add_field(0, offsetof(struct smbios_type_0,
>> system_bios_major_release),
>> &major, 1);
>>
>
> Right. OTOH if any of the decimal strings provided doesn't fit into the
> space provided (eg. you pass "256" for an "unsigned char" which happens
> to be uint8_t), the behavior is undefined anyway. sscanf() cannot be
> used with "untrusted" data. ("... if the result of the conversion cannot
> be represented in the space provided, the behavior is undefined.")
Yes, this isn't rigorous parsing. It improves the code from "brazenly
careless" to the more common (in QEMU) "quick but sloppy".
> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks for the review!
