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 <wall...@linux.vnet.ibm.com>
>>>>>> Acked-by: Christian Borntraeger <borntrae...@de.ibm.com>
>>>>>> Reviewed-by: Janosch Frank <fran...@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?
>>> 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
