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