https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85175
--- Comment #3 from Arnd Bergmann <arnd at linaro dot org> ---
(In reply to Martin Sebor from comment #2)
> So with the change above GCC is doing a better but imperfect job of
> determining the range. Changing the variable to unsigned constrains the
> lower bound to zero and eliminates the warning even at -Os.
Ok, so this is actually the same thing I saw with seven other files in the
kernel (out of several hundred randconfig kernel builds on three
architectures). In each case, we now have a signed integer range that appears
to be constrained on one side after r257857, e.g. [-2147483647, 68] or [1,
2147483647] based on a partial range check, but the warning is still a false
positive in the end.
The one such warning we gained in the kernel that makes sense was this one (not
sent yet):
commit c18e88d296264d76f1a242ae95a43681cf198078
Author: Arnd Bergmann <a...@arndb.de>
Date: Tue Apr 3 09:45:35 2018 +0200
bluetooth: fix hci name overflow
gcc-8 warns that the index of the hci device could overflow the eight
character array:
net/bluetooth/hci_core.c: In function 'hci_register_dev':
net/bluetooth/hci_core.c:3093:26: error: '%d' directive writing between 1
and 10 bytes into a region of size 5 [-Werror=format-overflow=]
sprintf(hdev->name, "hci%d", id);
^~
net/bluetooth/hci_core.c:3093:22: note: directive argument in the range [0,
2147483647]
sprintf(hdev->name, "hci%d", id);
^~~~~~~
net/bluetooth/hci_core.c:3093:2: note: 'sprintf' output between 5 and 14
bytes into a destination of size 8
sprintf(hdev->name, "hci%d", id);
This uses snprintf() to enforce a valid string, and limits the range of
the integer to 0..9999. In practice this should not matter as we would
not be able connect more than 9999 bluetooth hci's simultaneously.
Signed-off-by: Arnd Bergmann <a...@arndb.de>
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 40d260f2bea5..9e2ad444d799 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3075,13 +3075,14 @@ int hci_register_dev(struct hci_dev *hdev)
/* Do not allow HCI_AMP devices to register at index 0,
* so the index can be used as the AMP controller ID.
+ * Ensure the name fits into eight characters id < 10000.
*/
switch (hdev->dev_type) {
case HCI_PRIMARY:
- id = ida_simple_get(&hci_index_ida, 0, 0, GFP_KERNEL);
+ id = ida_simple_get(&hci_index_ida, 0, 10000, GFP_KERNEL);
break;
case HCI_AMP:
- id = ida_simple_get(&hci_index_ida, 1, 0, GFP_KERNEL);
+ id = ida_simple_get(&hci_index_ida, 1, 10000, GFP_KERNEL);
break;
default:
return -EINVAL;
@@ -3090,7 +3091,7 @@ int hci_register_dev(struct hci_dev *hdev)
if (id < 0)
return id;
- sprintf(hdev->name, "hci%d", id);
+ snprintf(hdev->name, sizeof(hdev->name), "hci%d", id);
hdev->id = id;
BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
The other new warnings are either the same kind as this one (the compiler
should be able to figure it out), or the sort where the compiler is technically
right about the string overflow based on the types, but we can easily prove
that the range is more limited like in the ida_simple_get() case with correct
limits (this is an extern function that returns an unused number within a
strict range).
Would it be sensible to only warn with -Wformat-overflow=1 on a signed integer
if the overflow happens with an actual known limit, but not if the limit is
euqal to the minimum/maximum representable numbers? The documentation for
-Wformat-overflow=2 isn't completely clear on what behavior was intended here
for the =1 and =2 case if the range is only bounded on one side.
