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 <[email protected]>
---
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
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 <[email protected]>
Thanks, applied
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 <[email protected]>
> ---
> 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
>
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.
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 <[email protected]>
>> ---
>> 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
>>
>
>
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 <[email protected]>
> > > ---
> > > 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
> > >
> >
> >
>