From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38328) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VqsoL-0001T6-Rs for qemu-devel@nongnu.org; Wed, 11 Dec 2013 18:03:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VqsoG-0008LU-VJ for qemu-devel@nongnu.org; Wed, 11 Dec 2013 18:03:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:23313) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VqsoG-0008LM-NC for qemu-devel@nongnu.org; Wed, 11 Dec 2013 18:03:16 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rBBN3FBJ020287 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 11 Dec 2013 18:03:15 -0500 Message-ID: <52A8EF33.4020207@redhat.com> Date: Wed, 11 Dec 2013 16:03:15 -0700 From: Eric Blake MIME-Version: 1.0 References: <1386763230-9202-1-git-send-email-kraxel@redhat.com> In-Reply-To: <1386763230-9202-1-git-send-email-kraxel@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="mBu71BA8QAtiecpJRSav2976AgLjXE4VO" Subject: Re: [Qemu-devel] [PATCH] inet_listen_opts: add error checking List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann , qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --mBu71BA8QAtiecpJRSav2976AgLjXE4VO Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 12/11/2013 05:00 AM, Gerd Hoffmann wrote: > Don't use atoi() function which doesn't detect errors, switch to > strtol and error out on failures. Also add a range check while > being at it. >=20 > Signed-off-by: Gerd Hoffmann > --- > /* lookup */ > - if (port_offset) > - snprintf(port, sizeof(port), "%d", atoi(port) + port_offset); > + if (port_offset) { > + int baseport; > + errno =3D 0; > + baseport =3D strtol(port, NULL, 10); Fail #1: Silent truncation on 64-bit platforms. If the user passed 0x100000000 you will see baseport=3D=3D0 with no error indication (and doesn't make it any nicer that a 32-bit platform will do what you wanted - platform-dependent bugs are nasty). :( You need to assign the results of strtol() into a long, then do range checking before truncating to int. > + if (errno !=3D 0) { > + error_setg(errp, "can't convert to a number: %s", port); > + return -1; > + } Fail #2: You forgot to do validation that a number was actually parsed. We hate atoi because atoi("a") is happily 0, but your use of strtol() still has that bug. POSIX states that implementations are allowed to fail with EINVAL when parsing "a", but this failure mode is not required to give an errno diagnostic. Your code would reject "a" on glibc, but accept it on other platforms (system-dependent bugs are nasty). The only portable way to ensure that a number was actually parsed and that there is no garbage in the suffix is to pass in the address of a char* in the second argument, then validate it against your string to ensure that enough information was parsed and any suffix is expected. The rest of this email is generic, and not specifically directed at you or your patch: WHY is strtol() such a PAINFUL interface to use correctly? And WHY can't qemu copy libvirt's lead of writing a SANE wrapper function, and then mandating that the rest of the code base use the sane wrapper instead of strtol()? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --mBu71BA8QAtiecpJRSav2976AgLjXE4VO Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.15 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJSqO8zAAoJEKeha0olJ0NqdaYH/24VDBwHTD9V91lku0dkX4ja L+qB0pcpiFmtrS/fK6o3+3vJ6K8uglqhG29VMdi53haxmXjdb1Hkl6plbJrfsbrh 7I1xgi3pB/hQJj9rFeYGSwIAD6v20OlXtiFHEHd9gxu12mb5Wpgc96Qe3UXua3AB JqWehewsv3/jpNKPqfVJYDeW4aN6udZM5/krCTLVR1hhLtt/Q0AatFla7o08wqV0 iticOS32un/q6msN6w7Ux8hSKMy3WKT+vMq1rxoSDMteharQvckHCBzdHa8c7Axh qlNHIjNUeiBQwR068CvQUFbg4PZ2eqVRg7WGgMxtvE1Wp4/0Uzy6eG4A2m9aU68= =qWkE -----END PGP SIGNATURE----- --mBu71BA8QAtiecpJRSav2976AgLjXE4VO--