{"thread":{"id":"25418","subject":"[PATCH next] don't let mailmap provoke use of freed memory","startedAt":"2010-10-11T15:41:16Z","lastAt":"2010-10-20T06:31:52Z","messageCount":9,"participants":["Jim Meyering","Jonathan Nieder","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"153239","messageId":"87tyksd9er.fsf@meyering.net","threadId":"25418","inReplyTo":null,"subject":"[PATCH next] don't let mailmap provoke use of freed memory","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2010-10-11T15:41:16Z","receivedAt":"2010-10-11T15:41:16Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"\nOn an x86_64 system (F13-based), I ran these commands in an empty directory:\n\n git init\n printf '%s\\n' \\\n ' ' \\\n 'John ' > .mailmap\n git shortlog < /dev/null\n\nHere's the result:\n\n (reading log message from standard input)\n *** glibc detected *** git: free(): invalid pointer: 0x0000000000f53730 ***\n ======= Backtrace: =========\n /lib64/libc.so.6[0x31ba875676]\n git[0x48c2a5]\n git[0x4b9858]\n ...\n zsh: abort (core dumped) git shortlog\n\nWhat happened?\n\nSome .mailmap entry is of the form,\nwhile a subsequent one looks like \"User Name ,\nand the two email addresses on the right are not identical\nbut are \"equal\" when using a case-insensitive comparator.\n\nThen, when add_mapping is processing the latter line, new_email is NULL\nand we free me->email, yet do not replace it with a new strdup'd string.\nThus, when later we attempt to use the buffer behind that ->email pointer,\nwe reference freed memory.\n\nThe solution is to free ->email and ->name only if we're about to replace them.\n\nSigned-off-by: Jim Meyering \n---\n mailmap.c | 10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex f80b701..02fcfde 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -79,12 +79,14 @@ static void add_mapping(struct string_list *map,\n \tif (old_name == NULL) {\n \t\tdebug_mm(\"mailmap: adding (simple) entry for %s at index %d\\n\", old_email, index);\n \t\t/* Replace current name and new email for simple entry */\n-\t\tfree(me->name);\n-\t\tfree(me->email);\n-\t\tif (new_name)\n+\t\tif (new_name) {\n+\t\t\tfree(me->name);\n \t\t\tme->name = xstrdup(new_name);\n-\t\tif (new_email)\n+\t\t}\n+\t\tif (new_email) {\n+\t\t\tfree(me->email);\n \t\t\tme->email = xstrdup(new_email);\n+\t\t}\n \t} else {\n \t\tstruct mailmap_info *mi = xmalloc(sizeof(struct mailmap_info));\n \t\tdebug_mm(\"mailmap: adding (complex) entry for %s at index %d\\n\", old_email, index);\n--\n1.7.3.1.104.gc752e\n"},{"id":"153248","messageId":"20101011162153.GG25842@burratino","threadId":"25418","inReplyTo":"87tyksd9er.fsf@meyering.net","subject":"Re: [PATCH next] don't let mailmap provoke use of freed memory","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-11T16:21:53Z","receivedAt":"2010-10-11T16:21:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jim Meyering wrote:\n\n> Signed-off-by: Jim Meyering \n\nApplies to maint, too --- confirmed with the tests below\n(both fail before, pass after).\n\nThanks.\n\nSigned-off-by: Jonathan Nieder \n---\n t/t4203-mailmap.sh | 40 +++++++++++++++++++++++++++++++++++++---\n 1 files changed, 37 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex 9a7d1b4..3c5188f 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -54,7 +54,7 @@ Repo Guy (1):\n \n EOF\n test_expect_success 'mailmap.file set' '\n-\tmkdir internal_mailmap &&\n+\tmkdir -p internal_mailmap &&\n \techo \"Internal Guy \" > internal_mailmap/.mailmap &&\n \tgit config mailmap.file internal_mailmap/.mailmap &&\n \tgit shortlog HEAD >actual &&\n@@ -93,6 +93,40 @@ test_expect_success 'mailmap.file non-existant' '\n '\n \n cat >expect <<\\EOF\n+Internal Guy (1):\n+ second\n+\n+Repo Guy (1):\n+ initial\n+\n+EOF\n+\n+test_expect_success 'name entry after email entry' '\n+\tmkdir -p internal_mailmap &&\n+\techo \" \" >internal_mailmap/.mailmap &&\n+\techo \"Internal Guy \" >>internal_mailmap/.mailmap &&\n+\tgit shortlog >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<\\EOF\n+Internal Guy (1):\n+ second\n+\n+Repo Guy (1):\n+ initial\n+\n+EOF\n+\n+test_expect_success 'name entry after email entry, case-insensitive' '\n+\tmkdir -p internal_mailmap &&\n+\techo \" \" >internal_mailmap/.mailmap &&\n+\techo \"Internal Guy \" >>internal_mailmap/.mailmap &&\n+\tgit shortlog >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<\\EOF\n A U Thor (1):\n initial\n \n@@ -101,7 +135,7 @@ nick1 (1):\n \n EOF\n test_expect_success 'No mailmap files, but configured' '\n-\trm .mailmap &&\n+\trm -f .mailmap internal_mailmap/.mailmap &&\n \tgit shortlog HEAD >actual &&\n \ttest_cmp expect actual\n '\n@@ -153,7 +187,7 @@ test_expect_success 'Shortlog output (complex mapping)' '\n \ttest_tick &&\n \tgit commit --author \"CTO \" -m seventh &&\n \n-\tmkdir internal_mailmap &&\n+\tmkdir -p internal_mailmap &&\n \techo \"Committed \" > internal_mailmap/.mailmap &&\n \techo \" \" >> internal_mailmap/.mailmap &&\n \techo \"Some Dude nick1 \" >> internal_mailmap/.mailmap &&\n-- \n1.7.2.3\n"},{"id":"153559","messageId":"AANLkTimpN5CAC5UbhC2Kydhu_BMouriQn1d6OL5vt09m@mail.gmail.com","threadId":"25418","inReplyTo":"20101011162153.GG25842@burratino","subject":"Re: [PATCH next] don't let mailmap provoke use of freed memory","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-10-15T05:22:21Z","receivedAt":"2010-10-15T05:22:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Oct 11, 2010 at 16:21, Jonathan Nieder wrote:\n\n> Applies to maint, too --- confirmed with the tests below\n> (both fail before, pass after).\n\nBoth the smokers trast and I run have started to fail on pu with these tests:\n\n http://smoke.git.nix.is/app/projects/tap_stream/172/260\n http://smoke.git.nix.is/app/projects/tap_stream/170/260\n\nI haven't investigated it. Maybe it's something else.\n"},{"id":"153565","messageId":"20101015061821.GE21830@burratino","threadId":"25418","inReplyTo":"AANLkTimpN5CAC5UbhC2Kydhu_BMouriQn1d6OL5vt09m@mail.gmail.com","subject":"Re: [PATCH next] don't let mailmap provoke use of freed memory","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-15T06:18:21Z","receivedAt":"2010-10-15T06:18:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n\n> ok 6 - mailmap.file non-existant\n> not ok - 7 name entry after email entry\n> not ok - 8 name entry after email entry, case-insensitive\n> ok 9 - No mailmap files, but configured\n> ok 10 - Shortlog output (complex mapping)\n> ok 11 - Log output (complex mapping)\n> not ok - 12 Blame output (complex mapping)\n> # failed 3 among 12 test(s)\n\nOdd. I can reproduce test 12 failing with commit ids changed\n(embarrassed I didn't notice before) but the others pass here.\nTrying a --valgrind run.\n"},{"id":"153568","messageId":"20101015075954.GA8557@burratino","threadId":"25418","inReplyTo":"20101015061821.GE21830@burratino","subject":"Re: [PATCH next] don't let mailmap provoke use of freed memory","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-15T07:59:54Z","receivedAt":"2010-10-15T07:59:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Ævar Arnfjörð Bjarmason wrote:\n\n>> ok 6 - mailmap.file non-existant\n>> not ok - 7 name entry after email entry\n>> not ok - 8 name entry after email entry, case-insensitive\n>> ok 9 - No mailmap files, but configured\n>> ok 10 - Shortlog output (complex mapping)\n>> ok 11 - Log output (complex mapping)\n>> not ok - 12 Blame output (complex mapping)\n>> # failed 3 among 12 test(s)\n>\n> Odd. I can reproduce test 12 failing with commit ids changed\n> (embarrassed I didn't notice before) but the others pass here.\n> Trying a --valgrind run.\n\nWell, this is embarrasing. Here's a minimal fix for the test #12\nfailure (for squashing --- it just undoes a change that should never\nhave escaped the lab in the first place).\n\nSigned-off-by: Jonathan Nieder \n---\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex f4f82c0..3c5188f 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -11,7 +11,6 @@ test_expect_success setup '\n \tgit commit -m initial &&\n \techo two >>one &&\n \tgit add one &&\n-\ttest_tick &&\n \tgit commit --author \"nick1 \" -m second\n '\n \n-- \n"},{"id":"153591","messageId":"20101015171219.GC10204@burratino","threadId":"25418","inReplyTo":"20101015075954.GA8557@burratino","subject":"[PATCH] t4203 (mailmap): stop hardcoding commit ids and dates","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-15T17:12:19Z","receivedAt":"2010-10-15T17:12:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"A seemingly innocuous change like adding test_tick somewhere can\ncompletely upset the final mailmap test, since it checks commit\nhashes and dates. Make the test less fragile by fuzzing away the\nunpredictable parts and leaving in the authors (which is what the\ntest is about, anyway).\n\nSigned-off-by: Jonathan Nieder \n---\nJonathan Nieder wrote:\n\n> --- a/t/t4203-mailmap.sh\n> +++ b/t/t4203-mailmap.sh\n> @@ -11,7 +11,6 @@ test_expect_success setup '\n> \tgit commit -m initial &&\n> \techo two >>one &&\n> \tgit add one &&\n> -\ttest_tick &&\n> \tgit commit --author \"nick1 \" -m second\n> '\n\nHere's a better fix. Still no idea about the other failures Ævar\nand Thomas were seeing.\n\n t/t4203-mailmap.sh | 26 +++++++++++++++++---------\n 1 files changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex 3c5188f..9198b30 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -4,6 +4,14 @@ test_description='.mailmap configurations'\n \n . ./test-lib.sh\n \n+fuzz_blame () {\n+\tsed \"\n+\t\ts/$_x05[0-9a-f][0-9a-f][0-9a-f]/OBJID/g\n+\t\ts/$_x05[0-9a-f][0-9a-f]/OBJI/g\n+\t\ts/[-0-9]\\{10\\} [:0-9]\\{8\\} [-+][0-9]\\{4\\}/DATE/g\n+\t\" \"$@\"\n+}\n+\n test_expect_success setup '\n \techo one >one &&\n \tgit add one &&\n@@ -232,18 +240,18 @@ test_expect_success 'Log output (complex mapping)' '\n \n # git blame\n cat >expect <<\\EOF\n-^3a2fdcb (A U Thor 2005-04-07 15:13:13 -0700 1) one\n-7de6f99b (Some Dude 2005-04-07 15:13:13 -0700 2) two\n-5815879d (Other Author 2005-04-07 15:14:13 -0700 3) three\n-ff859d96 (Other Author 2005-04-07 15:15:13 -0700 4) four\n-5ab6d4fa (Santa Claus 2005-04-07 15:16:13 -0700 5) five\n-38a42d8b (Santa Claus 2005-04-07 15:17:13 -0700 6) six\n-8ddc0386 (CTO 2005-04-07 15:18:13 -0700 7) seven\n+^OBJI (A U Thor DATE 1) one\n+OBJID (Some Dude DATE 2) two\n+OBJID (Other Author DATE 3) three\n+OBJID (Other Author DATE 4) four\n+OBJID (Santa Claus DATE 5) five\n+OBJID (Santa Claus DATE 6) six\n+OBJID (CTO DATE 7) seven\n EOF\n-\n test_expect_success 'Blame output (complex mapping)' '\n \tgit blame one >actual &&\n-\ttest_cmp expect actual\n+\tfuzz_blame actual >actual.fuzz &&\n+\ttest_cmp expect actual.fuzz\n '\n \n test_done\n-- \n1.7.2.3\n"},{"id":"153656","messageId":"7vpqv95szu.fsf@alter.siamese.dyndns.org","threadId":"25418","inReplyTo":"20101015171219.GC10204@burratino","subject":"Re: [PATCH] t4203 (mailmap): stop hardcoding commit ids and dates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-17T04:43:33Z","receivedAt":"2010-10-17T04:43:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Here's a better fix. Still no idea about the other failures Ævar\n> and Thomas were seeing.\n\nI saw intermittent failures too. Thanks for the objid fix.\n"},{"id":"153853","messageId":"7vsk01wf65.fsf@alter.siamese.dyndns.org","threadId":"25418","inReplyTo":"7vpqv95szu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t4203 (mailmap): stop hardcoding commit ids and dates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-20T06:29:06Z","receivedAt":"2010-10-20T06:29:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Nieder writes:\n>\n>> Here's a better fix. Still no idea about the other failures Ævar\n>> and Thomas were seeing.\n>\n> I saw intermittent failures too. Thanks for the objid fix.\n\nI found a way to reliably make this fail at test #7.\n\n $ sh t4203-mailmap.sh writes:\n\n> Junio C Hamano writes:\n>\n>> Jonathan Nieder writes:\n>>\n>>> Here's a better fix. Still no idea about the other failures Ævar\n>>> and Thomas were seeing.\n>>\n>> I saw intermittent failures too. Thanks for the objid fix.\n>\n> I found a way to reliably make this fail at test #7.\n>\n> $ sh t4203-mailmap.sh \n> Without the input redirection, things seem to work Ok.\n>\n> Still puzzled...\n\nAh, of course.\n\n t/t4203-mailmap.sh | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex a267d07..e818de6 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -114,7 +114,7 @@ test_expect_success 'name entry after email entry' '\n \tmkdir -p internal_mailmap &&\n \techo \" \" >internal_mailmap/.mailmap &&\n \techo \"Internal Guy \" >>internal_mailmap/.mailmap &&\n-\tgit shortlog >actual &&\n+\tgit shortlog HEAD >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -131,7 +131,7 @@ test_expect_success 'name entry after email entry, case-insensitive' '\n \tmkdir -p internal_mailmap &&\n \techo \" \" >internal_mailmap/.mailmap &&\n \techo \"Internal Guy \" >>internal_mailmap/.mailmap &&\n-\tgit shortlog >actual &&\n+\tgit shortlog HEAD >actual &&\n \ttest_cmp expect actual\n '\n \n"}]}