[PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time

[PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time

[Nguyễn Thái Ngọc Duy]

strbuf_grow(sb, 0) may allocate less than requested len and violate the
next assertion.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 strbuf.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index 07060ce..ab213da 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -34,7 +34,7 @@ extern void strbuf_grow(struct strbuf *, size_t);
 
 static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
 	if (!sb->alloc)
-		strbuf_grow(sb, 0);
+		strbuf_grow(sb, len);
 	assert(len < sb->alloc);
 	sb->len = len;
 	sb->buf[len] = '\0';
-- 
1.7.4.74.g639db

Re: [PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time

[Nguyen Thai Ngoc Duy]

On Tue, Apr 26, 2011 at 07:24:20PM +0700, Nguyễn Thái Ngọc Duy wrote:
>  static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
>  	if (!sb->alloc)
> -		strbuf_grow(sb, 0);
> +		strbuf_grow(sb, len);
>  	assert(len < sb->alloc);
>  	sb->len = len;
>  	sb->buf[len] = '\0';

This should be better. But I fail to see why it corrupts memory :(

@@ -33,9 +33,8 @@ static inline size_t strbuf_avail(const struct strbuf *sb) {
 extern void strbuf_grow(struct strbuf *, size_t);
 
 static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
-	if (!sb->alloc)
-		strbuf_grow(sb, 0);
-	assert(len < sb->alloc);
+	if (sb->alloc < len + 1)
+		strbuf_grow(sb, len - sb->alloc);
 	sb->len = len;
 	sb->buf[len] = '\0';
 }

-- 
Duy

Re: [PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> strbuf_grow(sb, 0) may allocate less than requested len and violate the
> next assertion.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  strbuf.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/strbuf.h b/strbuf.h
> index 07060ce..ab213da 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -34,7 +34,7 @@ extern void strbuf_grow(struct strbuf *, size_t);
>  
>  static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
>  	if (!sb->alloc)
> -		strbuf_grow(sb, 0);
> +		strbuf_grow(sb, len);
>  	assert(len < sb->alloc);

This looks so obviously correct that it is scary.

How could 60 callsites of this function manage to have run without crashes
so far?  They all happen to use the function on a buffer that already has
something on it?

Well spotted, thanks.

Re: [PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time

[Nguyen Thai Ngoc Duy]

2011/4/26 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> strbuf_grow(sb, 0) may allocate less than requested len and violate the
>> next assertion.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  strbuf.h |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/strbuf.h b/strbuf.h
>> index 07060ce..ab213da 100644
>> --- a/strbuf.h
>> +++ b/strbuf.h
>> @@ -34,7 +34,7 @@ extern void strbuf_grow(struct strbuf *, size_t);
>>
>>  static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
>>       if (!sb->alloc)
>> -             strbuf_grow(sb, 0);
>> +             strbuf_grow(sb, len);
>>       assert(len < sb->alloc);
>
> This looks so obviously correct that it is scary.
>
> How could 60 callsites of this function manage to have run without crashes
> so far?  They all happen to use the function on a buffer that already has
> something on it?

I guess no current call site does _setlen right after initialization.
It's new code that triggers it.
-- 
Duy

Re: [PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time

[René Scharfe]

Am 26.04.2011 17:32, schrieb Nguyen Thai Ngoc Duy:
> 2011/4/26 Junio C Hamano<gitster@pobox.com>:
>> Nguyễn Thái Ngọc Duy<pclouds@gmail.com>  writes:
>>
>>> strbuf_grow(sb, 0) may allocate less than requested len and violate the
>>> next assertion.
>>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy<pclouds@gmail.com>
>>> ---
>>>   strbuf.h |    2 +-
>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/strbuf.h b/strbuf.h
>>> index 07060ce..ab213da 100644
>>> --- a/strbuf.h
>>> +++ b/strbuf.h
>>> @@ -34,7 +34,7 @@ extern void strbuf_grow(struct strbuf *, size_t);
>>>
>>>   static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
>>>        if (!sb->alloc)
>>> -             strbuf_grow(sb, 0);
>>> +             strbuf_grow(sb, len);
>>>        assert(len<  sb->alloc);
>>
>> This looks so obviously correct that it is scary.
>>
>> How could 60 callsites of this function manage to have run without crashes
>> so far?  They all happen to use the function on a buffer that already has
>> something on it?
>
> I guess no current call site does _setlen right after initialization.
> It's new code that triggers it.

Documentation/technical/api-strbuf.txt says that you can't use 
strbuf_setlen() to allocate more space for a strbuf, i.e. you can use it 
only to shorten a string or set it to its current length.  If the strbuf 
in question hasn't allocated any buffer at all (sb->alloc == 0) then the 
only valid len is zero.

Where is it used to extend strbufs, i.e. which is the new code you 
mentioned?

René

Re: [PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> strbuf_grow(sb, 0) may allocate less than requested len and violate the
> next assertion.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  strbuf.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/strbuf.h b/strbuf.h
> index 07060ce..ab213da 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -34,7 +34,7 @@ extern void strbuf_grow(struct strbuf *, size_t);
>  
>  static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
>  	if (!sb->alloc)
> -		strbuf_grow(sb, 0);
> +		strbuf_grow(sb, len);
>  	assert(len < sb->alloc);

Wait.  Isn't _setlen() meant to be used only to truncate at the length,
and isn't that clearly documented?

I don't think we need this patch at all.

Re: [PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time

[René Scharfe]

Am 26.04.2011 19:18, schrieb Junio C Hamano:
> Nguyễn Thái Ngọc Duy<pclouds@gmail.com>  writes:
> 
>> strbuf_grow(sb, 0) may allocate less than requested len and violate the
>> next assertion.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy<pclouds@gmail.com>
>> ---
>>   strbuf.h |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/strbuf.h b/strbuf.h
>> index 07060ce..ab213da 100644
>> --- a/strbuf.h
>> +++ b/strbuf.h
>> @@ -34,7 +34,7 @@ extern void strbuf_grow(struct strbuf *, size_t);
>>
>>   static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
>>   	if (!sb->alloc)
>> -		strbuf_grow(sb, 0);
>> +		strbuf_grow(sb, len);
>>   	assert(len<  sb->alloc);
> 
> Wait.  Isn't _setlen() meant to be used only to truncate at the length,
> and isn't that clearly documented?
> 
> I don't think we need this patch at all.

How about something like this instead?  The call to strbuf_grow() was
introduced in a8f3e2219 when there was no strbuf_slopbuf buffer that
nowadays makes sure we always have a place to write an initial NUL.
We can take it out again now, simplifying the code and hopefully
avoiding future confusion.

 strbuf.h |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index 07060ce..b157ede 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -33,9 +33,7 @@ static inline size_t strbuf_avail(const struct strbuf *sb) {
 extern void strbuf_grow(struct strbuf *, size_t);
 
 static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
-	if (!sb->alloc)
-		strbuf_grow(sb, 0);
-	assert(len < sb->alloc);
+	assert(len < (sb->alloc ? sb->alloc : 1));
 	sb->len = len;
 	sb->buf[len] = '\0';
 }

Re: [PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> How about something like this instead?  The call to strbuf_grow() was
> introduced in a8f3e2219 when there was no strbuf_slopbuf buffer that
> nowadays makes sure we always have a place to write an initial NUL.
> We can take it out again now, simplifying the code and hopefully
> avoiding future confusion.

Thanks; I think that makes sense.

It further may make sense to turn the assert into BUG() though, to clarify
what kind of programming error we are trying to catch.  Perhaps like:

>  static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
> +	assert(len < (sb->alloc ? sb->alloc : 1));

	if (len < (sb->alloc ? sb->alloc : 1))
		die("programming error: using strbuf_setlen() to extend a strbuf");

>  	sb->len = len;
>  	sb->buf[len] = '\0';
>  }

Re: [PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time

[René Scharfe]

Am 26.04.2011 23:51, schrieb Junio C Hamano:
> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:
> 
>> How about something like this instead?  The call to strbuf_grow() was
>> introduced in a8f3e2219 when there was no strbuf_slopbuf buffer that
>> nowadays makes sure we always have a place to write an initial NUL.
>> We can take it out again now, simplifying the code and hopefully
>> avoiding future confusion.
> 
> Thanks; I think that makes sense.
> 
> It further may make sense to turn the assert into BUG() though, to clarify
> what kind of programming error we are trying to catch.  Perhaps like:
> 
>>   static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
>> +	assert(len<  (sb->alloc ? sb->alloc : 1));
> 
> 	if (len<  (sb->alloc ? sb->alloc : 1))
> 		die("programming error: using strbuf_setlen() to extend a strbuf");
> 
>>   	sb->len = len;
>>   	sb->buf[len] = '\0';
>>   }

I like the idea, except the comparison needs to be inverted.

Compiled, but not tested.  The test suite takes too long and skips too
many tests on my Windows box and I don't have any other machine
available right now. :-/

-- >8 --
Subject: strbuf: clarify assertion in strbuf_setlen()

Commit a8f3e2219 introduced the strbuf_grow() call to strbuf_setlen() to
make ensure that there was at least one byte available to write the
mandatory trailing NUL, even for previously unallocated strbufs.

Then b315c5c0 added strbuf_slopbuf for the same reason, only globally for
all uses of strbufs.

Thus the strbuf_grow() call can be removed now.  This avoids readers of
strbuf.h from mistakenly thinking that strbuf_setlen() can be used to
extend a strbuf.

The following assert() needs to be changed to cope with the fact that
sb->alloc can now be zero, which is OK as long as len is also zero.  As
suggested by Junio, use the chance to convert it to a die() with a short
explanatory message.  The pattern of 'die("BUG: ...")' is already used in
strbuf.c.

This was the only assert() in strbuf.[ch], so assert.h doesn't have to be
included anymore either.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 strbuf.h |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index 07060ce..9e6d9fa 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -3,8 +3,6 @@
 
 /* See Documentation/technical/api-strbuf.txt */
 
-#include <assert.h>
-
 extern char strbuf_slopbuf[];
 struct strbuf {
 	size_t alloc;
@@ -33,9 +31,8 @@ static inline size_t strbuf_avail(const struct strbuf *sb) {
 extern void strbuf_grow(struct strbuf *, size_t);
 
 static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
-	if (!sb->alloc)
-		strbuf_grow(sb, 0);
-	assert(len < sb->alloc);
+	if (len > (sb->alloc ? sb->alloc - 1 : 0))
+		die("BUG: strbuf_setlen() beyond buffer");
 	sb->len = len;
 	sb->buf[len] = '\0';
 }
-- 
1.7.5

Re: [PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Subject: strbuf: clarify assertion in strbuf_setlen()
>
> Commit a8f3e2219 introduced the strbuf_grow() call to strbuf_setlen() to
> make ensure that there was at least one byte available to write the
> mandatory trailing NUL, even for previously unallocated strbufs.
>
> Then b315c5c0 added strbuf_slopbuf for the same reason, only globally for
> all uses of strbufs.
>
> Thus the strbuf_grow() call can be removed now.  This avoids readers of
> strbuf.h from mistakenly thinking that strbuf_setlen() can be used to
> extend a strbuf.
>
> The following assert() needs to be changed to cope with the fact that
> sb->alloc can now be zero, which is OK as long as len is also zero.  As
> suggested by Junio, use the chance to convert it to a die() with a short
> explanatory message.  The pattern of 'die("BUG: ...")' is already used in
> strbuf.c.
>
> This was the only assert() in strbuf.[ch], so assert.h doesn't have to be
> included anymore either.
>
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>

Very nicely explained.  I wish everybody wrote his log message as clearly
as this.

Thanks, will queue.

> ---
>  strbuf.h |    7 ++-----
>  1 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/strbuf.h b/strbuf.h
> index 07060ce..9e6d9fa 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -3,8 +3,6 @@
>  
>  /* See Documentation/technical/api-strbuf.txt */
>  
> -#include <assert.h>
> -
>  extern char strbuf_slopbuf[];
>  struct strbuf {
>  	size_t alloc;
> @@ -33,9 +31,8 @@ static inline size_t strbuf_avail(const struct strbuf *sb) {
>  extern void strbuf_grow(struct strbuf *, size_t);
>  
>  static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
> -	if (!sb->alloc)
> -		strbuf_grow(sb, 0);
> -	assert(len < sb->alloc);
> +	if (len > (sb->alloc ? sb->alloc - 1 : 0))
> +		die("BUG: strbuf_setlen() beyond buffer");
>  	sb->len = len;
>  	sb->buf[len] = '\0';
>  }

Re: [PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time

[Nguyen Thai Ngoc Duy]

2011/4/27 René Scharfe <rene.scharfe@lsrfire.ath.cx>:
> Am 26.04.2011 19:18, schrieb Junio C Hamano:
>> Nguyễn Thái Ngọc Duy<pclouds@gmail.com>  writes:
>>
>>> strbuf_grow(sb, 0) may allocate less than requested len and violate the
>>> next assertion.
>>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy<pclouds@gmail.com>
>>> ---
>>>   strbuf.h |    2 +-
>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/strbuf.h b/strbuf.h
>>> index 07060ce..ab213da 100644
>>> --- a/strbuf.h
>>> +++ b/strbuf.h
>>> @@ -34,7 +34,7 @@ extern void strbuf_grow(struct strbuf *, size_t);
>>>
>>>   static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
>>>      if (!sb->alloc)
>>> -            strbuf_grow(sb, 0);
>>> +            strbuf_grow(sb, len);
>>>      assert(len<  sb->alloc);
>>
>> Wait.  Isn't _setlen() meant to be used only to truncate at the length,
>> and isn't that clearly documented?
>>
>> I don't think we need this patch at all.
>
> How about something like this instead?  The call to strbuf_grow() was
> introduced in a8f3e2219 when there was no strbuf_slopbuf buffer that
> nowadays makes sure we always have a place to write an initial NUL.
> We can take it out again now, simplifying the code and hopefully
> avoiding future confusion.
>
>  strbuf.h |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/strbuf.h b/strbuf.h
> index 07060ce..b157ede 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -33,9 +33,7 @@ static inline size_t strbuf_avail(const struct strbuf *sb) {
>  extern void strbuf_grow(struct strbuf *, size_t);
>
>  static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
> -       if (!sb->alloc)
> -               strbuf_grow(sb, 0);
> -       assert(len < sb->alloc);
> +       assert(len < (sb->alloc ? sb->alloc : 1));
>        sb->len = len;
>        sb->buf[len] = '\0';
>  }
>
>

Fine to me. I forgot the document, saw _grow and was under impression
that _setlen can grow too.
-- 
Duy