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

[Markus Armbruster]

Thomas Huth <address@hidden> 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 <address@hidden>
>> Cc: Thomas Huth <address@hidden>
>> Cc: Cornelia Huck <address@hidden>
>> Cc: address@hidden
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  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.