[PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again

[PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again

[Sebastian Schuberth]

2564aa4 started to initialize buf.alloc, but that should actually be one
more byte than the string length due to the trailing \0.
---
 builtin/blame.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 86c0537..45f0dcc 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2114,7 +2114,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 		case S_IFREG:
 			if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
 			    textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) {
-				buf.alloc = buf_len;
+				buf.alloc = buf_len + 1;
 				buf.len = buf_len;
 			}
 			else if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)
-- 
1.7.8.rc0.46.g5ae0f.dirty

Re: [PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> 2564aa4 started to initialize buf.alloc, but that should actually be one
> more byte than the string length due to the trailing \0.

Even when the conversion result is a zero-length string?

> ---

Sign-off?

>  builtin/blame.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 86c0537..45f0dcc 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2114,7 +2114,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
>  		case S_IFREG:
>  			if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
>  			    textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) {
> -				buf.alloc = buf_len;
> +				buf.alloc = buf_len + 1;
>  				buf.len = buf_len;
>  			}
>  			else if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)

Re: [PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again

[Jeff King]

On Mon, Nov 07, 2011 at 04:53:10PM +0100, Sebastian Schuberth wrote:

> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2114,7 +2114,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
>  		case S_IFREG:
>  			if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
>  			    textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) {
> -				buf.alloc = buf_len;
> +				buf.alloc = buf_len + 1;
>  				buf.len = buf_len;
>  			}
>  			else if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)

Your patch is correct in the sense that this fixes a bug, but it just
seems wrong to be touching the alloc parameter outside of the strbuf
code. It looks like the intent is to do the equivalent of:

  strbuf_add(&buf, some_heap_buf, len);
  free(some_heap_buf);

but avoid the extra allocation and copy. Should there be a
"strbuf_attach" function to encapsulate this pattern?

-Peff

Re: [PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again

[Sebastian Schuberth]

On 07.11.2011 17:47, Junio C Hamano wrote:

>> 2564aa4 started to initialize buf.alloc, but that should actually be one
>> more byte than the string length due to the trailing \0.
>
> Even when the conversion result is a zero-length string?

In this case, yes. The string buffer is initialized (and detached) in 
run_textconv, which calls strbuf_read, which grows the (yet empty) 
string to 8192 bytes. So alloc is always > 0, and the detach will trim 
alloc to len + 1.

However, Peff made another valid point and I'll send v2 soon.

-- 
Sebastian Schuberth

[PATCHv2] blame.c: Properly initialize strbuf after calling textconv_object(), again

[Sebastian Schuberth]

2564aa4 started to initialize buf.alloc, but that should actually be one
more byte than the string length due to the trailing \0. Also, do not
modify buf.alloc out of the strbuf code. Use the existing strbuf_attach
instead.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 builtin/blame.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 86c0537..80febbe 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2096,6 +2096,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	if (!contents_from || strcmp("-", contents_from)) {
 		struct stat st;
 		const char *read_from;
+		char *buf_ptr;
 		unsigned long buf_len;
 
 		if (contents_from) {
@@ -2113,10 +2114,8 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 		switch (st.st_mode & S_IFMT) {
 		case S_IFREG:
 			if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
-			    textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) {
-				buf.alloc = buf_len;
-				buf.len = buf_len;
-			}
+			    textconv_object(read_from, mode, null_sha1, &buf_ptr, &buf_len))
+				strbuf_attach(&buf, buf_ptr, buf_len, buf_len + 1);
 			else if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)
 				die_errno("cannot open or read '%s'", read_from);
 			break;
-- 
1.7.8.rc0.47.gf2c75.dirty

Re: [PATCHv2] blame.c: Properly initialize strbuf after calling textconv_object(), again

[Jeff King]

On Mon, Nov 07, 2011 at 06:33:34PM +0100, Sebastian Schuberth wrote:

> 2564aa4 started to initialize buf.alloc, but that should actually be one
> more byte than the string length due to the trailing \0. Also, do not
> modify buf.alloc out of the strbuf code. Use the existing strbuf_attach
> instead.

Heh. When I wrote the previous email I just made up the name
"strbuf_attach" as the opposite of "strbuf_detach". I didn't know it
actually already existed.

> -			    textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) {
> -				buf.alloc = buf_len;
> -				buf.len = buf_len;
> -			}
> +			    textconv_object(read_from, mode, null_sha1, &buf_ptr, &buf_len))
> +				strbuf_attach(&buf, buf_ptr, buf_len, buf_len + 1);

Looks OK to me.

-Peff