[ofa-general] snprintf() usage bug [was: Re: [PATCH v3] opensm/osm_qos_policy.c: log matched QoS criteria]

[Sasha Khapyorsky]

Hi Yevgeny,

On 16:11 Mon 16 Jun     , Yevgeny Kliteynik wrote:
> @@ -621,6 +627,14 @@ static osm_qos_match_rule_t *__qos_policy_get_match_rule_by_params(
>  				list_iterator = cl_list_next(list_iterator);
>  				continue;
>  			}
> +
> +			if (n)
> +				n += snprintf(buff + n, sizeof(buff) - n,
> +					      ", ");
> +			n += snprintf(buff + n, sizeof(buff) - n,
> +				      "S_Port (0x%016" PRIx64 ")",
> +				      cl_ntoh64(osm_physp_get_port_guid(
> +					      p_src_physp)));

I tested this patch (from performance impacts perspective) and found that
such usage of snprintf() is just wrong and leads to buffer overflow.

snprintf man page ('Return value' section) states:

The functions snprintf() and vsnprintf() do not write  more than size
bytes (including the trailing '\0'). If the output was truncated due
to this limit then the return value is the number of characters (not
including the trailing '\0') which would have been written to the final
string if enough space had been available. Thus, a return value of
size or more means that the output was truncated.

This effectively invalidates repeated usage of things like:

	n += snprintf(buf + n, sizeof(buf) - n, ...)

, when it is done without actual 'n' value checks.

And ugh, I guess we have a lot of such in current management tree. :(

Sasha