[PATCH 0/3] Avoid some warnings reported by Coverity

[PATCH 0/3] Avoid some warnings reported by Coverity

[Stefano Brivio]

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

[PATCH 1/3] tcp, tcp_splice: Get rid of false positive CWE-394 Coverity warning from fls()

[Stefano Brivio]

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@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]);
 		}
 	}
 
-- 
@@ -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

Re: [PATCH 1/3] tcp, tcp_splice: Get rid of false positive CWE-394 Coverity warning from fls()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3898 bytes --]

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@redhat.com>

Reviewed-by: David Gibson <david@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/3] tcp: Avoid false (but convoluted) positive Coverity CWE-476 warning

[Stefano Brivio]

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@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) {
-- 
@@ -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

Re: [PATCH 2/3] tcp: Avoid false (but convoluted) positive Coverity CWE-476 warning

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]

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@redhat.com>

Reviewed-by: David Gibson <david@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/3] tcp: Avoid (theoretical) resource leak (CWE-772) Coverity warning

[Stefano Brivio]

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@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;
-- 
@@ -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

Re: [PATCH 3/3] tcp: Avoid (theoretical) resource leak (CWE-772) Coverity warning

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

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@redhat.com>

Reviewed-by: David Gibson <david@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]