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

[Collin L. Walling]

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.

--
- Collin L Walling