Skip to contentSkip to conversation

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

PATCH Superseded Sebastian Schuberth 3 participants 6 messages Jun 15, 2016

lore
Share & tools
lore

Sebastian Schuberth

author
#0 lore raw

PATCH 1/1 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.

---
 builtin/blame.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
builtin/blame.c +1 -1
index 86c0537..45f0dcc 100644
@@ -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)
Signature · 1 line
1.7.8.rc0.46.g5ae0f.dirty
#1 lore raw

builtin/blame.c static struct commit *fake_working_tree_commit(struct diff_options *opt, · line 2120

Quoted 11 lines · Sebastian Schuberth
builtin/blame.c +1 -1
@@ -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

author
#2 lore raw

PATCH v2 1/1 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 <[email protected]>
---
 builtin/blame.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)
builtin/blame.c +3 -4
index 86c0537..80febbe 100644
@@ -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;
Signature · 1 line
1.7.8.rc0.47.gf2c75.dirty
#3 lore raw
Quoted 4 lines · 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.

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.

builtin/blame.c static struct commit *fake_working_tree_commit(struct diff_options *opt, · line 2118

Quoted 6 lines
builtin/blame.c +2 -4
-			    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

#4 lore raw
In reply to Sebastian Schuberth
Quoted 2 lines · 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.

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

Quoted 1 line

---

Sign-off?

Quoted 16 lines
 builtin/blame.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
builtin/blame.c +1 -1
index 86c0537..45f0dcc 100644
@@ -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)
author
#5 lore raw
Quoted 4 lines · Junio C Hamano
Quoted 2 lines

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.

Signature · 1 line
Sebastian Schuberth