{"thread":{"id":"34763","subject":"[PATCH] mailmap: handle mailmap blobs without trailing newlines","startedAt":"2013-08-25T08:45:51Z","lastAt":"2013-08-25T09:11:00Z","messageCount":3,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"225846","messageId":"20130825084549.GA5912@sigill.intra.peff.net","threadId":"34763","inReplyTo":null,"subject":"[PATCH] mailmap: handle mailmap blobs without trailing newlines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-25T08:45:51Z","receivedAt":"2013-08-25T08:45:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The read_mailmap_buf function reads each line of the mailmap\nusing strchrnul, like:\n\n const char *end = strchrnul(buf, '\\n');\n unsigned long linelen = end - buf + 1;\n\nBut that's off-by-one when we actually hit the NUL byte; our\nline does not have a terminator, and so is only \"end - buf\"\nbytes long. As a result, when we subtract the linelen from\nthe total len, we end up with (unsigned long)-1 bytes left\nin the buffer, and we start reading random junk from memory.\n\nWe could fix it with:\n\n unsigned long linelen = end - buf + !!*end;\n\nbut it is questionable for a function called read_mailmap_buf\nto be using strchrnul in the first place. It happens to work\nbecause our buffers come from blobs, and read_sha1_file always\nNUL-terminates the object data. But let's future-proof the\nfunction by actually handling non-terminated strings\ncorrectly, and fix the off-by-one at the same time.\n\nSigned-off-by: Jeff King \n---\nIntended for 'maint'. The bug was introduced in 0861090, but I built the\nfix on top of 8c473ce, the tip of the jk/mailmap-from-blob topic, as it\navoids annoying textual conflicts in the test script.\n\nv1.8.2 was the first version with the bug, so this is not an \"oops, we\nfailed to find this new bug during v1.8.4-rc series\" problem. I found it\nnow because I turned on mailmap.blob for all of github.com, which\nexposed the code to a much larger array of random inputs.\n\nThis is the minimal fix. Another option would be to switch\nread_mailmap_buf to read_mailmap_string, and I think we could even get\naway with avoiding the extra allocation/copy in the loop (because\nread_mailmap_line seems to cope with newline-or-EOS just fine). But it\nmay be better to save that for 'master'.\n\n mailmap.c | 12 +++++++++---\n t/t4203-mailmap.sh | 16 +++++++++++++++-\n 2 files changed, 24 insertions(+), 4 deletions(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex b16542f..a635873 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -192,10 +192,16 @@ static void read_mailmap_buf(struct string_list *map,\n \t\t\t char **repo_abbrev)\n {\n \twhile (len) {\n-\t\tconst char *end = strchrnul(buf, '\\n');\n-\t\tunsigned long linelen = end - buf + 1;\n-\t\tchar *line = xmemdupz(buf, linelen);\n+\t\tconst char *end = memchr(buf, '\\n', len);\n+\t\tunsigned long linelen;\n+\t\tchar *line;\n \n+\t\tif (end)\n+\t\t\tlinelen = end - buf + 1;\n+\t\telse\n+\t\t\tlinelen = len;\n+\n+\t\tline = xmemdupz(buf, linelen);\n \t\tread_mailmap_line(map, line, repo_abbrev);\n \n \t\tfree(line);\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex aae30d9..10c7b12 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -159,7 +159,8 @@ test_expect_success 'setup mailmap blob tests' '\n \tBlob Guy \n \tBlob Guy \n \tEOF\n-\tgit add just-bugs both &&\n+\tprintf \"Tricky Guy \" >no-newline &&\n+\tgit add just-bugs both no-newline &&\n \tgit commit -m \"my mailmaps\" &&\n \techo \"Repo Guy \" >.mailmap &&\n \techo \"Internal Guy \" >internal.map\n@@ -243,6 +244,19 @@ test_expect_success 'mailmap.blob defaults to HEAD:.mailmap in bare repo' '\n \t)\n '\n \n+test_expect_success 'mailmap.blob can handle blobs without trailing newline' '\n+\tcat >expect <<-\\EOF &&\n+\tTricky Guy (1):\n+\t initial\n+\n+\tnick1 (1):\n+\t second\n+\n+\tEOF\n+\tgit -c mailmap.blob=map:no-newline shortlog HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cleanup after mailmap.blob tests' '\n \trm -f .mailmap\n '\n-- \n1.8.4.2.g87d4a77\n"},{"id":"225848","messageId":"20130825085500.GA6089@sigill.intra.peff.net","threadId":"34763","inReplyTo":"20130825084549.GA5912@sigill.intra.peff.net","subject":"Re: [PATCH] mailmap: handle mailmap blobs without trailing newlines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-25T08:55:00Z","receivedAt":"2013-08-25T08:55:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 25, 2013 at 04:45:50AM -0400, Jeff King wrote:\n\n> This is the minimal fix. Another option would be to switch\n> read_mailmap_buf to read_mailmap_string, and I think we could even get\n> away with avoiding the extra allocation/copy in the loop (because\n> read_mailmap_line seems to cope with newline-or-EOS just fine). But it\n> may be better to save that for 'master'.\n\nHmm, actually, this isn't quite true. read_mailmap_line does handle the\noptional trailing newline properly, but the underlying parsing routines\nreally do want to see a NUL at the end of each line (because they came\nfrom code that just calls fgets). So we really do want to tie off each\nline. But given that our only caller is handing us blob contents which\nget immediately freed, we could still do that without an extra\nallocation if:\n\n 1. We make it clear that the input must be NUL-terminated (i.e., by\n renaming the function and dropping the len parameter).\n\n 2. We drop the \"const\" from the buf parameter so that we can simply\n terminate each line as we go.\n\nI'll see what the patch looks like.\n\n-Peff\n"},{"id":"225849","messageId":"20130825091100.GB6089@sigill.intra.peff.net","threadId":"34763","inReplyTo":"20130825085500.GA6089@sigill.intra.peff.net","subject":"Re: [PATCH] mailmap: handle mailmap blobs without trailing newlines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-25T09:11:00Z","receivedAt":"2013-08-25T09:11:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 25, 2013 at 04:55:00AM -0400, Jeff King wrote:\n\n> On Sun, Aug 25, 2013 at 04:45:50AM -0400, Jeff King wrote:\n> \n> > This is the minimal fix. Another option would be to switch\n> > read_mailmap_buf to read_mailmap_string, and I think we could even get\n> > away with avoiding the extra allocation/copy in the loop (because\n> > read_mailmap_line seems to cope with newline-or-EOS just fine). But it\n> > may be better to save that for 'master'.\n> \n> Hmm, actually, this isn't quite true. read_mailmap_line does handle the\n> optional trailing newline properly, but the underlying parsing routines\n> really do want to see a NUL at the end of each line (because they came\n> from code that just calls fgets). So we really do want to tie off each\n> line. But given that our only caller is handing us blob contents which\n> get immediately freed, we could still do that without an extra\n> allocation if:\n> \n> 1. We make it clear that the input must be NUL-terminated (i.e., by\n> renaming the function and dropping the len parameter).\n> \n> 2. We drop the \"const\" from the buf parameter so that we can simply\n> terminate each line as we go.\n> \n> I'll see what the patch looks like.\n\nI think the end result is actually more readable and easier to follow.\nThe only downside is that a true \"I have a const buffer without NUL\ntermination\" caller would have to make a copy of its buffer. But there\nis no such caller currently (and I do not foresee adding one).\n\nHere's the patch directly on top of my other one. I had originally\nthought to put the first to 'maint' and then do the refactoring in\n'master', but this really didn't end up being any more invasive than the\noriginal fix. So maybe it is worth doing both on 'maint' (or squashing\nthem together, in which case the rationale in the commit messages need\ncombined).\n\n-- >8 --\nSubject: mailmap: avoid allocation when reading from blob\n\nThe read_mailmap_blob function reads the whole blob into a\nbuffer, then calls read_mailmap_buf to do the heavy lifting.\nThe latter function ends up making a NUL-terminated copy of\neach line of the blob in order to call read_mailmap_line,\nwhich was originally written to handle the line-at-a-time\ninput from fgets().\n\nWe can avoid the extra copy if we simply NUL-terminate each\nline in place, and assume that the input buffer is itself\nNUL-terminated. Neither of these is a problem, since our\nonly caller is read_mailmap_blob, which has a non-const\nNUL-terminated buffer already.\n\nSigned-off-by: Jeff King \n---\n mailmap.c | 27 +++++++++------------------\n 1 file changed, 9 insertions(+), 18 deletions(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex a635873..caa7c6b 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -187,26 +187,17 @@ static void read_mailmap_buf(struct string_list *map,\n \treturn 0;\n }\n \n-static void read_mailmap_buf(struct string_list *map,\n-\t\t\t const char *buf, unsigned long len,\n-\t\t\t char **repo_abbrev)\n+static void read_mailmap_string(struct string_list *map, char *buf,\n+\t\t\t\tchar **repo_abbrev)\n {\n-\twhile (len) {\n-\t\tconst char *end = memchr(buf, '\\n', len);\n-\t\tunsigned long linelen;\n-\t\tchar *line;\n-\n-\t\tif (end)\n-\t\t\tlinelen = end - buf + 1;\n-\t\telse\n-\t\t\tlinelen = len;\n+\twhile (*buf) {\n+\t\tchar *end = strchrnul(buf, '\\n');\n \n-\t\tline = xmemdupz(buf, linelen);\n-\t\tread_mailmap_line(map, line, repo_abbrev);\n+\t\tif (*end)\n+\t\t\t*end++ = '\\0';\n \n-\t\tfree(line);\n-\t\tbuf += linelen;\n-\t\tlen -= linelen;\n+\t\tread_mailmap_line(map, buf, repo_abbrev);\n+\t\tbuf = end;\n \t}\n }\n \n@@ -230,7 +221,7 @@ static int read_mailmap_blob(struct string_list *map,\n \tif (type != OBJ_BLOB)\n \t\treturn error(\"mailmap is not a blob: %s\", name);\n \n-\tread_mailmap_buf(map, buf, size, repo_abbrev);\n+\tread_mailmap_string(map, buf, repo_abbrev);\n \n \tfree(buf);\n \treturn 0;\n-- \n1.8.4.2.g87d4a77\n"}]}