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

[Yevgeny Kliteynik]

Hi Sasha,

Sasha Khapyorsky wrote:
> 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. :(

Right, I noticed you already started taking care of it.
Anyway, as for this patch - I'll rework it not to have string
manipulations, so the fast path will remain fast.

Thanks.

-- Yevgeny

> Sasha
>