inet_listen_opts: add error checking

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 <kraxel@redhat.com>
---
 util/qemu-sockets.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

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 <kraxel@redhat.com>
> ---

>      /* 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>

Hi,

> > +    if (port_offset) {
> > +        int baseport;
> > +        errno = 0;
> > +        baseport = strtol(port, NULL, 10);

> <rant>
> WHY is strtol() such a PAINFUL interface to use correctly?

Crossed my mind too after reading the manpage, which sayed you should
clear errno to reliable detect errors as checking the return value
doesn't cut it.

Your points obviously underline that.

>   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>

Care to share a pointer to the code?

thanks,
  Gerd

On 12/12/2013 05:27 AM, Gerd Hoffmann wrote:
>   Hi,
> 
>>> +    if (port_offset) {
>>> +        int baseport;
>>> +        errno = 0;
>>> +        baseport = strtol(port, NULL, 10);
> 
>> <rant>
>> WHY is strtol() such a PAINFUL interface to use correctly?
> 
> Crossed my mind too after reading the manpage, which sayed you should
> clear errno to reliable detect errors as checking the return value
> doesn't cut it.
> 
> Your points obviously underline that.
> 
>>   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>
> 
> Care to share a pointer to the code?

/* Like strtol, but produce an "int" result, and check more carefully.
   Return 0 upon success;  return -1 to indicate failure.
   When END_PTR is NULL, the byte after the final valid digit must be NUL.
   Otherwise, it's like strtol and lets the caller check any suffix for
   validity.  This function is careful to return -1 when the string S
   represents a number that is not representable as an "int". */
int
virStrToLong_i(char const *s, char **end_ptr, int base, int *result)
{
    long int val;
    char *p;
    int err;

    errno = 0;
    val = strtol(s, &p, base); /* exempt from syntax-check */
    err = (errno || (!end_ptr && *p) || p == s || (int) val != val);
    if (end_ptr)
        *end_ptr = p;
    if (err)
        return -1;
    *result = val;
    return 0;
}

and other variants of virStrToLong_* for parsing into unsigned int,
long, etc.

Libvirt then couples that with a syntax check that gets run during 'make
syntax-check' (or we could even migrate it into 'make check') that
forbids all use of strtol() not on a line with the magic exemption
comment.  Therefore, the number of actual uses of strtol() in the source
code base is limited to just these wrapper functions, and everyone else
gets sane semantics.

Hi,

> <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>

Turns out there already is one, just /me didn't know.
So, for the record and the mailing list archives:

util/cutil.c provides parse_uint() and parse_uint_full().

The first returns a pointer to the remaining bits to parse, so you can
inspect the suffix (if any).  The second errors out in case there is
trailing garbage, simliar to the libvirt wrapper in case you pass in
NULL as end_ptr.

cheers,
  Gerd