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

[Thomas Huth]

On 18.12.2017 17:16, Collin L. Walling wrote:
> 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 <address@hidden>
>>> Acked-by: Christian Borntraeger <address@hidden>
>>> Reviewed-by: Janosch Frank <address@hidden>
>>> ---
>>>   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?

Keeping them libc-like likely makes sense ... but could we somehow also
make sure that we're not running into unexpected errors when using them?
Something like "IPL_assert(entries < 1000, ...)" before calling the
functions in patch 5?

 Thomas