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 } 2307regards, dan carpenter
Thanks, Dmitry