On Wed, Apr 04, 2018 at 01:30:52PM +0200, Theo Buehler wrote:
> > +/* convert uint64_t to base N string */
> > 
> >  char *
> > -ulton(long unsigned int n, int base)
> > +u64ton(uint64_t n, int base)
> >  {
> >     char *p;
> >     static char buf [20];
> 
> I don't think it's actually an issue since I don't see how n can ever be
> that large, but 20 is too small to hold a NUL-terminated base 10 version
> of UINT64_MAX, as 18446744073709551615 has 20 digits. Thus,
> u64ton(UINT64_MAX, 10) will write to and return buf[-1].
> 
> With bases < 10 you'll have trouble with buf[21] (base 8 will need
> buf[23] and base 2 will need buf[65]).
> 
> Since all three callers of u64ton() use base 10 anyway, doing buf[21]
> would be enough for now. I wonder if we should get rid of the base
> argument and simplify accordingly (that would be a separate diff).

I fully agree with your spottings. It's actually worse than that.
Take a look at tree.c:

        p = ulton((neg) ? -n : n, 10);
        if (neg)
                *--p = '-';

It means that we would need buf[22]. But I don't want to fix it in this
patch, because this issue is not related to a switch from 32 to 64 bits
but an already existing issue (and it's theoretical, it would take
an int of size 64 bit if you look above that code snippet).

What I want to do with another patch:

- Remove the "n" argument, it's always 10
- Instead, add a "char prefix" argument which could be '-' for negative
  numbers. Nobody should ever change a returned pointer like that.
- Fix the comment above the function

But this is just an issue like the other existing integer overflows
which have already been fixed in test and expr. I want to fix them in
the same way in ksh, thus a unification of the code bases should be
first priority.


Tobias

Reply via email to