[bug report] selftest/net/xfrm: Add test for ipsec tunnel

[bug report] selftest/net/xfrm: Add test for ipsec tunnel

[Dan Carpenter]

Hello Dmitry Safonov,

The patch bc2652b7ae1e: "selftest/net/xfrm: Add test for ipsec
tunnel" from Sep 21, 2020, leads to the following Smatch static
checker warning:

	tools/testing/selftests/net/ipsec.c:2294 main()
	warn: impossible condition '(nr_process == 9223372036854775807) => (0-4294967295 == s64max)'

tools/testing/selftests/net/ipsec.c
    2278 int main(int argc, char **argv)
    2279 {
    2280         unsigned int nr_process = 1;
    2281         int route_sock = -1, ret = KSFT_SKIP;
    2282         int test_desc_fd[2];
    2283         uint32_t route_seq;
    2284         unsigned int i;
    2285 
    2286         if (argc > 2)
    2287                 exit_usage(argv);
    2288 
    2289         if (argc > 1) {
    2290                 char *endptr;
    2291 
    2292                 errno = 0;
    2293                 nr_process = strtol(argv[1], &endptr, 10);
--> 2294                 if ((errno == ERANGE && (nr_process == LONG_MAX || nr_process == LONG_MIN))

nr_process is a u32 so it can't be LONG_MIN/MAX.  Do we even need to test
this or could we just fall through to the the > MAX_PROCESSES warning?

    2295                                 || (errno != 0 && nr_process == 0)
    2296                                 || (endptr == argv[1]) || (*endptr != '\0')) {
    2297                         printk("Failed to parse [nr_process]");
    2298                         exit_usage(argv);
    2299                 }
    2300 
    2301                 if (nr_process > MAX_PROCESSES || !nr_process) {
    2302                         printk("nr_process should be between [1; %u]",
    2303                                         MAX_PROCESSES);
    2304                         exit_usage(argv);
    2305                 }
    2306         }
    2307 

regards,
dan carpenter

Re: [bug report] selftest/net/xfrm: Add test for ipsec tunnel

[Dmitry Safonov]

On 7/19/22 14:44, Dan Carpenter wrote:
> Hello Dmitry Safonov,
> 
> The patch bc2652b7ae1e: "selftest/net/xfrm: Add test for ipsec
> tunnel" from Sep 21, 2020, leads to the following Smatch static
> checker warning:
> 
> 	tools/testing/selftests/net/ipsec.c:2294 main()
> 	warn: impossible condition '(nr_process == 9223372036854775807) => (0-4294967295 == s64max)'
> 
> tools/testing/selftests/net/ipsec.c
>     2278 int main(int argc, char **argv)
>     2279 {
>     2280         unsigned int nr_process = 1;
>     2281         int route_sock = -1, ret = KSFT_SKIP;
>     2282         int test_desc_fd[2];
>     2283         uint32_t route_seq;
>     2284         unsigned int i;
>     2285 
>     2286         if (argc > 2)
>     2287                 exit_usage(argv);
>     2288 
>     2289         if (argc > 1) {
>     2290                 char *endptr;
>     2291 
>     2292                 errno = 0;
>     2293                 nr_process = strtol(argv[1], &endptr, 10);
> --> 2294                 if ((errno == ERANGE && (nr_process == LONG_MAX || nr_process == LONG_MIN))
> 
> nr_process is a u32 so it can't be LONG_MIN/MAX.  Do we even need to test
> this or could we just fall through to the the > MAX_PROCESSES warning?

I would think it's still needed to check parsing underflow/overflow.
But comparing u32 to LONG_MIN/LONG_MAX clearly doesn't make sense.
I think, I did it as the man strtol told, without thinking twice about
the type:

> The  strtol()  function returns the result of the conversion, unless
> the value would underflow or overflow. If an  underflow occurs,
> strtol() returns LONG_MIN. If an overflow occurs,  strtol() returns
> LONG_MAX. In both cases, errno is set to ERANGE.

I think the check of errno == ERANGE can stand without checks for
LONG_MIN/LONG_MAX.

> 
>     2295                                 || (errno != 0 && nr_process == 0)
>     2296                                 || (endptr == argv[1]) || (*endptr != '\0')) {
>     2297                         printk("Failed to parse [nr_process]");
>     2298                         exit_usage(argv);
>     2299                 }
>     2300 
>     2301                 if (nr_process > MAX_PROCESSES || !nr_process) {
>     2302                         printk("nr_process should be between [1; %u]",
>     2303                                         MAX_PROCESSES);
>     2304                         exit_usage(argv);
>     2305                 }
>     2306         }
>     2307 
> 
> regards,
> dan carpenter


Thanks,
         Dmitry