[PATCH i-g-t v4 3/4] lib/igt_sysfs: Fix off-by-one in buffer size

[Lucas De Marchi]

On Thu, Feb 29, 2024 at 12:07:01PM +0100, Janusz Krzysztofik wrote:
>Hi Lucas,
>
>On Wednesday, 28 February 2024 23:31:33 CET Lucas De Marchi wrote:
>> vsnprintf() should receive the buffer size as argument, here called `len`,
>> including the trailing '\0'. There was truncation if the return is "size
>> or more". In this second call to vsnprintf() the value should be exactly
>> the same as in the first call, otherwise something really unexpected
>> happened.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  lib/igt_sysfs.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
>> index 2997925e5..a1ff5655d 100644
>> --- a/lib/igt_sysfs.c
>> +++ b/lib/igt_sysfs.c
>> @@ -529,8 +529,8 @@ int igt_sysfs_vprintf(int dir, const char *attr, const
>char *fmt, va_list ap)
>>  			goto end;
>>  		}
>>
>> -		ret = vsnprintf(buf, ret, fmt, ap);
>> -		if (igt_debug_on(ret > len)) {
>> +		ret = vsnprintf(buf, len, fmt, ap);
>
>Oh, so I missed that we didn't use the len variable, initialized with a
>calculated value of required buffer length, when allocating that buffer --
>good catch.  OTOH, since we then pass the buffer to a function that doesn't
>care for a terminating null char, a buffer of ret length, with no room for
>that terminating null char, should be sufficient.  But anyway, let's request
>that extra byte so the code is less confusing.
>
>> +		if (igt_debug_on(ret != len - 1)) {
>
>OK, let's also take care of strict consistency of the result with that from
>the initial vsnprintf().
>
>But then, the len variable is really needed only for that comparison with the
>new result  The required size of the buffer doesn't need to be calculated from
>ret as ret + 1 in advance, only just when passing it as an argument to
>malloc().  Under such circumstances, wouldn't that be more clear if we changed
>semantics of len to always carry an initially detected length of the data to
>be printed, not the required buffer length, and then compare it directly with
>the new result, without recalculating that initial value back from the buffer
>length?

this patch only fixes the bug. Path 4 is already reworking this to make
it prettier, which afaiu includes this reasoning, with len always being
the length of the string, not mixed with buffer size.

Lucas De Marchi

>
>Thanks,
>Janusz
>
>>  			ret = -EINVAL;
>>  			goto free_buf;
>>  		}
>>
>
>
>
>