{"thread":{"id":"45534","subject":"[PATCH] read-cache: avoid git_path() race in freshen_shared_index()","startedAt":"2017-03-29T08:12:11Z","lastAt":"2017-04-01T08:21:12Z","messageCount":7,"participants":["Christian Couder","Junio C Hamano","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"315655","messageId":"20170329080820.8084-1-chriscool@tuxfamily.org","threadId":"45534","inReplyTo":null,"subject":"[PATCH] read-cache: avoid git_path() race in freshen_shared_index()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-03-29T08:08:20Z","receivedAt":"2017-03-29T08:12:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When performing an interactive rebase in split-index mode,\nthe commit message that one should rework when squashing commits\ncan contain some garbage instead of the usual concatenation of\nboth of the commit messages.\n\nWhen bisecting it appears that 94c9b5af70 (Merge branch\n'cc/split-index-config', 2017-03-17) is the first bad commit.\n\nBut when rebasing cc/split-index-config on top of the commit it\nwas merged with, the first bad commit is then c3a0082502\n(read-cache: use freshen_shared_index() in read_index_from(),\n2017-03-06).\n\nThis shows that we should be careful not to use git_path() in\nfreshen_shared_index(). It is using a shared buffer that can\ntoo easily lead to races.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n read-cache.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex e447751823..2f10242c24 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1682,9 +1682,10 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n  */\n static void freshen_shared_index(char *base_sha1_hex, int warn)\n {\n-\tconst char *shared_index = git_path(\"sharedindex.%s\", base_sha1_hex);\n+\tchar *shared_index = git_pathdup(\"sharedindex.%s\", base_sha1_hex);\n \tif (!check_and_freshen_file(shared_index, 1) && warn)\n \t\twarning(\"could not freshen shared index '%s'\", shared_index);\n+\tfree(shared_index);\n }\n \n int read_index_from(struct index_state *istate, const char *path)\n-- \n2.12.0.339.g3df8399f7e.dirty\n\n"},{"id":"315687","messageId":"xmqqfuhwau6r.fsf@gitster.mtv.corp.google.com","threadId":"45534","inReplyTo":"20170329080820.8084-1-chriscool@tuxfamily.org","subject":"Re: [PATCH] read-cache: avoid git_path() race in freshen_shared_index()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-29T17:06:52Z","receivedAt":"2017-03-29T17:07:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> When performing an interactive rebase in split-index mode,\n> the commit message that one should rework when squashing commits\n> can contain some garbage instead of the usual concatenation of\n> both of the commit messages.\n\nOK, that is an understandable explanation of what problem you are\ntrying to fix.\n\n>\n> When bisecting it appears that 94c9b5af70 (Merge branch\n> 'cc/split-index-config', 2017-03-17) is the first bad commit.\n>\n> But when rebasing cc/split-index-config on top of the commit it\n> was merged with, the first bad commit is then c3a0082502\n> (read-cache: use freshen_shared_index() in read_index_from(),\n> 2017-03-06).\n\nThis part however doesn't help understanding the issue.  \"When X but\nwhen Y\" sounds as if you found a botched merge, but that does not\nseem to be the case.  The resulting tree after rebasing (with\nconflict resolution) is the same as the recorded merge result.  It\ncould be saying that \"git bisect\" is buggy and does not pinpoint the\nbroken commit, but this is not a commit to fix \"bisect\".\n\nThat leaves the reader confused.\n\n> This shows that we should be careful not to use git_path() in\n> freshen_shared_index(). It is using a shared buffer that can\n> too easily lead to races.\n\nThe impression I get from the symptom is that after git_path() is\ncalled here, before check_and_freshen_file() uses that result, it\n(or functions it calls) uses git_path(), and the number of times it\ndoes so has changed since cc/split-index-config was written on the\nmainline, and the rotating 4-element buffer get_pathname() gives is\nnow exhausted, leading to the failure you observed.  By the way,\nthat does not sound a race to me.\n\nIn any case, that explains why bisect says the merge is the first\nbad one, and cures the confused reader ;-) The use of git_path() on\nthe topic was still safe; it was a timebomb waiting to go off.  The\nmainline started using more calls and the merge result was unsafe.\n\nIf you meant to summarise the whole two paragraphs above that I\nneeded to think it through with \"This shows that\", I'd have to say\nthat you are expecting too much from your readers.  Please be a bit\nmore gentle to them.\n\nThanks.\n\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  read-cache.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index e447751823..2f10242c24 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1682,9 +1682,10 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n>   */\n>  static void freshen_shared_index(char *base_sha1_hex, int warn)\n>  {\n> -\tconst char *shared_index = git_path(\"sharedindex.%s\", base_sha1_hex);\n> +\tchar *shared_index = git_pathdup(\"sharedindex.%s\", base_sha1_hex);\n>  \tif (!check_and_freshen_file(shared_index, 1) && warn)\n>  \t\twarning(\"could not freshen shared index '%s'\", shared_index);\n> +\tfree(shared_index);\n>  }\n>  \n>  int read_index_from(struct index_state *istate, const char *path)\n"},{"id":"315692","messageId":"20170329175614.uecvtvmzoj2wfle3@sigill.intra.peff.net","threadId":"45534","inReplyTo":"xmqqfuhwau6r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] read-cache: avoid git_path() race in freshen_shared_index()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-29T17:56:14Z","receivedAt":"2017-03-29T17:56:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 29, 2017 at 10:06:52AM -0700, Junio C Hamano wrote:\n\n> > This shows that we should be careful not to use git_path() in\n> > freshen_shared_index(). It is using a shared buffer that can\n> > too easily lead to races.\n> \n> The impression I get from the symptom is that after git_path() is\n> called here, before check_and_freshen_file() uses that result, it\n> (or functions it calls) uses git_path(), and the number of times it\n> does so has changed since cc/split-index-config was written on the\n> mainline, and the rotating 4-element buffer get_pathname() gives is\n> now exhausted, leading to the failure you observed.  By the way,\n> that does not sound a race to me.\n> \n> In any case, that explains why bisect says the merge is the first\n> bad one, and cures the confused reader ;-) The use of git_path() on\n> the topic was still safe; it was a timebomb waiting to go off.  The\n> mainline started using more calls and the merge result was unsafe.\n\nYeah, it looks like that is what happened. I see that Christian bisected\nthe rebase to find the commit in the series that introduces the problem.\nI'm mildly curious which commit upstream created the problem[1].\nThere's a reasonable chance it's some innocent-looking cleanup (possibly\none of my recent \"stop using a fixed buffer\" ones).\n\nBut in the end it doesn't really matter. I think code like:\n\n  const char *filename = git_path(...);\n\nor\n\n  nontrivial_function(git_path(...));\n\nis an anti-pattern. It _might_ be safe, but it's really hard to tell\nwithout following the complete lifetime of the return value. I've been\ntempted to suggest we should abolish git_path() entirely. But it's so\ndarn useful for things like unlink(git_path(...)), or other direct\nsystem calls.\n\nAs an aside, this kind of static-buffer reuse _used_ to mean you might\nsee somebody else's buffer. Which is bad enough. But since the move to\nuse strbufs underneath the hood of git_path(), it may produce that\neffect or it may be a use-after-free (if the strbuf had to reallocate to\ngrow in the meantime).\n\nAnyway. The fix in the patch is obviously the right thing.\n\n-Peff\n\n[1] I think we could pinpoint the upstream change that caused the bad\n    interaction by bisecting between the merge-base and the first-parent\n    of the broken merge. For each commit, cherry-pick the complete\n    series on top of it, and test the result.\n"},{"id":"315759","messageId":"CAP8UFD29xWU5bHwewUSq26pVTwK-gy3uSVG2aGTCrDak3620=A@mail.gmail.com","threadId":"45534","inReplyTo":"20170329175614.uecvtvmzoj2wfle3@sigill.intra.peff.net","subject":"Re: [PATCH] read-cache: avoid git_path() race in freshen_shared_index()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-03-30T08:40:38Z","receivedAt":"2017-03-30T08:41:12Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Mar 29, 2017 at 7:56 PM, Jeff King <peff@peff.net> wrote:\n> On Wed, Mar 29, 2017 at 10:06:52AM -0700, Junio C Hamano wrote:\n>\n>> > This shows that we should be careful not to use git_path() in\n>> > freshen_shared_index(). It is using a shared buffer that can\n>> > too easily lead to races.\n>>\n>> The impression I get from the symptom is that after git_path() is\n>> called here, before check_and_freshen_file() uses that result, it\n>> (or functions it calls) uses git_path(), and the number of times it\n>> does so has changed since cc/split-index-config was written on the\n>> mainline, and the rotating 4-element buffer get_pathname() gives is\n>> now exhausted, leading to the failure you observed.  By the way,\n>> that does not sound a race to me.\n>>\n>> In any case, that explains why bisect says the merge is the first\n>> bad one, and cures the confused reader ;-) The use of git_path() on\n>> the topic was still safe; it was a timebomb waiting to go off.  The\n>> mainline started using more calls and the merge result was unsafe.\n>\n> Yeah, it looks like that is what happened. I see that Christian bisected\n> the rebase to find the commit in the series that introduces the problem.\n> I'm mildly curious which commit upstream created the problem[1].\n\nI bisected it to 18633e1a22 (rebase -i: use the rebase--helper\nbuiltin, 2017-02-09).\nThis commit is indeed changing how the interactive rebase works, but\nit is not easy to see how it impact git_path() usage.\n\n> There's a reasonable chance it's some innocent-looking cleanup (possibly\n> one of my recent \"stop using a fixed buffer\" ones).\n>\n> But in the end it doesn't really matter. I think code like:\n>\n>   const char *filename = git_path(...);\n>\n> or\n>\n>   nontrivial_function(git_path(...));\n>\n> is an anti-pattern. It _might_ be safe, but it's really hard to tell\n> without following the complete lifetime of the return value. I've been\n> tempted to suggest we should abolish git_path() entirely. But it's so\n> darn useful for things like unlink(git_path(...)), or other direct\n> system calls.\n\nYeah, I am very tempted to just rewrite the commit message like this:\n\n------------\n\nWhen performing an interactive rebase in split-index mode,\nthe commit message that one should rework when squashing commits\ncan contain some garbage instead of the usual concatenation of\nboth of the commit messages.\n\nBisecting shows that c3a0082502 (read-cache: use\nfreshen_shared_index() in read_index_from(), 2017-03-06) is involved,\nwhich points to the unsafe use of git_path() in\nfreshen_shared_index().\n\n------------\n\nand change the subject to \"read-cache: avoid using git_path() in\nfreshen_shared_index()\".\n"},{"id":"315761","messageId":"CACsJy8BR9=Ju_7tSmZEH+UDqWG3Ud1KXY-m6r_jHb-aQUoUMAg@mail.gmail.com","threadId":"45534","inReplyTo":"20170329175614.uecvtvmzoj2wfle3@sigill.intra.peff.net","subject":"Re: [PATCH] read-cache: avoid git_path() race in freshen_shared_index()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-30T09:24:40Z","receivedAt":"2017-03-30T09:25:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 30, 2017 at 12:56 AM, Jeff King <peff@peff.net> wrote:\n> But in the end it doesn't really matter. I think code like:\n>\n>   const char *filename = git_path(...);\n>\n> or\n>\n>   nontrivial_function(git_path(...));\n>\n> is an anti-pattern. It _might_ be safe, but it's really hard to tell\n> without following the complete lifetime of the return value. I've been\n> tempted to suggest we should abolish git_path() entirely. But it's so\n> darn useful for things like unlink(git_path(...)), or other direct\n> system calls.\n\nYeah. I thought we killed most of those (was it your patches?).\n\nI had a quick look at \"git grep -w git_path\" again. The ones in\nbuiltin/am.c, builtin/grep.c and submodule.c look very much like that\nanti-pattern. The one in read_index_from() probably should be replaced\nwith git_pathdup() as well. Sorry no patches (I'm very slow these\ndays).\n-- \nDuy\n"},{"id":"315788","messageId":"xmqq7f367o0s.fsf@gitster.mtv.corp.google.com","threadId":"45534","inReplyTo":"CAP8UFD29xWU5bHwewUSq26pVTwK-gy3uSVG2aGTCrDak3620=A@mail.gmail.com","subject":"Re: [PATCH] read-cache: avoid git_path() race in freshen_shared_index()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-30T16:00:35Z","receivedAt":"2017-03-30T16:00:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> On Wed, Mar 29, 2017 at 7:56 PM, Jeff King <peff@peff.net> wrote:\n> ...\n>> Yeah, it looks like that is what happened. I see that Christian bisected\n>> the rebase to find the commit in the series that introduces the problem.\n>> I'm mildly curious which commit upstream created the problem[1].\n>\n> I bisected it to 18633e1a22 (rebase -i: use the rebase--helper\n> builtin, 2017-02-09).\n> This commit is indeed changing how the interactive rebase works, but\n> it is not easy to see how it impact git_path() usage.\n\nThat small series activates a large body of code that hasn't been exercised\nso it is not surprising.  \n\n> Yeah, I am very tempted to just rewrite the commit message like this:\n>\n> ------------\n>\n> When performing an interactive rebase in split-index mode,\n> the commit message that one should rework when squashing commits\n> can contain some garbage instead of the usual concatenation of\n> both of the commit messages.\n>\n> Bisecting shows that c3a0082502 (read-cache: use\n> freshen_shared_index() in read_index_from(), 2017-03-06) is involved,\n> which points to the unsafe use of git_path() in\n> freshen_shared_index().\n>\n> ------------\n>\n> and change the subject to \"read-cache: avoid using git_path() in\n> freshen_shared_index()\".\n\nSure.  It may not be even worth mentioning a not-so-useful bisect\nresult, and the potential riskiness of the original code (iow why\nthis fix is the right thing) can be seen from the patch alone.\n\nThanks for following it through.\n"},{"id":"316000","messageId":"20170401082059.wdgvcv2qv56b75b4@sigill.intra.peff.net","threadId":"45534","inReplyTo":"CACsJy8BR9=Ju_7tSmZEH+UDqWG3Ud1KXY-m6r_jHb-aQUoUMAg@mail.gmail.com","subject":"Re: [PATCH] read-cache: avoid git_path() race in freshen_shared_index()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-01T08:20:59Z","receivedAt":"2017-04-01T08:21:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 30, 2017 at 04:24:40PM +0700, Duy Nguyen wrote:\n\n> On Thu, Mar 30, 2017 at 12:56 AM, Jeff King <peff@peff.net> wrote:\n> > But in the end it doesn't really matter. I think code like:\n> >\n> >   const char *filename = git_path(...);\n> >\n> > or\n> >\n> >   nontrivial_function(git_path(...));\n> >\n> > is an anti-pattern. It _might_ be safe, but it's really hard to tell\n> > without following the complete lifetime of the return value. I've been\n> > tempted to suggest we should abolish git_path() entirely. But it's so\n> > darn useful for things like unlink(git_path(...)), or other direct\n> > system calls.\n> \n> Yeah. I thought we killed most of those (was it your patches?).\n\nYes, after fixing a bug where static buffer reuse caused git to randomly\ndelete a ref, I rage-converted most of the dangerous looking cases.\n\n> I had a quick look at \"git grep -w git_path\" again. The ones in\n> builtin/am.c, builtin/grep.c and submodule.c look very much like that\n> anti-pattern. The one in read_index_from() probably should be replaced\n> with git_pathdup() as well. Sorry no patches (I'm very slow these\n> days).\n\nYeah, I think a number of them are actually OK if you dig (e.g., passing\nit to am_state_init() immediately duplicates the result), but it's a bad\npattern if you have to dig to see if it's right. It's hard to tell when\na sub-function may reuse the buffer. For instance, git-init passes the\nresult to adjust_shared_perm(), which might lazily load the config from\ndisk. I don't know if that calls git_path() or not, but it's an awful\nlot of code to run.\n\nA lot of the cases look like they could be fixed by using git_path_foo()\ninstead of git_path(\"FOO\"). (And in many cases we even have\ngit_path_foo() defined already!).\n\nMy favorite is the one in add_worktree(), which calls strbuf_addstr() on\nthe result of git_path(0. That one's _not_ dangerous, but surely it\nwould be simpler to just write directly into the strbuf. :)\n\n-Peff\n"}]}