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

[Thomas Huth]

On 19.12.2017 21:23, Collin L. Walling wrote:
> On 12/19/2017 11:29 AM, Collin L. Walling wrote:
>> On 12/19/2017 02:31 AM, Thomas Huth wrote:
>>> 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
>>>
>>>
>>
>> Sounds good to me.
>>
> 
> 
> What if we made a wrapper function for itostr. This func will have a tmp
> variable
> that stores the return of itostr. We then do an assertion to make sure
> we did not
> return 0 (which indicates that the size of the array was not large
> enough). If we
> pass, then just return tmp.
> 
> e.g.
> 
> static char *_itostr(int num, char *str, size_t len)
> {
>     ...
> }
> 
> char *itostr(int num, char *str, size_t len)
> {
>     char *tmp = _itostr(num, str, len);
>     IPL_assert(tmp != 0, "array too small for itostr conversion");
>     return tmp;
> }

That's fine for me, too.

 Thomas