Skip to contentSkip to conversation

don't let mailmap provoke use of freed memory

PATCH In review Jim Meyering 4 participants 9 messages Jun 15, 2016

lore
Share & tools
lore

Jim Meyering

author
#0 lore raw

PATCH 1/1 don't let mailmap provoke use of freed memory

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

git init
printf '%s\n' \
'<[email protected]> <[email protected]>' \
'John <[email protected]>' > .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 <[email protected]>
---
 mailmap.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)
mailmap.c +6 -4
index f80b701..02fcfde 100644
@@ -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

#1 lore raw
Quoted 1 line · Jim Meyering
Signed-off-by Jim Meyering <[email protected]>

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

Thanks.

Signed-off-by Jonathan Nieder <[email protected]>
---
 t/t4203-mailmap.sh |   40 +++++++++++++++++++++++++++++++++++++---
 1 files changed, 37 insertions(+), 3 deletions(-)
t/t4203-mailmap.sh +37 -3
index 9a7d1b4..3c5188f 100755
@@ -54,7 +54,7 @@ Repo Guy (1):
 
 EOF
 test_expect_success 'mailmap.file set' '
-	mkdir internal_mailmap &&
+	mkdir -p internal_mailmap &&
 	echo "Internal Guy <[email protected]>" > 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 "<[email protected]> <[email protected]>" >internal_mailmap/.mailmap &&
+	echo "Internal Guy <[email protected]>" >>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 "<[email protected]> <[email protected]>" >internal_mailmap/.mailmap &&
+	echo "Internal Guy <[email protected]>" >>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 <[email protected]>" -m seventh &&
 
-	mkdir internal_mailmap &&
+	mkdir -p internal_mailmap &&
 	echo "Committed <[email protected]>" > internal_mailmap/.mailmap &&
 	echo "<[email protected]>                       <[email protected]>" >> internal_mailmap/.mailmap &&
 	echo "Some Dude <[email protected]>         nick1 <[email protected]>" >> internal_mailmap/.mailmap &&
Signature · 1 line
1.7.2.3
#3 lore raw
Quoted 8 lines · Ævar Arnfjörð Bjarmason

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.

#4 lore raw
Quoted 11 lines · Jonathan Nieder
Quoted 8 lines · Ævar Arnfjörð Bjarmason

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)
Quoted 3 lines

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 <[email protected]>

---

t/t4203-mailmap.sh +0 -1
index f4f82c0..3c5188f 100755
@@ -11,7 +11,6 @@ test_expect_success setup '
 	git commit -m initial &&
 	echo two >>one &&
 	git add one &&
-	test_tick &&
 	git commit --author "nick1 <[email protected]>" -m second
 '
#5 lore raw

PATCH 1/1 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 <[email protected]>

---

t/t4203-mailmap.sh test_expect_success setup ' · line 15

Quoted 9 lines · Jonathan Nieder
t/t4203-mailmap.sh +0 -1
@@ -11,7 +11,6 @@ test_expect_success setup '
 	git commit -m initial &&
 	echo two >>one &&
 	git add one &&
-	test_tick &&
 	git commit --author "nick1 <[email protected]>" -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(-)
t/t4203-mailmap.sh +17 -9
index 3c5188f..9198b30 100755
@@ -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
Signature · 1 line
1.7.2.3
#6 lore raw

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

Quoted 2 lines · Jonathan Nieder

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.

#7 lore raw

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

Quoted 6 lines · Junio C Hamano
Quoted 2 lines · Jonathan Nieder

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...

#8 lore raw

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

Quoted 9 lines · Junio C Hamano
Quoted 6 lines · Junio C Hamano
Quoted 2 lines · Jonathan Nieder

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.

Quoted 7 lines

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(-)
t/t4203-mailmap.sh +2 -2
index a267d07..e818de6 100755
@@ -114,7 +114,7 @@ test_expect_success 'name entry after email entry' '
 	mkdir -p internal_mailmap &&
 	echo "<[email protected]> <[email protected]>" >internal_mailmap/.mailmap &&
 	echo "Internal Guy <[email protected]>" >>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 "<[email protected]> <[email protected]>" >internal_mailmap/.mailmap &&
 	echo "Internal Guy <[email protected]>" >>internal_mailmap/.mailmap &&
-	git shortlog >actual &&
+	git shortlog HEAD >actual &&
 	test_cmp expect actual
 '