Re: [Qemu-devel] [qemu-s390x] [PATCH v2 1/5] s390-ccw: update libc

["Collin L. Walling" <walling@linux.vnet.ibm.com>]

On 12/18/2017 08:06 AM, Thomas Huth wrote:
> On 11.12.2017 23:19, Collin L. Walling wrote:
>> Moved:
>>    memcmp from bootmap.h to libc.h (renamed from _memcmp)
>>    strlen from sclp.c to libc.h (renamed from _strlen)
>>
>> Added C standard functions:
>>    isdigit
>>    atoi
>>
>> Added non-C standard function:
>>    itostr
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
>> ---
>>   pc-bios/s390-ccw/Makefile  |  2 +-
>>   pc-bios/s390-ccw/bootmap.c |  4 +--
>>   pc-bios/s390-ccw/bootmap.h | 16 +---------
>>   pc-bios/s390-ccw/libc.c    | 75 ++++++++++++++++++++++++++++++++++++++++++++++
>>   pc-bios/s390-ccw/libc.h    | 31 +++++++++++++++++++
>>   pc-bios/s390-ccw/main.c    | 17 +----------
>>   pc-bios/s390-ccw/sclp.c    | 10 +------
>>   7 files changed, 112 insertions(+), 43 deletions(-)
>>   create mode 100644 pc-bios/s390-ccw/libc.c
> [...]
>> +
>> +/**
>> + * itostr:
>> + * @num: the integer to be converted.
>> + * @str: a pointer to a string to store the conversion.
>> + * @len: the length of the passed string.
>> + *
>> + * Given an integer @num, convert it to a string. The string @str must be
>> + * allocated beforehand. The resulting string will be null terminated and
>> + * returned.
>> + *
>> + * Returns: the string @str of the converted integer @num.
>> + */
>> +char *itostr(int num, char *str, size_t len)
>> +{
>> +    long num_len = 1;
>> +    int tmp = num;
>> +    int i;
>> +
>> +    /* Count length of num */
>> +    while ((tmp /= 10) > 0) {
>> +        num_len++;
>> +    }
>> +
>> +    /* Check if we have enough space for num and null */
>> +    if (len < num_len) {
>> +        return 0;
>> +    }
> I'm afraid, but I think you've got an off-by-one bug in this code.
>
> In patch 5, you're using this function like this:
>
>      char tmp[4];
>
>      sclp_print(itostr(entries, tmp, sizeof(tmp)));
>
> That means if entries is >= 1000 for example, num_len is 4 ...
>
>> +    /* Convert int to string */
>> +    for (i = num_len - 1; i >= 0; i--) {
>> +        str[i] = num % 10 + '0';
>> +        num /= 10;
>> +    }
>> +
>> +    str[num_len] = '\0';
> ... and then you run into a buffer overflow here.


Doh, you're correct.  I forgot to put a "<=" in the len / num_len check.
That should fix things up.  Thanks for catching that.


>
>> +    return str;
>> +}
> Maybe it would also make more sense to panic() instead of "return 0"
> since you don't check the return value in patch 5 ?


I'm a bit conflicted about doing something like that.  I'm not sure if 
there's any kind
of guideline we want to follow for defining functions in libc.

I see one of two possibilities:

a.  define these functions as "libc-like" as possible, and use them as 
if they were
      regular standard libc functions

     or

b.  change up these functions to better fit their use cases in 
pc-bios/s390-ccw

Does that make sense?  What do you think?


>
>   Thomas
>
-- 
- Collin L Walling