From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37126) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkgOL-0000fu-PO for qemu-devel@nongnu.org; Thu, 06 Jun 2013 16:02:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UkgOH-0007ZV-1b for qemu-devel@nongnu.org; Thu, 06 Jun 2013 16:02:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59413) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkgOG-0007ZN-QN for qemu-devel@nongnu.org; Thu, 06 Jun 2013 16:02:32 -0400 From: Markus Armbruster References: <1370536046-15125-1-git-send-email-armbru@redhat.com> <1370536046-15125-8-git-send-email-armbru@redhat.com> <51B0D75E.8000305@redhat.com> Date: Thu, 06 Jun 2013 22:02:30 +0200 In-Reply-To: <51B0D75E.8000305@redhat.com> (Laszlo Ersek's message of "Thu, 06 Jun 2013 20:39:26 +0200") Message-ID: <874ndbrort.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 7/7] smbios: Check R in -smbios type=0, release=R parses okay List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org Laszlo Ersek writes: > On 06/06/13 18:27, Markus Armbruster wrote: >> Signed-off-by: Markus Armbruster >> --- >> 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 Thanks for the review!