threads / patch / 25418

patchdon't let mailmap provoke use of freed memory

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

## tl;dr

9 messages between Oct 11, 2010 and Oct 20, 2010. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Jim Meyering· Oct 11, 2010, 15:41 UTC · lore
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(-)
Show changes to mailmap.c +6 −5
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
Jonathan Nieder· Oct 11, 2010, 16:21 UTC · re: Jim Meyering · lore

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

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(-)
Show changes to t/t4203-mailmap.sh +37 −3
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
Ævar Arnfjörð Bjarmason· Oct 15, 2010, 05:22 UTC · re: Jonathan Nieder · lore

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

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.
Jonathan Nieder· Oct 15, 2010, 06:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason wrote:
Show 8 quoted lines
> 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.

Jonathan Nieder· Oct 15, 2010, 07:59 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder wrote:
> Ævar Arnfjörð Bjarmason wrote:
Show 12 quoted lines
>> 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>
---
Show changes to t/t4203-mailmap.sh +0 −1
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
 '
 
Jonathan Nieder· Oct 15, 2010, 17:12 UTC · re: Jonathan Nieder · lore

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

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:
Show 9 quoted lines
> --- 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(-)
Show changes to t/t4203-mailmap.sh +17 −9
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
Junio C Hamano· Oct 17, 2010, 04:43 UTC · re: Jonathan Nieder · lore

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

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.
Junio C Hamano· Oct 20, 2010, 06:29 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> 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...
Junio C Hamano· Oct 20, 2010, 06:31 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
> 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(-)
Show changes to t/t4203-mailmap.sh +2 −2
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
 '
 

← back to recent threads