Re: [Qemu-devel] [PATCH] inet_listen_opts: add error checking

[Eric Blake]

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.
> 
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---

>      /* lookup */
> -    if (port_offset)
> -        snprintf(port, sizeof(port), "%d", atoi(port) + port_offset);
> +    if (port_offset) {
> +        int baseport;
> +        errno = 0;
> +        baseport = strtol(port, NULL, 10);

Fail #1: Silent truncation on 64-bit platforms.  If the user passed
0x100000000 you will see baseport==0 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 != 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:

<rant>
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()?
</rant>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature