diff mbox series

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

Message ID 0f40108bed3e084057223bdbe32c4b37f8500ff3.1694845203.git.christophe.jaillet@wanadoo.fr
State New
Headers
Series bcachefs: Avoid a potential useless over memory allocation in bch2_prt_vprintf() |

Commit Message

Christophe JAILLET Sept. 16, 2023, 6:20 a.m. UTC 
  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(-)

Comments

Brian Foster Sept. 19, 2023, 1:17 p.m. UTC | #1 
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
>
Christophe JAILLET Sept. 19, 2023, 6:18 p.m. UTC | #2 
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
>>
> 
>
diff mbox series

Patch

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,