On Sat, Feb 28, 2004 at 11:59:23PM +0100, Christian Biere wrote:
> Jun-ichiro itojun Hagino wrote:
>  
> > 	more error check to strtoul().

Not really worth the effort.....

> Eh, is invalid input unimportant here? Sorry, I really don't see why
> anybody would use atoi(), atol() etc. unless you already know that
> the given parameter is a valid value within the correct range. These
> functions are *crap*, period.

True - but they are moderately ok when the input is controlled.
In this case the only read problem is that atoi() saturates at 2^31-1.

> > +	u_int32_t m;							\
> 
> Wouldn't it be better to use uintmax_t here?

The values are being written into fields defined as u_int32_t....

> 
> >  									\
> >  	_CHECKLINE							\
> >  	cp = tp, tp = word(cp);						\
> > -	m = (int)strtol(cp, &ptr, 10);					\
> > -	if (*ptr == '\0')						\
> > +	errno = 0;							\
> > +	m = strtoul(cp, &ptr, 10);					\
> > +	if (*cp && ptr && *ptr == '\0' && errno == 0)			\
> 
> unsigned long isn't necessarily an alias for u_int32_t and strtoul() happily
> accepts negative values.

true, but does it matter?
Fixing this program properly would require daddr_t in the disklabel.

> Hmm, if it's save to assume (errno == 0) in case of a successful strtoul()
> you might want to shorten the other checks to "errno" instead of
> "errno == ERANGE". Otherwise, I'd change this to "errno != ERANGE".

Or if you are willing to say '*cp && ptr', why not '!*ptr && !errno' ?

In any case the point of teh fix was to allow numbers between 2^31 and
2^32-1 to be input, not to fix the error checking.

	David

-- 
David Laight: david@l8s.co.uk