Re: [PATCH] ifconfig: prefix length handling fixes for -A

[Erik Auerswald]

Hi Simon,

On Wed, Apr 24, 2024 at 02:55:41PM +0200, Simon Josefsson wrote:
> Erik Auerswald <auerswal@unix-ag.uni-kl.de> writes:
> >
> > while "ifconfig -A" now accepts CIDR notation, it does not reject
> > prefix length values outside of [0,32].  Also, with a prefix length
> > of 0, undefined begavior is invoked, and at least on x86_64 a wrong
> > netmask is computed.
> >
> > I think the attached patch fixes this.
> >
> > If it is OK, I can commit the changes.  What do you think?
> 
> Makes sense to me -- when using strtol() one ought to check for LONG_MIN
> and LONG_MAX too, but your comparison address that.
> 
>        The  strtol() function returns the result of the conversion,
>        unless the value would underflow or overflow.  If an  underflow
>        occurs,  strtol() returns  LONG_MIN.
> 
> Given that, I would re-order the if statement to compare "n" before
> comparing (stale) "end", although I suppose this is somewhat cosmetic.

This first check tests if strtol() found a leading digit, i.e., if the
value of "n" comes from the textual representation of a number.

    If there were no digits at all, strtol() stores the original value
    of nptr in *endptr (and returns 0).

The other two checks then verify that the number is in the valid range.
Thus I prefer this order of checks.

This still allows wrong input, e.g., "192.0.2.47/3k".  I'll consider
tightening the checks, possibly based on the following from the strtol(3)
man page:

    In  particular, if  *nptr is not '\0' but **endptr is '\0' on return,
    the entire string is valid.

This would still allow strange values like "192.0.2.47/ +24", but I do
not think it is worth the effort to detect this.

> Maybe add some other odd values in the self-test?  Like
> 1212237832782387238723823782 and -1238912x1298129.  Thanks for adding
> regression checks!

I can do that, no problem.

Thanks for the quick feedback! :-)

Br,
Erik