Re: [qemu-s390x] [PATCH v7 04/12] s390-ccw: update libc

[Thomas Huth]

On 19.02.2018 16:40, Collin L. Walling wrote:
> On 02/17/2018 02:48 AM, Thomas Huth wrote:
>> On 16.02.2018 23:07, Collin L. Walling wrote:
>> [...]
>>> +/**
>>> + * uitoa:
>>> + * @num: an integer (base 10) 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. This function only handles numbers between 0 and
>>> UINT64_MAX
>>> + * inclusive.
>>> + *
>>> + * Returns: the string @str of the converted integer @num
>>> + */
>>> +char *uitoa(uint64_t num, char *str, size_t len)
>>> +{
>>> +    size_t num_idx = 0;
>>> +    uint64_t tmp = num;
>>> +
>>> +    IPL_assert(str != NULL, "uitoa: no space allocated to store
>>> string");
>>> +
>>> +    /* Get index to ones place */
>>> +    while ((tmp /= 10) != 0) {
>>> +        num_idx++;
>>> +    }
>>> +
>>> +    /* Check if we have enough space for num and null */
>>> +    IPL_assert(len > num_idx, "uitoa: array too small for conversion");
>> Well, in v5 of this patch you've had "len >= num_idx + 1" where we
>> agreed that it was wrong. Now you have "len > num_idx" which is pretty
>> much the same. WTF?
>> I still think you need "len > num_idx + 1" here to properly take the
>> trailing NUL-byte into account properly. Please fix it!
>>
>>   Thomas
>>
> You're correct, and my apologies for not correcting the true problem here:
> I messed up the value of num_idx.  It is off by one, but initializing the
> value to 1 instead of 0 should fix this. I must've accounted for this in
> my test file but forgot to update it in the actual source code.

Are you sure that initializing it to 1 is right? Unless you also change
the final while loop in this function, this will put the character into
the wrong location ("str[num_idx] = num % 10 + '0';"). Imagine a number
that only consists of one digit ... the digit will be placed in str[1]
which sounds wrong to me...?

 Thomas