[PATCH iproute2] ip: Fix size_columns() invocation that passes a 32-bit quantity

[PATCH iproute2] ip: Fix size_columns() invocation that passes a 32-bit quantity

[Petr Machata]

In print_stats64(), the last size_columns() invocation passes number of
carrier changes as one of the arguments. The value is decoded as a 32-bit
quantity, but size_columns() expects a 64-bit one. This is undefined
behavior.

The reason valgrind does not cite this is that the previous size_columns()
invocations prime the ABI area used for the value transfer. When these
other invocations are commented away, valgrind does complain that
"conditional jump or move depends on uninitialised value", as would be
expected.

Fixes: 49437375b6c1 ("ip: dynamically size columns when printing stats")
Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 ip/ipaddress.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 5a3b1cae..8cd76073 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -788,8 +788,9 @@ void print_stats64(FILE *fp, struct rtnl_link_stats64 *s,
 				     s->tx_aborted_errors, s->tx_fifo_errors,
 				     s->tx_window_errors,
 				     s->tx_heartbeat_errors,
-				     carrier_changes ?
-				     rta_getattr_u32(carrier_changes) : 0);
+				     (uint64_t)(carrier_changes ?
+						rta_getattr_u32(carrier_changes)
+						: 0));
 
 		/* RX stats */
 		fprintf(fp, "    RX: %*s %*s %*s %*s %*s %*s %*s%s",
-- 
2.35.3

Re: [PATCH iproute2] ip: Fix size_columns() invocation that passes a 32-bit quantity

[Stephen Hemminger]

On Mon, 27 Jun 2022 15:20:01 +0200
Petr Machata <petrm@nvidia.com> wrote:

> In print_stats64(), the last size_columns() invocation passes number of
> carrier changes as one of the arguments. The value is decoded as a 32-bit
> quantity, but size_columns() expects a 64-bit one. This is undefined
> behavior.
> 
> The reason valgrind does not cite this is that the previous size_columns()
> invocations prime the ABI area used for the value transfer. When these
> other invocations are commented away, valgrind does complain that
> "conditional jump or move depends on uninitialised value", as would be
> expected.
> 
> Fixes: 49437375b6c1 ("ip: dynamically size columns when printing stats")
> Signed-off-by: Petr Machata <petrm@nvidia.com>
> ---
>  ip/ipaddress.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index 5a3b1cae..8cd76073 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -788,8 +788,9 @@ void print_stats64(FILE *fp, struct rtnl_link_stats64 *s,
>  				     s->tx_aborted_errors, s->tx_fifo_errors,
>  				     s->tx_window_errors,
>  				     s->tx_heartbeat_errors,
> -				     carrier_changes ?
> -				     rta_getattr_u32(carrier_changes) : 0);
> +				     (uint64_t)(carrier_changes ?
> +						rta_getattr_u32(carrier_changes)
> +						: 0));

Looks good, but would be clearer with a local temporary variable
which would eliminate the cast etc.

Re: [PATCH iproute2] ip: Fix size_columns() invocation that passes a 32-bit quantity

[Petr Machata]

Stephen Hemminger <stephen@networkplumber.org> writes:

>> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
>> index 5a3b1cae..8cd76073 100644
>> --- a/ip/ipaddress.c
>> +++ b/ip/ipaddress.c
>> @@ -788,8 +788,9 @@ void print_stats64(FILE *fp, struct rtnl_link_stats64 *s,
>>  				     s->tx_aborted_errors, s->tx_fifo_errors,
>>  				     s->tx_window_errors,
>>  				     s->tx_heartbeat_errors,
>> -				     carrier_changes ?
>> -				     rta_getattr_u32(carrier_changes) : 0);
>> +				     (uint64_t)(carrier_changes ?
>> +						rta_getattr_u32(carrier_changes)
>> +						: 0));
>
> Looks good, but would be clearer with a local temporary variable
> which would eliminate the cast etc.

OK, I have a v2 queued up. I'll wait a bit before sending for possible
comments on the rx_otherhost_dropped patch, as it depends on this one,
and will need to be resent for this change anyway.