[PATCH next] don't let mailmap provoke use of freed memory

[PATCH next] don't let mailmap provoke use of freed memory

[Jim Meyering]

On an x86_64 system (F13-based), I ran these commands in an empty directory:

    git init
    printf '%s\n' \
      '<jdoe@example.com> <jdoe@example.COM>' \
      'John <jdoe@example.com>' > .mailmap
    git shortlog < /dev/null

Here's the result:

    (reading log message from standard input)
    *** glibc detected *** git: free(): invalid pointer: 0x0000000000f53730 ***
    ======= Backtrace: =========
    /lib64/libc.so.6[0x31ba875676]
    git[0x48c2a5]
    git[0x4b9858]
    ...
    zsh: abort (core dumped)  git shortlog

What happened?

Some .mailmap entry is of the <email1> <email2> form,
while a subsequent one looks like "User Name <Email2>,
and the two email addresses on the right are not identical
but are "equal" when using a case-insensitive comparator.

Then, when add_mapping is processing the latter line, new_email is NULL
and we free me->email, yet do not replace it with a new strdup'd string.
Thus, when later we attempt to use the buffer behind that ->email pointer,
we reference freed memory.

The solution is to free ->email and ->name only if we're about to replace them.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 mailmap.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index f80b701..02fcfde 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -79,12 +79,14 @@ static void add_mapping(struct string_list *map,
 	if (old_name == NULL) {
 		debug_mm("mailmap: adding (simple) entry for %s at index %d\n", old_email, index);
 		/* Replace current name and new email for simple entry */
-		free(me->name);
-		free(me->email);
-		if (new_name)
+		if (new_name) {
+			free(me->name);
 			me->name = xstrdup(new_name);
-		if (new_email)
+		}
+		if (new_email) {
+			free(me->email);
 			me->email = xstrdup(new_email);
+		}
 	} else {
 		struct mailmap_info *mi = xmalloc(sizeof(struct mailmap_info));
 		debug_mm("mailmap: adding (complex) entry for %s at index %d\n", old_email, index);
--
1.7.3.1.104.gc752e

Re: [PATCH next] don't let mailmap provoke use of freed memory

[Jonathan Nieder]

Jim Meyering wrote:

> Signed-off-by: Jim Meyering <meyering@redhat.com>

Applies to maint, too --- confirmed with the tests below
(both fail before, pass after).

Thanks.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4203-mailmap.sh |   40 +++++++++++++++++++++++++++++++++++++---
 1 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 9a7d1b4..3c5188f 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -54,7 +54,7 @@ Repo Guy (1):
 
 EOF
 test_expect_success 'mailmap.file set' '
-	mkdir internal_mailmap &&
+	mkdir -p internal_mailmap &&
 	echo "Internal Guy <bugs@company.xx>" > internal_mailmap/.mailmap &&
 	git config mailmap.file internal_mailmap/.mailmap &&
 	git shortlog HEAD >actual &&
@@ -93,6 +93,40 @@ test_expect_success 'mailmap.file non-existant' '
 '
 
 cat >expect <<\EOF
+Internal Guy (1):
+      second
+
+Repo Guy (1):
+      initial
+
+EOF
+
+test_expect_success 'name entry after email entry' '
+	mkdir -p internal_mailmap &&
+	echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
+	echo "Internal Guy <bugs@company.xx>" >>internal_mailmap/.mailmap &&
+	git shortlog >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<\EOF
+Internal Guy (1):
+      second
+
+Repo Guy (1):
+      initial
+
+EOF
+
+test_expect_success 'name entry after email entry, case-insensitive' '
+	mkdir -p internal_mailmap &&
+	echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
+	echo "Internal Guy <BUGS@Company.xx>" >>internal_mailmap/.mailmap &&
+	git shortlog >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<\EOF
 A U Thor (1):
       initial
 
@@ -101,7 +135,7 @@ nick1 (1):
 
 EOF
 test_expect_success 'No mailmap files, but configured' '
-	rm .mailmap &&
+	rm -f .mailmap internal_mailmap/.mailmap &&
 	git shortlog HEAD >actual &&
 	test_cmp expect actual
 '
@@ -153,7 +187,7 @@ test_expect_success 'Shortlog output (complex mapping)' '
 	test_tick &&
 	git commit --author "CTO <cto@coompany.xx>" -m seventh &&
 
-	mkdir internal_mailmap &&
+	mkdir -p internal_mailmap &&
 	echo "Committed <committer@example.com>" > internal_mailmap/.mailmap &&
 	echo "<cto@company.xx>                       <cto@coompany.xx>" >> internal_mailmap/.mailmap &&
 	echo "Some Dude <some@dude.xx>         nick1 <bugs@company.xx>" >> internal_mailmap/.mailmap &&
-- 
1.7.2.3

Re: [PATCH next] don't let mailmap provoke use of freed memory

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 11, 2010 at 16:21, Jonathan Nieder <jrnieder@gmail.com> wrote:

> Applies to maint, too --- confirmed with the tests below
> (both fail before, pass after).

Both the smokers trast and I run have started to fail on pu with these tests:

    http://smoke.git.nix.is/app/projects/tap_stream/172/260
    http://smoke.git.nix.is/app/projects/tap_stream/170/260

I haven't investigated it. Maybe it's something else.

Re: [PATCH next] don't let mailmap provoke use of freed memory

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> ok 6 - mailmap.file non-existant
> not ok - 7 name entry after email entry
> not ok - 8 name entry after email entry, case-insensitive
> ok 9 - No mailmap files, but configured
> ok 10 - Shortlog output (complex mapping)
> ok 11 - Log output (complex mapping)
> not ok - 12 Blame output (complex mapping)
> # failed 3 among 12 test(s)

Odd.  I can reproduce test 12 failing with commit ids changed
(embarrassed I didn't notice before) but the others pass here.
Trying a --valgrind run.

Re: [PATCH next] don't let mailmap provoke use of freed memory

[Jonathan Nieder]

Jonathan Nieder wrote:
> Ævar Arnfjörð Bjarmason wrote:

>> ok 6 - mailmap.file non-existant
>> not ok - 7 name entry after email entry
>> not ok - 8 name entry after email entry, case-insensitive
>> ok 9 - No mailmap files, but configured
>> ok 10 - Shortlog output (complex mapping)
>> ok 11 - Log output (complex mapping)
>> not ok - 12 Blame output (complex mapping)
>> # failed 3 among 12 test(s)
>
> Odd.  I can reproduce test 12 failing with commit ids changed
> (embarrassed I didn't notice before) but the others pass here.
> Trying a --valgrind run.

Well, this is embarrasing.  Here's a minimal fix for the test #12
failure (for squashing --- it just undoes a change that should never
have escaped the lab in the first place).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index f4f82c0..3c5188f 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -11,7 +11,6 @@ test_expect_success setup '
 	git commit -m initial &&
 	echo two >>one &&
 	git add one &&
-	test_tick &&
 	git commit --author "nick1 <bugs@company.xx>" -m second
 '
 
-- 

[PATCH] t4203 (mailmap): stop hardcoding commit ids and dates

[Jonathan Nieder]

A seemingly innocuous change like adding test_tick somewhere can
completely upset the final mailmap test, since it checks commit
hashes and dates.  Make the test less fragile by fuzzing away the
unpredictable parts and leaving in the authors (which is what the
test is about, anyway).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jonathan Nieder wrote:

> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -11,7 +11,6 @@ test_expect_success setup '
>  	git commit -m initial &&
>  	echo two >>one &&
>  	git add one &&
> -	test_tick &&
>  	git commit --author "nick1 <bugs@company.xx>" -m second
>  '

Here's a better fix.  Still no idea about the other failures Ævar
and Thomas were seeing.

 t/t4203-mailmap.sh |   26 +++++++++++++++++---------
 1 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 3c5188f..9198b30 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -4,6 +4,14 @@ test_description='.mailmap configurations'
 
 . ./test-lib.sh
 
+fuzz_blame () {
+	sed "
+		s/$_x05[0-9a-f][0-9a-f][0-9a-f]/OBJID/g
+		s/$_x05[0-9a-f][0-9a-f]/OBJI/g
+		s/[-0-9]\{10\} [:0-9]\{8\} [-+][0-9]\{4\}/DATE/g
+	" "$@"
+}
+
 test_expect_success setup '
 	echo one >one &&
 	git add one &&
@@ -232,18 +240,18 @@ test_expect_success 'Log output (complex mapping)' '
 
 # git blame
 cat >expect <<\EOF
-^3a2fdcb (A U Thor     2005-04-07 15:13:13 -0700 1) one
-7de6f99b (Some Dude    2005-04-07 15:13:13 -0700 2) two
-5815879d (Other Author 2005-04-07 15:14:13 -0700 3) three
-ff859d96 (Other Author 2005-04-07 15:15:13 -0700 4) four
-5ab6d4fa (Santa Claus  2005-04-07 15:16:13 -0700 5) five
-38a42d8b (Santa Claus  2005-04-07 15:17:13 -0700 6) six
-8ddc0386 (CTO          2005-04-07 15:18:13 -0700 7) seven
+^OBJI (A U Thor     DATE 1) one
+OBJID (Some Dude    DATE 2) two
+OBJID (Other Author DATE 3) three
+OBJID (Other Author DATE 4) four
+OBJID (Santa Claus  DATE 5) five
+OBJID (Santa Claus  DATE 6) six
+OBJID (CTO          DATE 7) seven
 EOF
-
 test_expect_success 'Blame output (complex mapping)' '
 	git blame one >actual &&
-	test_cmp expect actual
+	fuzz_blame actual >actual.fuzz &&
+	test_cmp expect actual.fuzz
 '
 
 test_done
-- 
1.7.2.3

Re: [PATCH] t4203 (mailmap): stop hardcoding commit ids and dates

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Here's a better fix.  Still no idea about the other failures Ævar
> and Thomas were seeing.

I saw intermittent failures too.  Thanks for the objid fix.

Re: [PATCH] t4203 (mailmap): stop hardcoding commit ids and dates

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Here's a better fix.  Still no idea about the other failures Ævar
>> and Thomas were seeing.
>
> I saw intermittent failures too.  Thanks for the objid fix.

I found a way to reliably make this fail at test #7.

 $ sh t4203-mailmap.sh </dev/null

Without the input redirection, things seem to work Ok.

Still puzzled...

Re: [PATCH] t4203 (mailmap): stop hardcoding commit ids and dates

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>> Here's a better fix.  Still no idea about the other failures Ævar
>>> and Thomas were seeing.
>>
>> I saw intermittent failures too.  Thanks for the objid fix.
>
> I found a way to reliably make this fail at test #7.
>
>  $ sh t4203-mailmap.sh </dev/null
>
> Without the input redirection, things seem to work Ok.
>
> Still puzzled...

Ah, of course.

 t/t4203-mailmap.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index a267d07..e818de6 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -114,7 +114,7 @@ test_expect_success 'name entry after email entry' '
 	mkdir -p internal_mailmap &&
 	echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
 	echo "Internal Guy <bugs@company.xx>" >>internal_mailmap/.mailmap &&
-	git shortlog >actual &&
+	git shortlog HEAD >actual &&
 	test_cmp expect actual
 '
 
@@ -131,7 +131,7 @@ test_expect_success 'name entry after email entry, case-insensitive' '
 	mkdir -p internal_mailmap &&
 	echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
 	echo "Internal Guy <BUGS@Company.xx>" >>internal_mailmap/.mailmap &&
-	git shortlog >actual &&
+	git shortlog HEAD >actual &&
 	test_cmp expect actual
 '