| Message ID | 20240704102617.1132337-2-hadess@hadess.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix a number of static analysis issues #5 | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | fail | WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 7: B1 Line exceeds max length (111>80): "bluez-5.76/src/shared/gatt-server.c:1121:2: zero_return: Function call "bt_att_get_mtu(server->att)" returns 0." 8: B1 Line exceeds max length (146>80): "bluez-5.76/src/shared/gatt-server.c:1121:2: assignment: Assigning: "data->mtu" = "bt_att_get_mtu(server->att)". The value of "data->mtu" is now 0." 9: B1 Line exceeds max length (152>80): "bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning: "__n" = "(size_t)(data->mtu - 1UL)". The value of "__n" is now 18446744073709551615." 10: B1 Line exceeds max length (82>80): "bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning: "__s" = "1UL"." 11: B1 Line exceeds max length (348>80): "bluez-5.76/src/shared/gatt-server.c:1123:19: overrun-buffer-arg: Calling "memset" with "__p" and "__n * __s" is suspicious because of the very large index, 18446744073709551615. The index may be due to a negative parameter being interpreted as unsigned. [Note: The source code implementation of the function has been overridden by a builtin model.]" 12: B3 Line contains hard tab characters (\t): "1121| data->mtu = bt_att_get_mtu(server->att);" 13: B3 Line contains hard tab characters (\t): "1122| data->length = 0;" 14: B3 Line contains hard tab characters (\t): "1123|-> data->rsp_data = new0(uint8_t, data->mtu - 1);" 16: B3 Line contains hard tab characters (\t): "1125| return data;" |
| tedd_an/BuildEll | success | Build ELL PASS |
| tedd_an/BluezMake | success | Bluez Make PASS |
| tedd_an/MakeCheck | success | Bluez Make Check PASS |
| tedd_an/MakeDistcheck | success | Make Distcheck PASS |
| tedd_an/CheckValgrind | success | Check Valgrind PASS |
| tedd_an/CheckSmatch | warning | CheckSparse WARNING src/shared/gatt-server.c:278:25: warning: Variable length array is used.src/shared/gatt-server.c:621:25: warning: Variable length array is used.src/shared/gatt-server.c:720:25: warning: Variable length array is used.src/shared/gatt-server.c:278:25: warning: Variable length array is used.src/shared/gatt-server.c:621:25: warning: Variable length array is used.src/shared/gatt-server.c:720:25: warning: Variable length array is used.src/shared/gatt-server.c:278:25: warning: Variable length array is used.src/shared/gatt-server.c:621:25: warning: Variable length array is used.src/shared/gatt-server.c:720:25: warning: Variable length array is used.src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):emulator/bthost.c:613:28: warning: Variable length array is used.emulator/bthost.c:787:28: warning: Variable length array is used.src/shared/gatt-server.c:278:25: warning: Variable length array is used.src/shared/gatt-server.c:621:25: warning: Variable length array is used.src/shared/gatt-server.c:720:25: warning: Variable length array is used.src/shared/gatt-server.c:278:25: warning: Variable length array is used.src/shared/gatt-server.c:621:25: warning: Variable length array is used.src/shared/gatt-server.c:720:25: warning: Variable length array is used.src/shared/gatt-server.c:278:25: warning: Variable length array is used.src/shared/gatt-server.c:621:25: warning: Variable length array is used.src/shared/gatt-server.c:720:25: warning: Variable length array is used. |
| tedd_an/bluezmakeextell | success | Make External ELL PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
Hi, to, 2024-07-04 kello 12:24 +0200, Bastien Nocera kirjoitti: > Set a lower-bound to the data MTU to avoid allocating -1 elements if > bt_att_get_mtu() returns zero. > > Error: OVERRUN (CWE-119): [#def36] [important] > bluez-5.76/src/shared/gatt-server.c:1121:2: zero_return: Function call "bt_att_get_mtu(server->att)" returns 0. > bluez-5.76/src/shared/gatt-server.c:1121:2: assignment: Assigning: "data->mtu" = "bt_att_get_mtu(server->att)". The value of "data->mtu" is now 0. > bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning: "__n" = "(size_t)(data->mtu - 1UL)". The value of "__n" is now 18446744073709551615. > bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning: "__s" = "1UL". > bluez-5.76/src/shared/gatt-server.c:1123:19: overrun-buffer-arg: Calling "memset" with "__p" and "__n * __s" is suspicious because of the very large index, 18446744073709551615. The index may be due to a negative parameter being interpreted as unsigned. [Note: The source code implementation of the function has been overridden by a builtin model.] > 1121| data->mtu = bt_att_get_mtu(server->att); > 1122| data->length = 0; > 1123|-> data->rsp_data = new0(uint8_t, data->mtu - 1); > 1124| > 1125| return data; > --- > src/shared/gatt-server.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c > index 3a53d5dfde6b..c587553d655d 100644 > --- a/src/shared/gatt-server.c > +++ b/src/shared/gatt-server.c > @@ -1118,7 +1118,7 @@ static struct read_mult_data *read_mult_data_new(struct bt_gatt_server *server, > data->server = server; > data->num_handles = num_handles; > data->cur_handle = 0; > - data->mtu = bt_att_get_mtu(server->att); > + data->mtu = MAX(bt_att_get_mtu(server->att), BT_ATT_DEFAULT_LE_MTU); Is this correct, probably MTU less than default are valid? > data->length = 0; > data->rsp_data = new0(uint8_t, data->mtu - 1); > Might be better to instead: MAX(data->mtu, 1) - 1
On Thu, 2024-07-04 at 15:25 +0300, Pauli Virtanen wrote: > Hi, > > to, 2024-07-04 kello 12:24 +0200, Bastien Nocera kirjoitti: > > Set a lower-bound to the data MTU to avoid allocating -1 elements > > if > > bt_att_get_mtu() returns zero. > > > > Error: OVERRUN (CWE-119): [#def36] [important] > > bluez-5.76/src/shared/gatt-server.c:1121:2: zero_return: Function > > call "bt_att_get_mtu(server->att)" returns 0. > > bluez-5.76/src/shared/gatt-server.c:1121:2: assignment: Assigning: > > "data->mtu" = "bt_att_get_mtu(server->att)". The value of "data- > > >mtu" is now 0. > > bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning: > > "__n" = "(size_t)(data->mtu - 1UL)". The value of "__n" is now > > 18446744073709551615. > > bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning: > > "__s" = "1UL". > > bluez-5.76/src/shared/gatt-server.c:1123:19: overrun-buffer-arg: > > Calling "memset" with "__p" and "__n * __s" is suspicious because > > of the very large index, 18446744073709551615. The index may be due > > to a negative parameter being interpreted as unsigned. [Note: The > > source code implementation of the function has been overridden by a > > builtin model.] > > 1121| data->mtu = bt_att_get_mtu(server->att); > > 1122| data->length = 0; > > 1123|-> data->rsp_data = new0(uint8_t, data->mtu - 1); > > 1124| > > 1125| return data; > > --- > > src/shared/gatt-server.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c > > index 3a53d5dfde6b..c587553d655d 100644 > > --- a/src/shared/gatt-server.c > > +++ b/src/shared/gatt-server.c > > @@ -1118,7 +1118,7 @@ static struct read_mult_data > > *read_mult_data_new(struct bt_gatt_server *server, > > data->server = server; > > data->num_handles = num_handles; > > data->cur_handle = 0; > > - data->mtu = bt_att_get_mtu(server->att); > > + data->mtu = MAX(bt_att_get_mtu(server->att), > > BT_ATT_DEFAULT_LE_MTU); > > Is this correct, probably MTU less than default are valid? This is the same code as in bt_gatt_server_new(). > > > data->length = 0; > > data->rsp_data = new0(uint8_t, data->mtu - 1); > > > > Might be better to instead: MAX(data->mtu, 1) - 1 I'd be fine with either, if somebody knows that particular part of the code better than I do...
diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c index 3a53d5dfde6b..c587553d655d 100644 --- a/src/shared/gatt-server.c +++ b/src/shared/gatt-server.c @@ -1118,7 +1118,7 @@ static struct read_mult_data *read_mult_data_new(struct bt_gatt_server *server, data->server = server; data->num_handles = num_handles; data->cur_handle = 0; - data->mtu = bt_att_get_mtu(server->att); + data->mtu = MAX(bt_att_get_mtu(server->att), BT_ATT_DEFAULT_LE_MTU); data->length = 0; data->rsp_data = new0(uint8_t, data->mtu - 1);