[Qemu-devel] [PATCH v2 5/6] pc-bios/s390-ccw: Clean up harmless misuse of isdigit()

Markus Armbruster posted 6 patches 6 years, 9 months ago
Maintainers: Christian Borntraeger <borntraeger@de.ibm.com>, Thomas Huth <thuth@redhat.com>, Cornelia Huck <cohuck@redhat.com>
[Qemu-devel] [PATCH v2 5/6] pc-bios/s390-ccw: Clean up harmless misuse of isdigit()
Posted by Markus Armbruster 6 years, 9 months ago
atoui() and get_index() pass char values to isdigit().  With a
standard isdigit(), we'd get undefined behavior when the value is
negative.  Can't happen as char is unsigned on s390x.  Even if it
could, we're actually using isdigit() from pc-bios/s390-ccw/libc.h
here, which works fine for negative values.  Clean up anyway, just
to avoid setting a bad example.

Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: qemu-s390x@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 pc-bios/s390-ccw/libc.c | 2 +-
 pc-bios/s390-ccw/menu.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/pc-bios/s390-ccw/libc.c b/pc-bios/s390-ccw/libc.c
index a786566c4c..3187923950 100644
--- a/pc-bios/s390-ccw/libc.c
+++ b/pc-bios/s390-ccw/libc.c
@@ -38,7 +38,7 @@ uint64_t atoui(const char *str)
     }
 
     while (*str) {
-        if (!isdigit(*str)) {
+        if (!isdigit(*(unsigned char *)str)) {
             break;
         }
         val = val * 10 + *str - '0';
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 82a4ae6315..ce3815b201 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -134,7 +134,7 @@ static int get_index(void)
 
     /* Check for erroneous input */
     for (i = 0; i < len; i++) {
-        if (!isdigit(buf[i])) {
+        if (!isdigit((unsigned char)buf[i])) {
             return -1;
         }
     }
-- 
2.17.2


Re: [Qemu-devel] [PATCH v2 5/6] pc-bios/s390-ccw: Clean up harmless misuse of isdigit()
Posted by Christian Borntraeger 6 years, 9 months ago

On 14.05.19 20:03, Markus Armbruster wrote:
> atoui() and get_index() pass char values to isdigit().  With a
> standard isdigit(), we'd get undefined behavior when the value is
> negative.  Can't happen as char is unsigned on s390x.  Even if it
> could, we're actually using isdigit() from pc-bios/s390-ccw/libc.h
> here, which works fine for negative values.  Clean up anyway, just
> to avoid setting a bad example.
> 
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: qemu-s390x@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  pc-bios/s390-ccw/libc.c | 2 +-
>  pc-bios/s390-ccw/menu.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/libc.c b/pc-bios/s390-ccw/libc.c
> index a786566c4c..3187923950 100644
> --- a/pc-bios/s390-ccw/libc.c
> +++ b/pc-bios/s390-ccw/libc.c
> @@ -38,7 +38,7 @@ uint64_t atoui(const char *str)
>      }
>  
>      while (*str) {
> -        if (!isdigit(*str)) {
> +        if (!isdigit(*(unsigned char *)str)) {
>              break;
>          }
>          val = val * 10 + *str - '0';
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> index 82a4ae6315..ce3815b201 100644
> --- a/pc-bios/s390-ccw/menu.c
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -134,7 +134,7 @@ static int get_index(void)
>  
>      /* Check for erroneous input */
>      for (i = 0; i < len; i++) {
> -        if (!isdigit(buf[i])) {
> +        if (!isdigit((unsigned char)buf[i])) {
>              return -1;
>          }
>      }
> 


Re: [Qemu-devel] [PATCH v2 5/6] pc-bios/s390-ccw: Clean up harmless misuse of isdigit()
Posted by Thomas Huth 6 years, 9 months ago
On 14/05/2019 20.04, Christian Borntraeger wrote:
> 
> 
> On 14.05.19 20:03, Markus Armbruster wrote:
>> atoui() and get_index() pass char values to isdigit().  With a
>> standard isdigit(), we'd get undefined behavior when the value is
>> negative.  Can't happen as char is unsigned on s390x.  Even if it
>> could, we're actually using isdigit() from pc-bios/s390-ccw/libc.h
>> here, which works fine for negative values.  Clean up anyway, just
>> to avoid setting a bad example.
>>
>> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

FYI, this patch is already queued in Cornelia's s390-next tree (and it
should also go via her tree, since you need to rebuild the bios binary
afterwards).

 Thomas