| Message ID | 20240702134106.102024-1-r.smirnov@omp.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [BlueZ,v1] gatt: add return value check of io_get_fd() to sock_io_send() | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| 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 | success | CheckSparse PASS |
| tedd_an/bluezmakeextell | success | Make External ELL PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
| tedd_an/ScanBuild | success | Scan Build PASS |
Hi Roman, On Tue, Jul 2, 2024 at 9:41 AM Roman Smirnov <r.smirnov@omp.ru> wrote: > > It is necessary to add a return value check. > > Found with the SVACE static analysis tool. > --- > src/gatt-database.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/src/gatt-database.c b/src/gatt-database.c > index 5756eb9d1..99aa6b63a 100644 > --- a/src/gatt-database.c > +++ b/src/gatt-database.c > @@ -2625,6 +2625,7 @@ static int sock_io_send(struct io *io, const void *data, size_t len) > { > struct msghdr msg; > struct iovec iov; > + int fd; > > iov.iov_base = (void *) data; > iov.iov_len = len; > @@ -2633,7 +2634,13 @@ static int sock_io_send(struct io *io, const void *data, size_t len) > msg.msg_iov = &iov; > msg.msg_iovlen = 1; > > - return sendmsg(io_get_fd(io), &msg, MSG_NOSIGNAL); > + fd = io_get_fd(io); > + if (fd < 0) { > + error("io_get_fd() returned %d\n", fd); > + return fd; > + } > + > + return sendmsg(fd, &msg, MSG_NOSIGNAL); > } So static analyzers are complaining that we pass a negative fd to the likes of sendmsg? I assume that it was safe to pass it this way since the sendmsg would check that fd is valid and return an error, anyway it is valid point that if we catch it earlier than we can print a specific error rather then depend on sendmsg return, just wondering what is the static analyzer trying to do with respect to checking the values passed to syscalls. > static void att_disconnect_cb(int err, void *user_data) > -- > 2.34.1 > >
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=867565 ---Test result--- Test Summary: CheckPatch PASS 0.51 seconds GitLint PASS 0.36 seconds BuildEll PASS 25.58 seconds BluezMake PASS 1814.43 seconds MakeCheck PASS 12.99 seconds MakeDistcheck PASS 184.08 seconds CheckValgrind PASS 261.93 seconds CheckSmatch PASS 361.97 seconds bluezmakeextell PASS 126.68 seconds IncrementalBuild PASS 1686.86 seconds ScanBuild PASS 1056.50 seconds --- Regards, Linux Bluetooth
On Tue, 2024-07-02 at 11:00 -0400, Luiz Augusto von Dentz wrote: > Hi Roman, > > On Tue, Jul 2, 2024 at 9:41 AM Roman Smirnov <r.smirnov@omp.ru> wrote: > > > > It is necessary to add a return value check. > > > > Found with the SVACE static analysis tool. > > --- > > src/gatt-database.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/src/gatt-database.c b/src/gatt-database.c > > index 5756eb9d1..99aa6b63a 100644 > > --- a/src/gatt-database.c > > +++ b/src/gatt-database.c > > @@ -2625,6 +2625,7 @@ static int sock_io_send(struct io *io, const void *data, size_t len) > > { > > struct msghdr msg; > > struct iovec iov; > > + int fd; > > > > iov.iov_base = (void *) data; > > iov.iov_len = len; > > @@ -2633,7 +2634,13 @@ static int sock_io_send(struct io *io, const void *data, size_t len) > > msg.msg_iov = &iov; > > msg.msg_iovlen = 1; > > > > - return sendmsg(io_get_fd(io), &msg, MSG_NOSIGNAL); > > + fd = io_get_fd(io); > > + if (fd < 0) { > > + error("io_get_fd() returned %d\n", fd); > > + return fd; > > + } > > + > > + return sendmsg(fd, &msg, MSG_NOSIGNAL); > > } > > So static analyzers are complaining that we pass a negative fd to the > likes of sendmsg? I assume that it was safe to pass it this way since > the sendmsg would check that fd is valid and return an error, anyway > it is valid point that if we catch it earlier than we can print a > specific error rather then depend on sendmsg return, just wondering > what is the static analyzer trying to do with respect to checking the > values passed to syscalls. From the analyzer's point of view, the problem here is not in passing a negative value. The problem is that io_get_fd() may return an error code but the function does not handle this case. This is what the analyzer writes: "Variable 'return value of io_get_fd(...)', which might receive a negative value at io-glib.c:127 by calling function 'io_get_fd' at gatt-database.c:2635, is used without checking at gatt-database.c:2635 by calling function 'sendmsg'." Is it worth sending patches for similar errors? > > > static void att_disconnect_cb(int err, void *user_data) > > -- > > 2.34.1 > > > > > >
diff --git a/src/gatt-database.c b/src/gatt-database.c index 5756eb9d1..99aa6b63a 100644 --- a/src/gatt-database.c +++ b/src/gatt-database.c @@ -2625,6 +2625,7 @@ static int sock_io_send(struct io *io, const void *data, size_t len) { struct msghdr msg; struct iovec iov; + int fd; iov.iov_base = (void *) data; iov.iov_len = len; @@ -2633,7 +2634,13 @@ static int sock_io_send(struct io *io, const void *data, size_t len) msg.msg_iov = &iov; msg.msg_iovlen = 1; - return sendmsg(io_get_fd(io), &msg, MSG_NOSIGNAL); + fd = io_get_fd(io); + if (fd < 0) { + error("io_get_fd() returned %d\n", fd); + return fd; + } + + return sendmsg(fd, &msg, MSG_NOSIGNAL); } static void att_disconnect_cb(int err, void *user_data)