{"thread":{"id":"22855","subject":"[PATCH] sha1_name: fix segfault caused by invalid index access","startedAt":"2010-02-28T15:49:15Z","lastAt":"2010-02-28T18:13:22Z","messageCount":5,"participants":["Markus Heidelberg","Matthieu Moy","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"135878","messageId":"1267372155-7578-1-git-send-email-markus.heidelberg@web.de","threadId":"22855","inReplyTo":null,"subject":"[PATCH] sha1_name: fix segfault caused by invalid index access","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2010-02-28T15:49:15Z","receivedAt":"2010-02-28T15:49:15Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"It can be reproduced in a bare repository with\n    $ git show :anyfile\n\nI didn't find a recipe for reliably reproducing it in a repository with\nworking tree, it happened depending on the filename and the repository.\n    $ git show :nonexistentfile\n\nSigned-off-by: Markus Heidelberg <markus.heidelberg@web.de>\n---\n\nIt seemed to happen more likely with high letters (x, y, z) as the first\ncharacter of the filename. This always worked for me:\n    $ git show :z\nBut I found this to be too strange to be added to the commit message.\n\nThe affected code path was introduced by commit 009fee477 (Detailed diagnosis\nwhen parsing an object name fails., 2009-12-07).\n\n sha1_name.c |   32 ++++++++++++++++++--------------\n 1 files changed, 18 insertions(+), 14 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 43884c6..bf92417 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -992,13 +992,15 @@ static void diagnose_invalid_index_path(int stage,\n \tpos = cache_name_pos(filename, namelen);\n \tif (pos < 0)\n \t\tpos = -pos - 1;\n-\tce = active_cache[pos];\n-\tif (ce_namelen(ce) == namelen &&\n-\t    !memcmp(ce->name, filename, namelen))\n-\t\tdie(\"Path '%s' is in the index, but not at stage %d.\\n\"\n-\t\t    \"Did you mean ':%d:%s'?\",\n-\t\t    filename, stage,\n-\t\t    ce_stage(ce), filename);\n+\tif (pos < active_nr) {\n+\t\tce = active_cache[pos];\n+\t\tif (ce_namelen(ce) == namelen &&\n+\t\t    !memcmp(ce->name, filename, namelen))\n+\t\t\tdie(\"Path '%s' is in the index, but not at stage %d.\\n\"\n+\t\t\t    \"Did you mean ':%d:%s'?\",\n+\t\t\t    filename, stage,\n+\t\t\t    ce_stage(ce), filename);\n+\t}\n \n \t/* Confusion between relative and absolute filenames? */\n \tfullnamelen = namelen + strlen(prefix);\n@@ -1008,13 +1010,15 @@ static void diagnose_invalid_index_path(int stage,\n \tpos = cache_name_pos(fullname, fullnamelen);\n \tif (pos < 0)\n \t\tpos = -pos - 1;\n-\tce = active_cache[pos];\n-\tif (ce_namelen(ce) == fullnamelen &&\n-\t    !memcmp(ce->name, fullname, fullnamelen))\n-\t\tdie(\"Path '%s' is in the index, but not '%s'.\\n\"\n-\t\t    \"Did you mean ':%d:%s'?\",\n-\t\t    fullname, filename,\n-\t\t    ce_stage(ce), fullname);\n+\tif (pos < active_nr) {\n+\t\tce = active_cache[pos];\n+\t\tif (ce_namelen(ce) == fullnamelen &&\n+\t\t    !memcmp(ce->name, fullname, fullnamelen))\n+\t\t\tdie(\"Path '%s' is in the index, but not '%s'.\\n\"\n+\t\t\t    \"Did you mean ':%d:%s'?\",\n+\t\t\t    fullname, filename,\n+\t\t\t    ce_stage(ce), fullname);\n+\t}\n \n \tif (!lstat(filename, &st))\n \t\tdie(\"Path '%s' exists on disk, but not in the index.\", filename);\n-- \n1.7.0.97.g2d6a2\n"},{"id":"135879","messageId":"vpq7hpxl4cp.fsf@bauges.imag.fr","threadId":"22855","inReplyTo":"1267372155-7578-1-git-send-email-markus.heidelberg@web.de","subject":"Re: [PATCH] sha1_name: fix segfault caused by invalid index access","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-02-28T16:20:06Z","receivedAt":"2010-02-28T16:20:06Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Markus Heidelberg <markus.heidelberg@web.de> writes:\n\n> I didn't find a recipe for reliably reproducing it in a repository with\n> working tree, it happened depending on the filename and the repository.\n>     $ git show :nonexistentfile\n\n> It seemed to happen more likely with high letters (x, y, z) as the first\n> character of the filename. This always worked for me:\n>     $ git show :z\n\nIt happens when you ask for a filename that is after the last index\nentry by alphabetical order, yes. pos will contain an index which is\nafter the previous entry, that is, after the last entry. And then, the\nactive_cache[pos] crashes.\n\n> The affected code path was introduced by commit 009fee477 (Detailed diagnosis\n> when parsing an object name fails., 2009-12-07).\n\nYes, my bad :-(. Thanks for the report and the fix :-).\n\n>  \tpos = cache_name_pos(filename, namelen);\n>  \tif (pos < 0)\n>  \t\tpos = -pos - 1;\n\nActually, if pos < 0, then cache_name_pos didn't find the entry,\nand we shouldn't try any complex thing to find out.\n\nA simpler fix is comming in a separate email. I'm still not familiar\nenough with the index to be 100% confident, but it should do the same\nas yours in a much simpler way. Reviews welcome.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"135880","messageId":"20100228162550.GA7315@coredump.intra.peff.net","threadId":"22855","inReplyTo":"1267372155-7578-1-git-send-email-markus.heidelberg@web.de","subject":"Re: [PATCH] sha1_name: fix segfault caused by invalid index access","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-28T16:25:50Z","receivedAt":"2010-02-28T16:25:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 28, 2010 at 04:49:15PM +0100, Markus Heidelberg wrote:\n\n> It can be reproduced in a bare repository with\n>     $ git show :anyfile\n> \n> I didn't find a recipe for reliably reproducing it in a repository with\n> working tree, it happened depending on the filename and the repository.\n>     $ git show :nonexistentfile\n\nI can confirm the bug here. It is not about bareness, but having no\nindex makes it easy to trigger, since it is easy to walk past the end of\na zero-length index. But it is not restricted to that case:\n\n> It seemed to happen more likely with high letters (x, y, z) as the first\n> character of the filename. This always worked for me:\n>     $ git show :z\n> But I found this to be too strange to be added to the commit message.\n\nThat's because cache_name_pos returns the position where the entry\n_would_ be if it existed (well, the negation minus one, but the intent\nis you can reconstruct that position if you did want to insert it). The\ndiagnose_invalid function then looks at that entry to see if it is a\nmissing filename or a missing stage. But of course, if it would be\ninserted past the end of what exists in the index, there is nothing to\nlook at. So your \":z\" is simply about being at the end of the index,\nwhich is sorted alphabetically.\n\nWhich means your fix (to make sure we are not at the end) is correct.\n\n-Peff\n"},{"id":"135882","messageId":"1267375122-13039-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"22855","inReplyTo":"vpq7hpxl4cp.fsf@bauges.imag.fr","subject":"[PATCH] sha1_name: fix segfault caused by invalid index access","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2010-02-28T16:38:42Z","receivedAt":"2010-02-28T16:38:42Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"009fee477 (Detailed diagnosis when parsing an object name fails,\n2009-12-07) introduced some invalid index access, inspired by the code of\nget_sha1_with_mode_1, which loops over the index entries having the same\nname. In the diagnosis, we just want to find whether one entry with the\nname is in the index, which is the case iff cache_name_pos's return value\nis positive.\n\nTrying anything complex on negative value is not only useless, but also\nbuggy here, since pos could end up being greater than active_nr, causing\na segfault in active_cache[pos]. This is always the case in bare\nrepositories, and happens when calling \"git show :inexistant\" if\n\"inexistant\" is greater than the last index entry in alphabetical order.\n\nBug report and initial fix by Markus Heidelberg\n<markus.heidelberg@web.de>.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n sha1_name.c |   16 ++++++----------\n 1 files changed, 6 insertions(+), 10 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 43884c6..fbbe3b4 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -990,15 +990,13 @@ static void diagnose_invalid_index_path(int stage,\n \n \t/* Wrong stage number? */\n \tpos = cache_name_pos(filename, namelen);\n-\tif (pos < 0)\n-\t\tpos = -pos - 1;\n-\tce = active_cache[pos];\n-\tif (ce_namelen(ce) == namelen &&\n-\t    !memcmp(ce->name, filename, namelen))\n+\tif (pos >= 0) {\n+\t\tce = active_cache[pos];\n \t\tdie(\"Path '%s' is in the index, but not at stage %d.\\n\"\n \t\t    \"Did you mean ':%d:%s'?\",\n \t\t    filename, stage,\n \t\t    ce_stage(ce), filename);\n+\t}\n \n \t/* Confusion between relative and absolute filenames? */\n \tfullnamelen = namelen + strlen(prefix);\n@@ -1006,15 +1004,13 @@ static void diagnose_invalid_index_path(int stage,\n \tstrcpy(fullname, prefix);\n \tstrcat(fullname, filename);\n \tpos = cache_name_pos(fullname, fullnamelen);\n-\tif (pos < 0)\n-\t\tpos = -pos - 1;\n-\tce = active_cache[pos];\n-\tif (ce_namelen(ce) == fullnamelen &&\n-\t    !memcmp(ce->name, fullname, fullnamelen))\n+\tif (pos >= 0) {\n+\t\tce = active_cache[pos];\n \t\tdie(\"Path '%s' is in the index, but not '%s'.\\n\"\n \t\t    \"Did you mean ':%d:%s'?\",\n \t\t    fullname, filename,\n \t\t    ce_stage(ce), fullname);\n+\t}\n \n \tif (!lstat(filename, &st))\n \t\tdie(\"Path '%s' exists on disk, but not in the index.\", filename);\n-- \n1.7.0.231.g97960.dirty\n"},{"id":"135883","messageId":"7vocj9cjp9.fsf@alter.siamese.dyndns.org","threadId":"22855","inReplyTo":"1267375122-13039-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH] sha1_name: fix segfault caused by invalid index access","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-28T18:13:22Z","receivedAt":"2010-02-28T18:13:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n\n> -\tif (pos < 0)\n> -\t\tpos = -pos - 1;\n> -\tce = active_cache[pos];\n> -\tif (ce_namelen(ce) == namelen &&\n> -\t    !memcmp(ce->name, filename, namelen))\n> +\tif (pos >= 0) {\n> +\t\tce = active_cache[pos];\n\nA positive return value of cache_name_pos() is \"I found a merged entry\nwith that name at this index\" while a negative is \"I would insert at this\nindex if you give me a merged entry with that name\".\n\nThe latter is what the name comparison logic is about.  The caller asked\nfor filename, and the return value may point at an existing entry (in\nwhich case you do not even need to memcmp(), but it doesn't hurt and\nsimplifies the code).  The nagative one with compensation could be\npointing at an unrelated entry, or an unmerged entry with filename you\nasked, which sorts higher than a merged entry with the same name.\n"}]}