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

From: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
To: igt-dev@lists.freedesktop.org,
	Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Subject: Re: [PATCH i-g-t v4 3/4] lib/igt_sysfs: Fix off-by-one in buffer size
Date: Thu, 29 Feb 2024 12:07:01 +0100	[thread overview]
Message-ID: <3709752.RUnXabflUD@jkrzyszt-mobl2.ger.corp.intel.com> (raw)
In-Reply-To: <20240228223134.3908035-3-lucas.demarchi@intel.com>

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@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?

Thanks,
Janusz

>  			ret = -EINVAL;
>  			goto free_buf;
>  		}
> 