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

[Collin L. Walling]

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;
}

And as a side note: we know that the number of boot table entries for 
both ECKD
DASD andSCSI cannot exceed 31, so we should be safe with a rather small 
value
for our char arrays.

--
- Collin L Walling