[PATCH] bcachefs: Avoid a potential useless over memory allocation in bch2_prt_vprintf()

[PATCH] bcachefs: Avoid a potential useless over memory allocation in bch2_prt_vprintf()

[Christophe JAILLET]

printbuf_remaining() returns the number of characters we can print to the
output buffer - i.e. excluding the terminating null.

vsnprintf() takes the size of the buffer, including the trailing null
space.
It is truncated if the returned value is greater than or equal to the size
of the buffer.

Knowing all that, buffer sizes and overflow checks can be fixed in order
to potentially avoid a useless memory over-allocation.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Un-tested
---
 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 de41f9a14492..77bee9060bfe 100644
--- a/fs/bcachefs/printbuf.c
+++ b/fs/bcachefs/printbuf.c
@@ -54,8 +54,9 @@ 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);
-	} while (len + 1 >= printbuf_remaining(out) &&
+		len = vsnprintf(out->buf + out->pos, printbuf_remaining(out) + 1,
+				fmt, args2);
+	} while (len >= printbuf_remaining(out) + 1 &&
 		 !bch2_printbuf_make_room(out, len + 1));
 
 	len = min_t(size_t, len,
@@ -70,9 +71,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(out) + 1,
+				fmt, args);
 		va_end(args);
-	} while (len + 1 >= printbuf_remaining(out) &&
+	} while (len >= printbuf_remaining(out) + 1 &&
 		 !bch2_printbuf_make_room(out, len + 1));
 
 	len = min_t(size_t, len,
-- 
2.34.1

Re: [PATCH] bcachefs: Avoid a potential useless over memory allocation in bch2_prt_vprintf()

[Brian Foster]

On Sat, Sep 16, 2023 at 08:20:24AM +0200, Christophe JAILLET wrote:
> printbuf_remaining() returns the number of characters we can print to the
> output buffer - i.e. excluding the terminating null.
> 
> vsnprintf() takes the size of the buffer, including the trailing null
> space.
> It is truncated if the returned value is greater than or equal to the size
> of the buffer.
> 
> Knowing all that, buffer sizes and overflow checks can be fixed in order
> to potentially avoid a useless memory over-allocation.
> 

For whatever reason I had a hard time parsing this last sentence.  Do
you mean to say there's an off by one here that leads to an unnecessary
overallocation?

> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Un-tested
> ---
>  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 de41f9a14492..77bee9060bfe 100644
> --- a/fs/bcachefs/printbuf.c
> +++ b/fs/bcachefs/printbuf.c
> @@ -54,8 +54,9 @@ 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);
> -	} while (len + 1 >= printbuf_remaining(out) &&
> +		len = vsnprintf(out->buf + out->pos, printbuf_remaining(out) + 1,
> +				fmt, args2);
> +	} while (len >= printbuf_remaining(out) + 1 &&
>  		 !bch2_printbuf_make_room(out, len + 1));

It's amazing how simple arithmetic can make my eyes cross at times. :) I
think I follow the fix after reading the commit log a couple times, but
could we use printbuf_remaining_size() appropriately in these places
that want to check actual buffer size (i.e. including terminating null)
instead of doing the manual size fixup?

Brian

>  
>  	len = min_t(size_t, len,
> @@ -70,9 +71,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(out) + 1,
> +				fmt, args);
>  		va_end(args);
> -	} while (len + 1 >= printbuf_remaining(out) &&
> +	} while (len >= printbuf_remaining(out) + 1 &&
>  		 !bch2_printbuf_make_room(out, len + 1));
>  
>  	len = min_t(size_t, len,
> -- 
> 2.34.1
> 

Re: [PATCH] bcachefs: Avoid a potential useless over memory allocation in bch2_prt_vprintf()

[Christophe JAILLET]

Le 19/09/2023 à 15:17, Brian Foster a écrit :
> On Sat, Sep 16, 2023 at 08:20:24AM +0200, Christophe JAILLET wrote:
>> printbuf_remaining() returns the number of characters we can print to the
>> output buffer - i.e. excluding the terminating null.
>>
>> vsnprintf() takes the size of the buffer, including the trailing null
>> space.
>> It is truncated if the returned value is greater than or equal to the size
>> of the buffer.
>>
>> Knowing all that, buffer sizes and overflow checks can be fixed in order
>> to potentially avoid a useless memory over-allocation.
>>
> 
> For whatever reason I had a hard time parsing this last sentence.  Do
> you mean to say there's an off by one here that leads to an unnecessary
> overallocation?

An off-by-two in fact, IIUC.
But yes, that's my point.

We consider that the string is truncated when it may not be (len+1 vs 
len) and we under-estimate the available space (printbuf_remaining() vs 
printbuf_remaining()+1 or printbuf_remaining_size())

> 
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Un-tested
>> ---
>>   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 de41f9a14492..77bee9060bfe 100644
>> --- a/fs/bcachefs/printbuf.c
>> +++ b/fs/bcachefs/printbuf.c
>> @@ -54,8 +54,9 @@ 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);
>> -	} while (len + 1 >= printbuf_remaining(out) &&
>> +		len = vsnprintf(out->buf + out->pos, printbuf_remaining(out) + 1,
>> +				fmt, args2);
>> +	} while (len >= printbuf_remaining(out) + 1 &&
>>   		 !bch2_printbuf_make_room(out, len + 1));
> 
> It's amazing how simple arithmetic can make my eyes cross at times. :) I
> think I follow the fix after reading the commit log a couple times, but
> could we use printbuf_remaining_size() appropriately in these places
> that want to check actual buffer size (i.e. including terminating null)
> instead of doing the manual size fixup?

Sure, it would be much better.
I had not seen this function.

CJ

> 
> Brian
> 
>>   
>>   	len = min_t(size_t, len,
>> @@ -70,9 +71,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(out) + 1,
>> +				fmt, args);
>>   		va_end(args);
>> -	} while (len + 1 >= printbuf_remaining(out) &&
>> +	} while (len >= printbuf_remaining(out) + 1 &&
>>   		 !bch2_printbuf_make_room(out, len + 1));
>>   
>>   	len = min_t(size_t, len,
>> -- 
>> 2.34.1
>>
> 
>