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
