[Qemu-devel] [PATCH] cutils: Assert in-range base for string-to-integer conversions

Eric Blake posted 1 patch 23 weeks ago
Test checkpatch passed
Test docker-quick@centos7 passed
Test docker-clang@ubuntu passed
Test docker-mingw@fedora passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20181206151856.77503-1-eblake@redhat.com
util/cutils.c | 8 ++++++++
1 file changed, 8 insertions(+)

[Qemu-devel] [PATCH] cutils: Assert in-range base for string-to-integer conversions

Posted by Eric Blake 23 weeks ago
POSIX states that the value of endptr is unspecified if strtol()
fails with EINVAL due to an invalid base argument.  Since none of
the callers to check_strtox_error() initialized endptr, we could
end up propagating uninitialized data back to a caller on error.
However, passing an out-of-range base is already a sign of poor
programming, so let's just assert that base is in range, at which
point check_strtox_error() can be tightened to assert that it is
receiving an initialized ep that points somewhere within the
caller's original string, regardless of whether strto*() succeeded
or failed with ERANGE.

Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---

Also tested that this does not negatively impact David's pending
additions of qemu_strtod{,_finite}().  Thus:
Based-on: <20181121164421.20780-1-david@redhat.com>

 util/cutils.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/util/cutils.c b/util/cutils.c
index 91930d1bbeb..e098debdc0c 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -278,6 +278,7 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result)
 static int check_strtox_error(const char *nptr, char *ep,
                               const char **endptr, int libc_errno)
 {
+    assert(ep >= nptr);
     if (endptr) {
         *endptr = ep;
     }
@@ -325,6 +326,7 @@ int qemu_strtoi(const char *nptr, const char **endptr, int base,
     char *ep;
     long long lresult;

+    assert((unsigned) base <= 36 && base != 1);
     if (!nptr) {
         if (endptr) {
             *endptr = nptr;
@@ -377,6 +379,7 @@ int qemu_strtoui(const char *nptr, const char **endptr, int base,
     char *ep;
     long long lresult;

+    assert((unsigned) base <= 36 && base != 1);
     if (!nptr) {
         if (endptr) {
             *endptr = nptr;
@@ -433,6 +436,7 @@ int qemu_strtol(const char *nptr, const char **endptr, int base,
 {
     char *ep;

+    assert((unsigned) base <= 36 && base != 1);
     if (!nptr) {
         if (endptr) {
             *endptr = nptr;
@@ -475,6 +479,7 @@ int qemu_strtoul(const char *nptr, const char **endptr, int base,
 {
     char *ep;

+    assert((unsigned) base <= 36 && base != 1);
     if (!nptr) {
         if (endptr) {
             *endptr = nptr;
@@ -502,6 +507,7 @@ int qemu_strtoi64(const char *nptr, const char **endptr, int base,
 {
     char *ep;

+    assert((unsigned) base <= 36 && base != 1);
     if (!nptr) {
         if (endptr) {
             *endptr = nptr;
@@ -525,6 +531,7 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base,
 {
     char *ep;

+    assert((unsigned) base <= 36 && base != 1);
     if (!nptr) {
         if (endptr) {
             *endptr = nptr;
@@ -657,6 +664,7 @@ int parse_uint(const char *s, unsigned long long *value, char **endptr,
     char *endp = (char *)s;
     unsigned long long val = 0;

+    assert((unsigned) base <= 36 && base != 1);
     if (!s) {
         r = -EINVAL;
         goto out;
-- 
2.17.2


Re: [Qemu-devel] [Qemu-trivial] [PATCH] cutils: Assert in-range base for string-to-integer conversions

Posted by Laurent Vivier 23 weeks ago
Le 06/12/2018 à 16:18, Eric Blake a écrit :
> POSIX states that the value of endptr is unspecified if strtol()
> fails with EINVAL due to an invalid base argument.  Since none of
> the callers to check_strtox_error() initialized endptr, we could
> end up propagating uninitialized data back to a caller on error.
> However, passing an out-of-range base is already a sign of poor
> programming, so let's just assert that base is in range, at which
> point check_strtox_error() can be tightened to assert that it is
> receiving an initialized ep that points somewhere within the
> caller's original string, regardless of whether strto*() succeeded
> or failed with ERANGE.
> 
> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> Also tested that this does not negatively impact David's pending
> additions of qemu_strtod{,_finite}().  Thus:
> Based-on: <20181121164421.20780-1-david@redhat.com>
> 
>  util/cutils.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

Applied to my trivial-patches branch.

Thanks,
Laurent


Re: [Qemu-devel] [PATCH] cutils: Assert in-range base for string-to-integer conversions

Posted by Philippe Mathieu-Daudé 23 weeks ago
On 12/6/18 4:18 PM, Eric Blake wrote:
> POSIX states that the value of endptr is unspecified if strtol()
> fails with EINVAL due to an invalid base argument.  Since none of
> the callers to check_strtox_error() initialized endptr, we could
> end up propagating uninitialized data back to a caller on error.
> However, passing an out-of-range base is already a sign of poor
> programming, so let's just assert that base is in range, at which
> point check_strtox_error() can be tightened to assert that it is
> receiving an initialized ep that points somewhere within the
> caller's original string, regardless of whether strto*() succeeded
> or failed with ERANGE.
> 
> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> Also tested that this does not negatively impact David's pending
> additions of qemu_strtod{,_finite}().  Thus:
> Based-on: <20181121164421.20780-1-david@redhat.com>
> 
>  util/cutils.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/util/cutils.c b/util/cutils.c
> index 91930d1bbeb..e098debdc0c 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -278,6 +278,7 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result)
>  static int check_strtox_error(const char *nptr, char *ep,
>                                const char **endptr, int libc_errno)
>  {
> +    assert(ep >= nptr);
>      if (endptr) {
>          *endptr = ep;
>      }
> @@ -325,6 +326,7 @@ int qemu_strtoi(const char *nptr, const char **endptr, int base,
>      char *ep;
>      long long lresult;
> 
> +    assert((unsigned) base <= 36 && base != 1);
>      if (!nptr) {
>          if (endptr) {
>              *endptr = nptr;
> @@ -377,6 +379,7 @@ int qemu_strtoui(const char *nptr, const char **endptr, int base,
>      char *ep;
>      long long lresult;
> 
> +    assert((unsigned) base <= 36 && base != 1);
>      if (!nptr) {
>          if (endptr) {
>              *endptr = nptr;
> @@ -433,6 +436,7 @@ int qemu_strtol(const char *nptr, const char **endptr, int base,
>  {
>      char *ep;
> 
> +    assert((unsigned) base <= 36 && base != 1);
>      if (!nptr) {
>          if (endptr) {
>              *endptr = nptr;
> @@ -475,6 +479,7 @@ int qemu_strtoul(const char *nptr, const char **endptr, int base,
>  {
>      char *ep;
> 
> +    assert((unsigned) base <= 36 && base != 1);
>      if (!nptr) {
>          if (endptr) {
>              *endptr = nptr;
> @@ -502,6 +507,7 @@ int qemu_strtoi64(const char *nptr, const char **endptr, int base,
>  {
>      char *ep;
> 
> +    assert((unsigned) base <= 36 && base != 1);
>      if (!nptr) {
>          if (endptr) {
>              *endptr = nptr;
> @@ -525,6 +531,7 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base,
>  {
>      char *ep;
> 
> +    assert((unsigned) base <= 36 && base != 1);
>      if (!nptr) {
>          if (endptr) {
>              *endptr = nptr;
> @@ -657,6 +664,7 @@ int parse_uint(const char *s, unsigned long long *value, char **endptr,
>      char *endp = (char *)s;
>      unsigned long long val = 0;
> 
> +    assert((unsigned) base <= 36 && base != 1);
>      if (!s) {
>          r = -EINVAL;
>          goto out;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH] cutils: Assert in-range base for string-to-integer conversions

Posted by Markus Armbruster 23 weeks ago
Eric Blake <eblake@redhat.com> writes:

> POSIX states that the value of endptr is unspecified if strtol()
> fails with EINVAL due to an invalid base argument.  Since none of
> the callers to check_strtox_error() initialized endptr, we could
> end up propagating uninitialized data back to a caller on error.
> However, passing an out-of-range base is already a sign of poor
> programming, so let's just assert that base is in range, at which
> point check_strtox_error() can be tightened to assert that it is
> receiving an initialized ep that points somewhere within the
> caller's original string, regardless of whether strto*() succeeded
> or failed with ERANGE.
>
> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [Qemu-trivial] [PATCH] cutils: Assert in-range base for string-to-integer conversions

Posted by Laurent Vivier 23 weeks ago
Le 06/12/2018 à 16:18, Eric Blake a écrit :
> POSIX states that the value of endptr is unspecified if strtol()
> fails with EINVAL due to an invalid base argument.  Since none of
> the callers to check_strtox_error() initialized endptr, we could
> end up propagating uninitialized data back to a caller on error.
> However, passing an out-of-range base is already a sign of poor
> programming, so let's just assert that base is in range, at which
> point check_strtox_error() can be tightened to assert that it is
> receiving an initialized ep that points somewhere within the
> caller's original string, regardless of whether strto*() succeeded
> or failed with ERANGE.
> 
> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> Also tested that this does not negatively impact David's pending
> additions of qemu_strtod{,_finite}().  Thus:
> Based-on: <20181121164421.20780-1-david@redhat.com>
> 
>  util/cutils.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/util/cutils.c b/util/cutils.c
> index 91930d1bbeb..e098debdc0c 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -278,6 +278,7 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result)
>  static int check_strtox_error(const char *nptr, char *ep,
>                                const char **endptr, int libc_errno)
>  {
> +    assert(ep >= nptr);
>      if (endptr) {
>          *endptr = ep;
>      }
> @@ -325,6 +326,7 @@ int qemu_strtoi(const char *nptr, const char **endptr, int base,
>      char *ep;
>      long long lresult;
> 
> +    assert((unsigned) base <= 36 && base != 1);

If you want to play with type, I think you can do:

   assert((unsigned)(base - 2) <= 34)

Thanks,
Laurent

Re: [Qemu-devel] [Qemu-trivial] [PATCH] cutils: Assert in-range base for string-to-integer conversions

Posted by Laurent Vivier 23 weeks ago
Le 06/12/2018 à 17:40, Laurent Vivier a écrit :
> Le 06/12/2018 à 16:18, Eric Blake a écrit :
>> POSIX states that the value of endptr is unspecified if strtol()
>> fails with EINVAL due to an invalid base argument.  Since none of
>> the callers to check_strtox_error() initialized endptr, we could
>> end up propagating uninitialized data back to a caller on error.
>> However, passing an out-of-range base is already a sign of poor
>> programming, so let's just assert that base is in range, at which
>> point check_strtox_error() can be tightened to assert that it is
>> receiving an initialized ep that points somewhere within the
>> caller's original string, regardless of whether strto*() succeeded
>> or failed with ERANGE.
>>
>> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>
>> Also tested that this does not negatively impact David's pending
>> additions of qemu_strtod{,_finite}().  Thus:
>> Based-on: <20181121164421.20780-1-david@redhat.com>
>>
>>  util/cutils.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/util/cutils.c b/util/cutils.c
>> index 91930d1bbeb..e098debdc0c 100644
>> --- a/util/cutils.c
>> +++ b/util/cutils.c
>> @@ -278,6 +278,7 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result)
>>  static int check_strtox_error(const char *nptr, char *ep,
>>                                const char **endptr, int libc_errno)
>>  {
>> +    assert(ep >= nptr);
>>      if (endptr) {
>>          *endptr = ep;
>>      }
>> @@ -325,6 +326,7 @@ int qemu_strtoi(const char *nptr, const char **endptr, int base,
>>      char *ep;
>>      long long lresult;
>>
>> +    assert((unsigned) base <= 36 && base != 1);
> 
> If you want to play with type, I think you can do:
> 
>    assert((unsigned)(base - 2) <= 34)

oops, no, '0' is a valid case. Forgive this...

Laurent