threads / patch / 28876

patchblame.c: Properly initialize strbuf after calling textconv_object(), again

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

## tl;dr

6 messages between Nov 7, 2011 and Nov 7, 2011. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Sebastian Schuberth· Nov 7, 2011, 15:53 UTC · lore
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(-)
Show changes to builtin/blame.c +1 −1
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
Junio C Hamano· Nov 7, 2011, 16:47 UTC · re: Sebastian Schuberth · lore

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

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?
Show 16 quoted lines
>  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)
Sebastian Schuberth· Nov 7, 2011, 17:31 UTC · re: Junio C Hamano · lore

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

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
Jeff King· Nov 7, 2011, 16:49 UTC · re: Sebastian Schuberth · lore

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

On Mon, Nov 07, 2011 at 04:53:10PM +0100, Sebastian Schuberth wrote:
Show 11 quoted lines
> --- 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
Sebastian Schuberth· Nov 7, 2011, 17:33 UTC · re: Jeff King · lore

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

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(-)
Show changes to builtin/blame.c +3 −4
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
Jeff King· Nov 7, 2011, 17:41 UTC · re: Sebastian Schuberth · lore

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

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.

Show 6 quoted lines
> -			    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

← back to recent threads