[PATCH v2] bcachefs: Avoid a potential useless over memory allocation in bch2_prt_[v]printf()

[PATCH v2] bcachefs: Avoid a potential useless over memory allocation in bch2_prt_[v]printf()

[Christophe JAILLET]

2 issues related to incorrect available space in the output buffer
computation may lead to some extra memory allocation.


First: vsnprintf() takes the size of the buffer, *including* the space for
the trailing null.

But printbuf_remaining() returns the number of characters we can print
to the output buffer, *excluding* the terminating null.

So, use printbuf_remaining_size(), which includes the space for the
terminating null.


Second: if the return value of vsnprintf() is greater than or equal to the
passed size, the resulting string is truncated.
So, in order to see if some extra space is needed, the check needs to be
changed.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Un-tested

v2: - Use printbuf_remaining_size() instead of hand-writing it.  [Brian Foster]
    - Reword the commit log, hoping it is clearer.
    - Synch with -next-20240215

v1: https://lore.kernel.org/all/0f40108bed3e084057223bdbe32c4b37f8500ff3.1694845203.git.christophe.jaillet@wanadoo.fr/
---
 fs/bcachefs/printbuf.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/bcachefs/printbuf.c b/fs/bcachefs/printbuf.c
index b27d22925929..8cee9c2f88c4 100644
--- a/fs/bcachefs/printbuf.c
+++ b/fs/bcachefs/printbuf.c
@@ -55,9 +55,10 @@ void bch2_prt_vprintf(struct printbuf *out, const char *fmt, va_list args)
 		va_list args2;
 
 		va_copy(args2, args);
-		len = vsnprintf(out->buf + out->pos, printbuf_remaining(out), fmt, args2);
+		len = vsnprintf(out->buf + out->pos, printbuf_remaining_size(out),
+				fmt, args2);
 		va_end(args2);
-	} while (len + 1 >= printbuf_remaining(out) &&
+	} while (len >= printbuf_remaining_size(out) &&
 		 !bch2_printbuf_make_room(out, len + 1));
 
 	len = min_t(size_t, len,
@@ -72,9 +73,10 @@ void bch2_prt_printf(struct printbuf *out, const char *fmt, ...)
 
 	do {
 		va_start(args, fmt);
-		len = vsnprintf(out->buf + out->pos, printbuf_remaining(out), fmt, args);
+		len = vsnprintf(out->buf + out->pos, printbuf_remaining_size(out),
+				fmt, args);
 		va_end(args);
-	} while (len + 1 >= printbuf_remaining(out) &&
+	} while (len >= printbuf_remaining_size(out) &&
 		 !bch2_printbuf_make_room(out, len + 1));
 
 	len = min_t(size_t, len,
-- 
2.43.2

Re: [PATCH v2] bcachefs: Avoid a potential useless over memory allocation in bch2_prt_[v]printf()

[Kent Overstreet]

On Sun, Feb 18, 2024 at 05:12:28PM +0100, Christophe JAILLET wrote:
> 2 issues related to incorrect available space in the output buffer
> computation may lead to some extra memory allocation.
> 
> 
> First: vsnprintf() takes the size of the buffer, *including* the space for
> the trailing null.
> 
> But printbuf_remaining() returns the number of characters we can print
> to the output buffer, *excluding* the terminating null.
> 
> So, use printbuf_remaining_size(), which includes the space for the
> terminating null.
> 
> 
> Second: if the return value of vsnprintf() is greater than or equal to the
> passed size, the resulting string is truncated.
> So, in order to see if some extra space is needed, the check needs to be
> changed.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Thanks, applied

Re: [PATCH v2] bcachefs: Avoid a potential useless over memory allocation in bch2_prt_[v]printf()

[Kent Overstreet]

On Sun, Feb 18, 2024 at 05:12:28PM +0100, Christophe JAILLET wrote:
> 2 issues related to incorrect available space in the output buffer
> computation may lead to some extra memory allocation.
> 
> 
> First: vsnprintf() takes the size of the buffer, *including* the space for
> the trailing null.
> 
> But printbuf_remaining() returns the number of characters we can print
> to the output buffer, *excluding* the terminating null.
> 
> So, use printbuf_remaining_size(), which includes the space for the
> terminating null.

nope, buggy.

passing printbuf_remaining_size() to snprintf() is correct, but
snprintf() returns the number of charecters wrote _excluding_ the null

> 
> 
> Second: if the return value of vsnprintf() is greater than or equal to the
> passed size, the resulting string is truncated.
> So, in order to see if some extra space is needed, the check needs to be
> changed.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Un-tested
> 
> v2: - Use printbuf_remaining_size() instead of hand-writing it.  [Brian Foster]
>     - Reword the commit log, hoping it is clearer.
>     - Synch with -next-20240215
> 
> v1: https://lore.kernel.org/all/0f40108bed3e084057223bdbe32c4b37f8500ff3.1694845203.git.christophe.jaillet@wanadoo.fr/
> ---
>  fs/bcachefs/printbuf.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/bcachefs/printbuf.c b/fs/bcachefs/printbuf.c
> index b27d22925929..8cee9c2f88c4 100644
> --- a/fs/bcachefs/printbuf.c
> +++ b/fs/bcachefs/printbuf.c
> @@ -55,9 +55,10 @@ void bch2_prt_vprintf(struct printbuf *out, const char *fmt, va_list args)
>  		va_list args2;
>  
>  		va_copy(args2, args);
> -		len = vsnprintf(out->buf + out->pos, printbuf_remaining(out), fmt, args2);
> +		len = vsnprintf(out->buf + out->pos, printbuf_remaining_size(out),
> +				fmt, args2);
>  		va_end(args2);
> -	} while (len + 1 >= printbuf_remaining(out) &&
> +	} while (len >= printbuf_remaining_size(out) &&
>  		 !bch2_printbuf_make_room(out, len + 1));
>  
>  	len = min_t(size_t, len,
> @@ -72,9 +73,10 @@ void bch2_prt_printf(struct printbuf *out, const char *fmt, ...)
>  
>  	do {
>  		va_start(args, fmt);
> -		len = vsnprintf(out->buf + out->pos, printbuf_remaining(out), fmt, args);
> +		len = vsnprintf(out->buf + out->pos, printbuf_remaining_size(out),
> +				fmt, args);
>  		va_end(args);
> -	} while (len + 1 >= printbuf_remaining(out) &&
> +	} while (len >= printbuf_remaining_size(out) &&
>  		 !bch2_printbuf_make_room(out, len + 1));
>  
>  	len = min_t(size_t, len,
> -- 
> 2.43.2
> 

Re: [PATCH v2] bcachefs: Avoid a potential useless over memory allocation in bch2_prt_[v]printf()

[Christophe JAILLET]

Le 19/02/2024 à 04:39, Kent Overstreet a écrit :
> On Sun, Feb 18, 2024 at 05:12:28PM +0100, Christophe JAILLET wrote:
>> 2 issues related to incorrect available space in the output buffer
>> computation may lead to some extra memory allocation.
>>
>>
>> First: vsnprintf() takes the size of the buffer, *including* the space for
>> the trailing null.
>>
>> But printbuf_remaining() returns the number of characters we can print
>> to the output buffer, *excluding* the terminating null.
>>
>> So, use printbuf_remaining_size(), which includes the space for the
>> terminating null.
> 
> nope, buggy.
> 
> passing printbuf_remaining_size() to snprintf() is correct, but
> snprintf() returns the number of charecters wrote _excluding_ the null

Hi,

I think that the patch is correct.

snprintf() returns the number of characters wrote _excluding_ the null. 
That is why the test is: len >= size_of_the_buffer   ==>  more space is 
needed.
The case you describe is handled by the == part of >=.

On the contrary, if len is < size_of_the_buffer, then we have at least 1 
place for the terminating NULL. It will then eventually lead to:
   len_of_the_string + space_for_'\0' ==  size_of_the_buffer
So it does not overflow.


Anyway, feel free to ignore the patch completely if it sounds too risky, 
take only half of it (s/printbuf_remaining/printbuf_remaining_size/) if 
you are more confident with it, or the complete patch if the explanation 
above convinced you.

 From my PoV, the 3 options lead to the same run-time output, with more 
or less memory allocated.

Best regards.

CJ


> 
>>
>>
>> Second: if the return value of vsnprintf() is greater than or equal to the
>> passed size, the resulting string is truncated.
>> So, in order to see if some extra space is needed, the check needs to be
>> changed.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Un-tested
>>
>> v2: - Use printbuf_remaining_size() instead of hand-writing it.  [Brian Foster]
>>      - Reword the commit log, hoping it is clearer.
>>      - Synch with -next-20240215
>>
>> v1: https://lore.kernel.org/all/0f40108bed3e084057223bdbe32c4b37f8500ff3.1694845203.git.christophe.jaillet@wanadoo.fr/
>> ---
>>   fs/bcachefs/printbuf.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/bcachefs/printbuf.c b/fs/bcachefs/printbuf.c
>> index b27d22925929..8cee9c2f88c4 100644
>> --- a/fs/bcachefs/printbuf.c
>> +++ b/fs/bcachefs/printbuf.c
>> @@ -55,9 +55,10 @@ void bch2_prt_vprintf(struct printbuf *out, const char *fmt, va_list args)
>>   		va_list args2;
>>   
>>   		va_copy(args2, args);
>> -		len = vsnprintf(out->buf + out->pos, printbuf_remaining(out), fmt, args2);
>> +		len = vsnprintf(out->buf + out->pos, printbuf_remaining_size(out),
>> +				fmt, args2);
>>   		va_end(args2);
>> -	} while (len + 1 >= printbuf_remaining(out) &&
>> +	} while (len >= printbuf_remaining_size(out) &&
>>   		 !bch2_printbuf_make_room(out, len + 1));
>>   
>>   	len = min_t(size_t, len,
>> @@ -72,9 +73,10 @@ void bch2_prt_printf(struct printbuf *out, const char *fmt, ...)
>>   
>>   	do {
>>   		va_start(args, fmt);
>> -		len = vsnprintf(out->buf + out->pos, printbuf_remaining(out), fmt, args);
>> +		len = vsnprintf(out->buf + out->pos, printbuf_remaining_size(out),
>> +				fmt, args);
>>   		va_end(args);
>> -	} while (len + 1 >= printbuf_remaining(out) &&
>> +	} while (len >= printbuf_remaining_size(out) &&
>>   		 !bch2_printbuf_make_room(out, len + 1));
>>   
>>   	len = min_t(size_t, len,
>> -- 
>> 2.43.2
>>
> 
> 

Re: [PATCH v2] bcachefs: Avoid a potential useless over memory allocation in bch2_prt_[v]printf()

[Kent Overstreet]

On Mon, Feb 19, 2024 at 07:10:10PM +0100, Christophe JAILLET wrote:
> Le 19/02/2024 à 04:39, Kent Overstreet a écrit :
> > On Sun, Feb 18, 2024 at 05:12:28PM +0100, Christophe JAILLET wrote:
> > > 2 issues related to incorrect available space in the output buffer
> > > computation may lead to some extra memory allocation.
> > > 
> > > 
> > > First: vsnprintf() takes the size of the buffer, *including* the space for
> > > the trailing null.
> > > 
> > > But printbuf_remaining() returns the number of characters we can print
> > > to the output buffer, *excluding* the terminating null.
> > > 
> > > So, use printbuf_remaining_size(), which includes the space for the
> > > terminating null.
> > 
> > nope, buggy.
> > 
> > passing printbuf_remaining_size() to snprintf() is correct, but
> > snprintf() returns the number of charecters wrote _excluding_ the null
> 
> Hi,
> 
> I think that the patch is correct.

You didn't test it, I did.

In the future, no more patches that you haven't tested - thanks.

> snprintf() returns the number of characters wrote _excluding_ the null. That
> is why the test is: len >= size_of_the_buffer   ==>  more space is needed.
> The case you describe is handled by the == part of >=.
> 
> On the contrary, if len is < size_of_the_buffer, then we have at least 1
> place for the terminating NULL. It will then eventually lead to:
>   len_of_the_string + space_for_'\0' ==  size_of_the_buffer
> So it does not overflow.
> 
> 
> Anyway, feel free to ignore the patch completely if it sounds too risky,
> take only half of it (s/printbuf_remaining/printbuf_remaining_size/) if you
> are more confident with it, or the complete patch if the explanation above
> convinced you.
> 
> From my PoV, the 3 options lead to the same run-time output, with more or
> less memory allocated.
> 
> Best regards.
> 
> CJ
> 
> 
> > 
> > > 
> > > 
> > > Second: if the return value of vsnprintf() is greater than or equal to the
> > > passed size, the resulting string is truncated.
> > > So, in order to see if some extra space is needed, the check needs to be
> > > changed.
> > > 
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > > Un-tested
> > > 
> > > v2: - Use printbuf_remaining_size() instead of hand-writing it.  [Brian Foster]
> > >      - Reword the commit log, hoping it is clearer.
> > >      - Synch with -next-20240215
> > > 
> > > v1: https://lore.kernel.org/all/0f40108bed3e084057223bdbe32c4b37f8500ff3.1694845203.git.christophe.jaillet@wanadoo.fr/
> > > ---
> > >   fs/bcachefs/printbuf.c | 10 ++++++----
> > >   1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/bcachefs/printbuf.c b/fs/bcachefs/printbuf.c
> > > index b27d22925929..8cee9c2f88c4 100644
> > > --- a/fs/bcachefs/printbuf.c
> > > +++ b/fs/bcachefs/printbuf.c
> > > @@ -55,9 +55,10 @@ void bch2_prt_vprintf(struct printbuf *out, const char *fmt, va_list args)
> > >   		va_list args2;
> > >   		va_copy(args2, args);
> > > -		len = vsnprintf(out->buf + out->pos, printbuf_remaining(out), fmt, args2);
> > > +		len = vsnprintf(out->buf + out->pos, printbuf_remaining_size(out),
> > > +				fmt, args2);
> > >   		va_end(args2);
> > > -	} while (len + 1 >= printbuf_remaining(out) &&
> > > +	} while (len >= printbuf_remaining_size(out) &&
> > >   		 !bch2_printbuf_make_room(out, len + 1));
> > >   	len = min_t(size_t, len,
> > > @@ -72,9 +73,10 @@ void bch2_prt_printf(struct printbuf *out, const char *fmt, ...)
> > >   	do {
> > >   		va_start(args, fmt);
> > > -		len = vsnprintf(out->buf + out->pos, printbuf_remaining(out), fmt, args);
> > > +		len = vsnprintf(out->buf + out->pos, printbuf_remaining_size(out),
> > > +				fmt, args);
> > >   		va_end(args);
> > > -	} while (len + 1 >= printbuf_remaining(out) &&
> > > +	} while (len >= printbuf_remaining_size(out) &&
> > >   		 !bch2_printbuf_make_room(out, len + 1));
> > >   	len = min_t(size_t, len,
> > > -- 
> > > 2.43.2
> > > 
> > 
> > 
> 

Re: [PATCH v2] bcachefs: Avoid a potential useless over memory allocation in bch2_prt_[v]printf()

[Kent Overstreet]

On Sun, Feb 18, 2024 at 05:12:28PM +0100, Christophe JAILLET wrote:
> 2 issues related to incorrect available space in the output buffer
> computation may lead to some extra memory allocation.
> 
> 
> First: vsnprintf() takes the size of the buffer, *including* the space for
> the trailing null.
> 
> But printbuf_remaining() returns the number of characters we can print
> to the output buffer, *excluding* the terminating null.
> 
> So, use printbuf_remaining_size(), which includes the space for the
> terminating null.
> 
> 
> Second: if the return value of vsnprintf() is greater than or equal to the
> passed size, the resulting string is truncated.
> So, in order to see if some extra space is needed, the check needs to be
> changed.

btw, the patch was suspect to begin with

in cases where off-by-one errors are difficult to avoid, but harmless in
one direction - just over allocate.