On Wed, Apr 04, 2018 at 07:18:43PM +0200, Tobias Stoeckmann wrote:
> 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 = '-';

ouch.

> 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).

Agreed.

> 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

Sounds good to me.

> 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.

Yes. Go ahead. I'm ok with the patch as it is.

Reply via email to