[Cocci] [PATCH] lib/test_kmod: Fix an integer overflow test

[Luis R. Rodriguez]

The main problem is that the parentheses are in the wrong place and the
unlikely() call returns either 0 or 1 so it's never less than zero.

Doh, thanks, yes. Seems worth considering a grammar rule for it.

The other problem is that signed integer overflows like "INT_MAX + 1" are
undefined behavior.

Likewise.

This seems like another possible generic typo issue. But I would not resolve it
the way you did, in this particular case below num_test_devs represents the
number of already registered devs, before we increment. So the way to resolve
this would be:

if (num_test_devs + 1 == INT_MAX)

I'll get this upstream, thanks!

Luis

Fixes: d9c6a72d6fa2 ("kmod: add test driver to stress test the module loader")
diff --git a/lib/test_kmod.c b/lib/test_kmod.c
index e372b97eee13..30fd6d9e5361 100644
--- a/lib/test_kmod.c
+++ b/lib/test_kmod.c
@@ -1141,7 +1141,7 @@ static struct kmod_test_device *register_test_dev_kmod(void)
mutex_lock(&reg_dev_mutex);
/* int should suffice for number of devices, test for wrap */
- if (unlikely(num_test_devs + 1) < 0) {
+ if (num_test_devs == INT_MAX) {
pr_err("reached limit of number of test devices\n");
goto out;
}

[Dan Carpenter]

Post by Luis R. Rodriguez

The main problem is that the parentheses are in the wrong place and the
unlikely() call returns either 0 or 1 so it's never less than zero.

Doh, thanks, yes. Seems worth considering a grammar rule for it.

The other problem is that signed integer overflows like "INT_MAX + 1" are
undefined behavior.

Likewise.
This seems like another possible generic typo issue. But I would not resolve it
the way you did, in this particular case below num_test_devs represents the
number of already registered devs, before we increment. So the way to resolve
if (num_test_devs + 1 == INT_MAX)
I'll get this upstream, thanks!

There is no issue if num_test_devs is INT_MAX. But capping it at
INT_MAX - 1 is also fine.

regards,
dan carpenter

[Luis R. Rodriguez]

Post by Dan Carpenter

Post by Luis R. Rodriguez

The main problem is that the parentheses are in the wrong place and the
unlikely() call returns either 0 or 1 so it's never less than zero.

Doh, thanks, yes. Seems worth considering a grammar rule for it.

The other problem is that signed integer overflows like "INT_MAX + 1" are
undefined behavior.

Likewise.
This seems like another possible generic typo issue. But I would not resolve it
the way you did, in this particular case below num_test_devs represents the
number of already registered devs, before we increment. So the way to resolve
if (num_test_devs + 1 == INT_MAX)
I'll get this upstream, thanks!

There is no issue if num_test_devs is INT_MAX. But capping it at
INT_MAX - 1 is also fine.

If num_test_devs is INT_MAX, then doing num_test_devs + 1 overflows
and as you noted that is undefined?

Luis

[Dan Carpenter]

Post by Luis R. Rodriguez

Post by Dan Carpenter

Post by Luis R. Rodriguez

The main problem is that the parentheses are in the wrong place and the
unlikely() call returns either 0 or 1 so it's never less than zero.

Doh, thanks, yes. Seems worth considering a grammar rule for it.

The other problem is that signed integer overflows like "INT_MAX + 1" are
undefined behavior.

Likewise.
This seems like another possible generic typo issue. But I would not resolve it
the way you did, in this particular case below num_test_devs represents the
number of already registered devs, before we increment. So the way to resolve
if (num_test_devs + 1 == INT_MAX)
I'll get this upstream, thanks!

There is no issue if num_test_devs is INT_MAX. But capping it at
INT_MAX - 1 is also fine.

If num_test_devs is INT_MAX, then doing num_test_devs + 1 overflows
and as you noted that is undefined?

If it's INT_MAX we never do "num_test_devs + 1", we return a NULL.

regards,
dan carpenter