Re: rev 244030 route command is not working

On Tue, Dec 11, 2012 at 04:07:03PM +0400, Gleb Smirnoff wrote:
> On Tue, Dec 11, 2012 at 12:21:20PM +0200, Artyom Mirgorodskiy wrote:
> A> Gleb, when I reset errno at the begin of fiboptlist_csv()
> A> everything work as expected.

> Artyom,

> can you please test attached patch?

> Index: route.c
> ===================================================================
> --- route.c	(revision 244082)
> +++ route.c	(working copy)
> _at__at_ -271,8 +271,7 _at__at_
>  		case 0:
>  		case 1:
>  			fib[i] = strtol(token, &endptr, 0);
> -			if (*endptr != '\0' || (fib[i] == 0 &&
> -			    (errno == EINVAL || errno == ERANGE)))
> +			if (*endptr != '\0')
>  				error = 1;
>  			break;
>  		default:
> _at__at_ -336,8 +335,7 _at__at_
>  				goto fiboptlist_csv_ret;
>  		} else {
>  			fib = strtol(token, &endptr, 0);
> -			if (*endptr != '\0' || (fib == 0 &&
> -			    (errno == EINVAL || errno == ERANGE))) {
> +			if (*endptr != '\0') {
>  				error = 1;
>  				goto fiboptlist_csv_ret;
>  			}

This patch will avoid erroneously failing but will let through certain
invalid strings. The empty string will silently be treated as 0 and
values outside the range [LONG_MIN, LONG_MAX] will be clamped silently
(the latter was already broken in most cases because [ERANGE] happens
only with a return value of LONG_MIN or LONG_MAX).

Other invalid strings that were and are let through (with unexpected
results) are ones containing a number that fits in a long but not in an
int.

To fix the range detection, errno should be set to 0 before the call and
checked afterwards; in a library function (this is an application),
errno should be saved and restored around that to avoid setting errno to
0 as a side effect of the function. The empty string needs a specific
check.

I don't insist on this being fixed but it shows that strtol() is too
hard to use correctly. The non-standard strtonum() looks easier but has
other problems (such as returning an English string in an API that
should be language-neutral).

-- 
Jilles Tjoelker