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
