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

[Lucas De Marchi]

On Fri, Mar 01, 2024 at 04:16:51PM +0100, Janusz Krzysztofik wrote:
>On Thursday, 29 February 2024 18:01:29 CET Lucas De Marchi wrote:
>> 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.
>
>So if there were no follow up patches in the series then you would agree to
>make this patch prettier, but in this case your choice is to leave it not
>quite pretty, right?

yes, probably as a second patch so we have 1) fix the bug; 2) make it
pretty.

>
>Up to you.
>
>Acked-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>

thanks
Lucas De Marchi

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