1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 0/6] Fix misuse of ctype.h functions
  4. View patch

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

Markus Armbruster posted 6 patches 6 years, 9 months ago
Maintainers: Thomas Huth <thuth@redhat.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Cornelia Huck <cohuck@redhat.com>
There is a newer version of this series
[Qemu-devel] [PATCH 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.  But we're using isdigit() from pc-bios/s390-ccw/libc.h
here, which behaves nicely.  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 5/6] pc-bios/s390-ccw: Clean up harmless misuse of isdigit()
Posted by Thomas Huth 6 years, 9 months ago 
On 18/04/2019 16.53, 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.  But we're using isdigit() from pc-bios/s390-ccw/libc.h
> here, which behaves nicely.  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;
>          }
>      }

FWIW, "char" is unsigned by default on s390x, so this is doing nothing.

 Thomas
Re: [Qemu-devel] [PATCH 5/6] pc-bios/s390-ccw: Clean up harmless misuse of isdigit()
Posted by Markus Armbruster 6 years, 9 months ago 
Thomas Huth <thuth@redhat.com> writes:

> On 18/04/2019 16.53, 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.  But we're using isdigit() from pc-bios/s390-ccw/libc.h
>> here, which behaves nicely.  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;
>>          }
>>      }
>
> FWIW, "char" is unsigned by default on s390x, so this is doing nothing.

I see.

If we decide to keep the patch, the commit message needs tweaking.
Perhaps:

    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.
Re: [Qemu-devel] [PATCH 5/6] pc-bios/s390-ccw: Clean up harmless misuse of isdigit()
Posted by Thomas Huth 6 years, 9 months ago 
On 18/04/2019 20.13, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 18/04/2019 16.53, 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.  But we're using isdigit() from pc-bios/s390-ccw/libc.h
>>> here, which behaves nicely.  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;
>>>          }
>>>      }
>>
>> FWIW, "char" is unsigned by default on s390x, so this is doing nothing.
> 
> I see.
> 
> If we decide to keep the patch, the commit message needs tweaking.
> Perhaps:
> 
>     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.

Ok, I'll pick this up with the updated commit message.

 Thomas

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.