On 18/04/2019 20.13, Markus Armbruster wrote:
> Thomas Huth <th...@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 <borntrae...@de.ibm.com>
>>> Cc: Thomas Huth <th...@redhat.com>
>>> Cc: Cornelia Huck <coh...@redhat.com>
>>> Cc: qemu-s3...@nongnu.org
>>> Signed-off-by: Markus Armbruster <arm...@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
