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

[Janusz Krzysztofik]

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?

Up to you.

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

Thanks,
Janusz


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