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

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

[Gerd Hoffmann]

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 | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 8818d7c..22971e2 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -133,8 +133,19 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
         ai.ai_family = PF_INET6;
 
     /* lookup */
-    if (port_offset)
-        snprintf(port, sizeof(port), "%d", atoi(port) + port_offset);
+    if (port_offset) {
+        unsigned long long baseport;
+        if (parse_uint_full(port, &baseport, 10) < 0) {
+            error_setg(errp, "can't convert to a number: %s", port);
+            return -1;
+        }
+        if (base_port > 65535 ||
+            baseport + port_offset > 65535) {
+            error_setg(errp, "port %s out of range", port);
+            return -1;
+        }
+        snprintf(port, sizeof(port), "%d", (int)baseport + port_offset);
+    }
     rc = getaddrinfo(strlen(addr) ? addr : NULL, port, &ai, &res);
     if (rc != 0) {
         error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
-- 
1.8.3.1

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

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

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

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Gonglei (Arei)]

> -----Original Message-----
> From: qemu-devel-bounces+arei.gonglei=huawei.com@nongnu.org
> [mailto:qemu-devel-bounces+arei.gonglei=huawei.com@nongnu.org] On
> Behalf Of Gerd Hoffmann
> Sent: Wednesday, May 21, 2014 6:53 PM
> To: qemu-devel@nongnu.org
> Cc: Gerd Hoffmann
> Subject: [Qemu-devel] [PATCH] 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 | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 8818d7c..22971e2 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -133,8 +133,19 @@ int inet_listen_opts(QemuOpts *opts, int port_offset,
> Error **errp)
>          ai.ai_family = PF_INET6;
> 
>      /* lookup */
> -    if (port_offset)
> -        snprintf(port, sizeof(port), "%d", atoi(port) + port_offset);
> +    if (port_offset) {
> +        unsigned long long baseport;

unsigned long long is not necessary, unsigned long is enough.

> +        if (parse_uint_full(port, &baseport, 10) < 0) {
> +            error_setg(errp, "can't convert to a number: %s", port);
> +            return -1;
> +        }
> +        if (base_port > 65535 ||

base_port is undeclared.

> +            baseport + port_offset > 65535) {
> +            error_setg(errp, "port %s out of range", port);
> +            return -1;
> +        }
> +        snprintf(port, sizeof(port), "%d", (int)baseport + port_offset);
> +    }
>      rc = getaddrinfo(strlen(addr) ? addr : NULL, port, &ai, &res);
>      if (rc != 0) {
>          error_setg(errp, "address resolution failed for %s:%s: %s", addr,
> port,
> --
> 1.8.3.1
> 


Best regards,
-Gonglei

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

[Gerd Hoffmann]

> >      /* lookup */
> > -    if (port_offset)
> > -        snprintf(port, sizeof(port), "%d", atoi(port) + port_offset);
> > +    if (port_offset) {
> > +        unsigned long long baseport;
> 
> unsigned long long is not necessary, unsigned long is enough.
> 
> > +        if (parse_uint_full(port, &baseport, 10) < 0) {

parse_uint_full prototype uses "unsigned long long".  And on 32bit
machines it actually is a difference (long long is 64bit integer whereas
long is 32bit only).

> > +            error_setg(errp, "can't convert to a number: %s", port);
> > +            return -1;
> > +        }
> > +        if (base_port > 65535 ||
> 
> base_port is undeclared.

Oops, I'll fix.

cheers,
  Gerd

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

[Gonglei (Arei)]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Thursday, May 22, 2014 1:44 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH] inet_listen_opts: add error checking
> 
> > >      /* lookup */
> > > -    if (port_offset)
> > > -        snprintf(port, sizeof(port), "%d", atoi(port) + port_offset);
> > > +    if (port_offset) {
> > > +        unsigned long long baseport;
> >
> > unsigned long long is not necessary, unsigned long is enough.
> >
> > > +        if (parse_uint_full(port, &baseport, 10) < 0) {
> 
> parse_uint_full prototype uses "unsigned long long".  And on 32bit
> machines it actually is a difference (long long is 64bit integer whereas
> long is 32bit only).
> 
Okay, Thanks.


Best regards,
-Gonglei

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

[Gerd Hoffmann]

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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-sockets.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 8818d7c..6861feb 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -133,8 +133,19 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
         ai.ai_family = PF_INET6;
 
     /* lookup */
-    if (port_offset)
-        snprintf(port, sizeof(port), "%d", atoi(port) + port_offset);
+    if (port_offset) {
+        unsigned long long baseport;
+        if (parse_uint_full(port, &baseport, 10) < 0) {
+            error_setg(errp, "can't convert to a number: %s", port);
+            return -1;
+        }
+        if (baseport > 65535 ||
+            baseport + port_offset > 65535) {
+            error_setg(errp, "port %s out of range", port);
+            return -1;
+        }
+        snprintf(port, sizeof(port), "%d", (int)baseport + port_offset);
+    }
     rc = getaddrinfo(strlen(addr) ? addr : NULL, port, &ai, &res);
     if (rc != 0) {
         error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
-- 
1.8.3.1

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

[Gerd Hoffmann]

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.

[ v2: use parse_uint_full instead of strtol ]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/qemu-sockets.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 6b97dc1..5636510 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -133,8 +133,20 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
         ai.ai_family = PF_INET6;
 
     /* 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);
+        if (errno != 0) {
+            error_setg(errp, "can't convert to a number: %s", port);
+            return -1;
+        }
+        if (baseport < 0 || baseport + port_offset > 65535) {
+            error_setg(errp, "port %s out of range", port);
+            return -1;
+        }
+        snprintf(port, sizeof(port), "%d", baseport + port_offset);
+    }
     rc = getaddrinfo(strlen(addr) ? addr : NULL, port, &ai, &res);
     if (rc != 0) {
         error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
-- 
1.8.3.1

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

[Gerd Hoffmann]

  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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2111 bytes --]



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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Gerd Hoffmann]

  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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2182 bytes --]

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>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Gerd Hoffmann]

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

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 6b97dc1..5636510 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -133,8 +133,20 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
         ai.ai_family = PF_INET6;
 
     /* 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);
+        if (errno != 0) {
+            error_setg(errp, "can't convert to a number: %s", port);
+            return -1;
+        }
+        if (baseport < 0 || baseport + port_offset > 65535) {
+            error_setg(errp, "port %s out of range", port);
+            return -1;
+        }
+        snprintf(port, sizeof(port), "%d", baseport + port_offset);
+    }
     rc = getaddrinfo(strlen(addr) ? addr : NULL, port, &ai, &res);
     if (rc != 0) {
         error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
-- 
1.8.3.1