Stefano Brivio (3):
tcp, tcp_splice: Get rid of false positive CWE-394 Coverity warning
from fls()
tcp: Avoid false (but convoluted) positive Coverity CWE-476 warning
tcp: Avoid (theoretical) resource leak (CWE-772) Coverity warning
tcp.c | 17 ++++++++++++-----
tcp_splice.c | 24 ++++++++++++++++--------
2 files changed, 28 insertions(+), 13 deletions(-)
--
2.39.1
We use the return value of fls() as array index for debug strings.
While fls() can return -1 (if no bit is set), Coverity Scan doesn't
see that we're first checking the return value of another fls() call
with the same bitmask, before using it.
Call fls() once, store its return value, check it, and use the stored
value as array index.
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
tcp.c | 12 ++++++++----
tcp_splice.c | 24 ++++++++++++++++--------
2 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/tcp.c b/tcp.c
index cbd537e..41210a3 100644
--- a/tcp.c
+++ b/tcp.c
@@ -743,15 +743,19 @@ static void conn_flag_do(const struct ctx *c, struct tcp_tap_conn
*conn,
unsigned long flag)
{
if (flag & (flag - 1)) {
+ int flag_index = fls(~flag);
+
if (!(conn->flags & ~flag))
return;
conn->flags &= flag;
- if (fls(~flag) >= 0) {
+ if (flag_index >= 0) {
debug("TCP: index %li: %s dropped", CONN_IDX(conn),
- tcp_flag_str[fls(~flag)]);
+ tcp_flag_str[flag_index]);
}
} else {
+ int flag_index = fls(~flag);
+
if (conn->flags & flag) {
/* Special case: setting ACK_FROM_TAP_DUE on a
* connection where it's already set is used to
@@ -766,9 +770,9 @@ static void conn_flag_do(const struct ctx *c, struct tcp_tap_conn
*conn,
}
conn->flags |= flag;
- if (fls(flag) >= 0) {
+ if (flag_index >= 0) {
debug("TCP: index %li: %s", CONN_IDX(conn),
- tcp_flag_str[fls(flag)]);
+ tcp_flag_str[flag_index]);
}
}
diff --git a/tcp_splice.c b/tcp_splice.c
index 84f855e..67af46b 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -127,22 +127,26 @@ static void conn_flag_do(const struct ctx *c, struct tcp_splice_conn
*conn,
unsigned long flag)
{
if (flag & (flag - 1)) {
+ int flag_index = fls(~flag);
+
if (!(conn->flags & ~flag))
return;
conn->flags &= flag;
- if (fls(~flag) >= 0) {
+ if (flag_index >= 0) {
debug("TCP (spliced): index %li: %s dropped", CONN_IDX(conn),
- tcp_splice_flag_str[fls(~flag)]);
+ tcp_splice_flag_str[flag_index]);
}
} else {
+ int flag_index = fls(flag);
+
if (conn->flags & flag)
return;
conn->flags |= flag;
- if (fls(flag) >= 0) {
+ if (flag_index >= 0) {
debug("TCP (spliced): index %li: %s", CONN_IDX(conn),
- tcp_splice_flag_str[fls(flag)]);
+ tcp_splice_flag_str[flag_index]);
}
}
@@ -207,22 +211,26 @@ static void conn_event_do(const struct ctx *c, struct
tcp_splice_conn *conn,
unsigned long event)
{
if (event & (event - 1)) {
+ int flag_index = fls(~event);
+
if (!(conn->events & ~event))
return;
conn->events &= event;
- if (fls(~event) >= 0) {
+ if (flag_index >= 0) {
debug("TCP (spliced): index %li, ~%s", CONN_IDX(conn),
- tcp_splice_event_str[fls(~event)]);
+ tcp_splice_event_str[flag_index]);
}
} else {
+ int flag_index = fls(event);
+
if (conn->events & event)
return;
conn->events |= event;
- if (fls(event) >= 0) {
+ if (flag_index >= 0) {
debug("TCP (spliced): index %li, %s", CONN_IDX(conn),
- tcp_splice_event_str[fls(event)]);
+ tcp_splice_event_str[flag_index]);
}
}
--
2.39.1
On Mon, Feb 27, 2023 at 10:59:39AM +0100, Stefano Brivio wrote:We use the return value of fls() as array index for debug strings. While fls() can return -1 (if no bit is set), Coverity Scan doesn't see that we're first checking the return value of another fls() call with the same bitmask, before using it. Call fls() once, store its return value, check it, and use the stored value as array index. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>--- tcp.c | 12 ++++++++---- tcp_splice.c | 24 ++++++++++++++++-------- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/tcp.c b/tcp.c index cbd537e..41210a3 100644 --- a/tcp.c +++ b/tcp.c @@ -743,15 +743,19 @@ static void conn_flag_do(const struct ctx *c, struct tcp_tap_conn *conn, unsigned long flag) { if (flag & (flag - 1)) { + int flag_index = fls(~flag); + if (!(conn->flags & ~flag)) return; conn->flags &= flag; - if (fls(~flag) >= 0) { + if (flag_index >= 0) { debug("TCP: index %li: %s dropped", CONN_IDX(conn), - tcp_flag_str[fls(~flag)]); + tcp_flag_str[flag_index]); } } else { + int flag_index = fls(~flag); + if (conn->flags & flag) { /* Special case: setting ACK_FROM_TAP_DUE on a * connection where it's already set is used to @@ -766,9 +770,9 @@ static void conn_flag_do(const struct ctx *c, struct tcp_tap_conn *conn, } conn->flags |= flag; - if (fls(flag) >= 0) { + if (flag_index >= 0) { debug("TCP: index %li: %s", CONN_IDX(conn), - tcp_flag_str[fls(flag)]); + tcp_flag_str[flag_index]); } } diff --git a/tcp_splice.c b/tcp_splice.c index 84f855e..67af46b 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -127,22 +127,26 @@ static void conn_flag_do(const struct ctx *c, struct tcp_splice_conn *conn, unsigned long flag) { if (flag & (flag - 1)) { + int flag_index = fls(~flag); + if (!(conn->flags & ~flag)) return; conn->flags &= flag; - if (fls(~flag) >= 0) { + if (flag_index >= 0) { debug("TCP (spliced): index %li: %s dropped", CONN_IDX(conn), - tcp_splice_flag_str[fls(~flag)]); + tcp_splice_flag_str[flag_index]); } } else { + int flag_index = fls(flag); + if (conn->flags & flag) return; conn->flags |= flag; - if (fls(flag) >= 0) { + if (flag_index >= 0) { debug("TCP (spliced): index %li: %s", CONN_IDX(conn), - tcp_splice_flag_str[fls(flag)]); + tcp_splice_flag_str[flag_index]); } } @@ -207,22 +211,26 @@ static void conn_event_do(const struct ctx *c, struct tcp_splice_conn *conn, unsigned long event) { if (event & (event - 1)) { + int flag_index = fls(~event); + if (!(conn->events & ~event)) return; conn->events &= event; - if (fls(~event) >= 0) { + if (flag_index >= 0) { debug("TCP (spliced): index %li, ~%s", CONN_IDX(conn), - tcp_splice_event_str[fls(~event)]); + tcp_splice_event_str[flag_index]); } } else { + int flag_index = fls(event); + if (conn->events & event) return; conn->events |= event; - if (fls(event) >= 0) { + if (flag_index >= 0) { debug("TCP (spliced): index %li, %s", CONN_IDX(conn), - tcp_splice_event_str[fls(event)]); + tcp_splice_event_str[flag_index]); } }-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
If there are no TCP options in the header, tcp_tap_handler() will
pass the corresponding pointer, fetched via packet_get(), as NULL to
tcp_conn_from_sock_finish(), which in turn indirectly calls
tcp_opt_get().
If there are no options, tcp_opt_get() will stop right away because
the option length is indicated as zero. However, if the logic is
complicated enough to follow for static checkers, adding an explicit
check against NULL in tcp_opt_get() is probably a good idea.
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
tcp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tcp.c b/tcp.c
index 41210a3..561064e 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1114,7 +1114,7 @@ static int tcp_opt_get(const char *opts, size_t len, uint8_t
type_find,
{
uint8_t type, optlen;
- if (!len)
+ if (!opts || !len)
return -1;
for (; len >= 2; opts += optlen, len -= optlen) {
--
2.39.1
On Mon, Feb 27, 2023 at 10:59:40AM +0100, Stefano Brivio wrote:If there are no TCP options in the header, tcp_tap_handler() will pass the corresponding pointer, fetched via packet_get(), as NULL to tcp_conn_from_sock_finish(), which in turn indirectly calls tcp_opt_get(). If there are no options, tcp_opt_get() will stop right away because the option length is indicated as zero. However, if the logic is complicated enough to follow for static checkers, adding an explicit check against NULL in tcp_opt_get() is probably a good idea. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>--- tcp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tcp.c b/tcp.c index 41210a3..561064e 100644 --- a/tcp.c +++ b/tcp.c @@ -1114,7 +1114,7 @@ static int tcp_opt_get(const char *opts, size_t len, uint8_t type_find, { uint8_t type, optlen; - if (!len) + if (!opts || !len) return -1; for (; len >= 2; opts += optlen, len -= optlen) {-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
If tcp_timer_ctl() gets a socket number greater than SOCKET_MAX
(2 ^ 24), we return error but we don't close the socket. This is a
rather formal issue given that, at least on Linux, socket numbers are
monotonic and we're in general not allowed to open so many sockets.
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
tcp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tcp.c b/tcp.c
index 561064e..b674311 100644
--- a/tcp.c
+++ b/tcp.c
@@ -702,6 +702,9 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn
*conn)
fd = timerfd_create(CLOCK_MONOTONIC, 0);
if (fd == -1 || fd > SOCKET_MAX) {
debug("TCP: failed to get timer: %s", strerror(errno));
+ if (fd > -1)
+ close(fd);
+ conn->timer = -1;
return;
}
conn->timer = fd;
--
2.39.1
On Mon, Feb 27, 2023 at 10:59:41AM +0100, Stefano Brivio wrote:If tcp_timer_ctl() gets a socket number greater than SOCKET_MAX (2 ^ 24), we return error but we don't close the socket. This is a rather formal issue given that, at least on Linux, socket numbers are monotonic and we're in general not allowed to open so many sockets. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>--- tcp.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tcp.c b/tcp.c index 561064e..b674311 100644 --- a/tcp.c +++ b/tcp.c @@ -702,6 +702,9 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn) fd = timerfd_create(CLOCK_MONOTONIC, 0); if (fd == -1 || fd > SOCKET_MAX) { debug("TCP: failed to get timer: %s", strerror(errno)); + if (fd > -1) + close(fd); + conn->timer = -1; return; } conn->timer = fd;-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson