threads / patch / 28788

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

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

## tl;dr

8 messages between Oct 28, 2011 and Oct 31, 2011. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Sebastian Schuberth· Oct 28, 2011, 15:28 UTC · lore

For a plain string where only the length is known, strbuf.alloc needs to be initialized to the length. Otherwise strbuf.alloc is 0 and a later call to strbuf_setlen() will fail.

This bug surfaced when calling git blame under Windows on a *.doc file. The *.doc file is converted to plain text by antiword via the textconv mechanism. However, the plain text returned by antiword contains DOS line endings instead of Unix line endings which triggered the strbuf_setlen() which previous to this patch failed.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 builtin/blame.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Show changes to builtin/blame.c +3 −1
diff --git a/builtin/blame.c b/builtin/blame.c
index 26a5d42..86c0537 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2113,8 +2113,10 @@ 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))
+			    textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) {
+				buf.alloc = buf_len;
 				buf.len = buf_len;
+			}
 			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.7.msysgit.1.4.gcc6bb.dirty
Sebastian Schuberth· Oct 28, 2011, 16:32 UTC · re: Junio C Hamano · lore

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

On Fri, Oct 28, 2011 at 18:00, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks; do you have no addition to the test suite to demonstrate the
> breakage?
Not yet. I'll try to come up with something.
-- 
Sebastian Schuberth
Junio C Hamano· Oct 28, 2011, 16:44 UTC · re: Sebastian Schuberth · lore

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

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 6 quoted lines
> On Fri, Oct 28, 2011 at 18:00, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Thanks; do you have no addition to the test suite to demonstrate the
>> breakage?
>
> Not yet. I'll try to come up with something.
Let's do this.
Show changes to t/t8006-blame-textconv.sh +11 −0
diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
index 32ec82a..4ee42f1 100755
--- a/t/t8006-blame-textconv.sh
+++ b/t/t8006-blame-textconv.sh
@@ -15,6 +15,7 @@ EOF
 chmod +x helper
 
 test_expect_success 'setup ' '
+	echo "bin: test number 0" >zero.bin &&
 	echo "bin: test 1" >one.bin &&
 	echo "bin: test number 2" >two.bin &&
 	if test_have_prereq SYMLINKS; then
@@ -43,6 +44,7 @@ test_expect_success 'no filter specified' '
 
 test_expect_success 'setup textconv filters' '
 	echo "*.bin diff=test" >.gitattributes &&
+	echo "zero.bin eol=crlf" >>.gitattributes &&
 	git config diff.test.textconv ./helper &&
 	git config diff.test.cachetextconv false
 '
@@ -74,6 +76,15 @@ test_expect_success 'blame --textconv going through revisions' '
 	test_cmp expected result
 '
 
+test_expect_success 'blame --textconv with local changes' '
+	test_when_finished "git checkout zero.bin" &&
+	printf "bin: updated number 0\015" >zero.bin &&
+	git blame --textconv zero.bin >blame &&
+	expect="(Not Committed Yet ....-..-.. ..:..:.. +0000 1)" &&
+	expect="$expect converted: updated number 0" &&
+	expr "$(find_blame <blame)" : "^$expect"
+'
+
 test_expect_success 'setup +cachetextconv' '
 	git config diff.test.cachetextconv true
 '
Sebastian Schuberth· Oct 28, 2011, 17:17 UTC · re: Junio C Hamano · lore

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

On Fri, Oct 28, 2011 at 18:44, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
>>> Thanks; do you have no addition to the test suite to demonstrate the
>>> breakage?
>>
>> Not yet. I'll try to come up with something.
>
> Let's do this.

Thanks, but that does not seem to work for me. The test breaks both without and with my patch. I'll look into it.

-- 
Sebastian Schuberth
Junio C Hamano· Oct 28, 2011, 17:20 UTC · re: Sebastian Schuberth · lore

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

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 11 quoted lines
> On Fri, Oct 28, 2011 at 18:44, Junio C Hamano <gitster@pobox.com> wrote:
>
>>>> Thanks; do you have no addition to the test suite to demonstrate the
>>>> breakage?
>>>
>>> Not yet. I'll try to come up with something.
>>
>> Let's do this.
>
> Thanks, but that does not seem to work for me. The test breaks both
> without and with my patch. I'll look into it.

Thanks. I suspect the difference is because you are on a crlf-native platform while I am not...

Sebastian Schuberth· Oct 28, 2011, 18:36 UTC · re: Junio C Hamano · lore

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

On 28.10.2011 19:20, Junio C Hamano wrote:
Show 12 quoted lines
>>>>> Thanks; do you have no addition to the test suite to demonstrate the
>>>>> breakage?
>>>>
>>>> Not yet. I'll try to come up with something.
>>>
>>> Let's do this.
>>
>> Thanks, but that does not seem to work for me. The test breaks both
>> without and with my patch. I'll look into it.
> 
> Thanks. I suspect the difference is because you are on a crlf-native
> platform while I am not...
I also didn't have any luck. I've created a test that should fail without my patch, but it succeeds when running the test script. However, if I copy and paste the lines from the test to the command line, the test fails as expected ("blame" is empty). I'm out of ideas right now.
Show changes to t/t8006-blame-textconv.sh +15 −0
diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
index 32ec82a..4fee5aa 100755
--- a/t/t8006-blame-textconv.sh
+++ b/t/t8006-blame-textconv.sh
@@ -14,6 +14,13 @@ sed 's/^bin: /converted: /' "$1"
 EOF
 chmod +x helper
 
+cat >helper-dos-line-endings <<'EOF'
+#!/bin/sh
+grep -q '^bin: ' "$1" || { echo "E: $1 is not \"binary\" file" 1>&2; exit 1; }
+sed 's/^bin: \(.*\)$/converted: \1\r/' "$1"
+EOF
+chmod +x helper-dos-line-endings
+
 test_expect_success 'setup ' '
 	echo "bin: test 1" >one.bin &&
 	echo "bin: test number 2" >two.bin &&
@@ -74,6 +81,14 @@ test_expect_success 'blame --textconv going through revisions' '
 	test_cmp expected result
 '
 
+test_expect_success 'blame --textconv with DOS line endings' '
+	git config diff.test.textconv ./helper-dos-line-endings &&
+	git blame --textconv two.bin >blame &&
+	git config diff.test.textconv ./helper &&
+	find_blame <blame >result &&
+	test_cmp expected result
+'
+
 test_expect_success 'setup +cachetextconv' '
 	git config diff.test.cachetextconv true
 '

-- 
Sebastian Schuberth
Sebastian Schuberth· Oct 31, 2011, 17:57 UTC · re: Junio C Hamano · lore

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

On Fri, Oct 28, 2011 at 19:20, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
>>>>> Thanks; do you have no addition to the test suite to demonstrate the
>>>>> breakage?
>>>>
>>>> Not yet. I'll try to come up with something.
>>>
>>> Let's do this.
>>
>> Thanks, but that does not seem to work for me. The test breaks both
>> without and with my patch. I'll look into it.
>
> Thanks. I suspect the difference is because you are on a crlf-native
> platform while I am not...

I've got a test now. However, that test revealed some more related issues. I'll come up with a v2 of the patch this week.

-- 
Sebastian Schuberth

← back to recent threads