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

[David Gibson <david@gibson.dropbear.id.au>]

[-- 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 --]