{"thread":{"id":"25772","subject":"Runaway \"git remote\" if group definition contains a remote by the same name","startedAt":"2010-11-17T17:10:00Z","lastAt":"2013-12-31T08:06:47Z","messageCount":5,"participants":["Alex Riesen","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"156057","messageId":"AANLkTinni=VJLoZp1Hjm4dfW8faChytDObJbXsFF5iXv@mail.gmail.com","threadId":"25772","inReplyTo":null,"subject":"Runaway \"git remote\" if group definition contains a remote by the same name","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-11-17T17:10:00Z","receivedAt":"2010-11-17T17:10:00Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Hi,\n\nit is also a way to create a fork bomb out of the innocent tool on platforms\nwhere pressing Ctrl-C does not terminate subprocesses of the foreground\nprocess (like, of course, Windows).\n\nTo reproduce, run\n\n   git -c remotes.origin='origin other' remote update origin\n\nI just cannot look at it right now, and have to resolve to only reporting\nthe problem to warn people. Something seems to resolve the remotes group\ndefinition over and over again.\n"},{"id":"232489","messageId":"CALxABCbRZ4MmiYS4JF20qf1-iubeTfa+3OLibqdb5+raekuKQg@mail.gmail.com","threadId":"25772","inReplyTo":"AANLkTinni=VJLoZp1Hjm4dfW8faChytDObJbXsFF5iXv@mail.gmail.com","subject":"Fwd: Runaway \"git remote\" if group definition contains a remote by the same name","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2013-12-28T14:56:55Z","receivedAt":"2013-12-28T14:56:55Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"FWIW, the issue is still present.\n\n---------- Forwarded message ----------\nFrom: Alex Riesen <raa.lkml@gmail.com>\nDate: Wed, Nov 17, 2010 at 6:10 PM\nSubject: Runaway \"git remote\" if group definition contains a remote by\nthe same name\nTo: Git Mailing List <git@vger.kernel.org>\n\n\nHi,\n\nit is also a way to create a fork bomb out of the innocent tool on platforms\nwhere pressing Ctrl-C does not terminate subprocesses of the foreground\nprocess (like, of course, Windows).\n\nTo reproduce, run\n\n   git -c remotes.origin='origin other' remote update origin\n\nI just cannot look at it right now, and have to resolve to only reporting\nthe problem to warn people. Something seems to resolve the remotes group\ndefinition over and over again.\n"},{"id":"232500","messageId":"20131229075838.GC31788@sigill.intra.peff.net","threadId":"25772","inReplyTo":"CALxABCbRZ4MmiYS4JF20qf1-iubeTfa+3OLibqdb5+raekuKQg@mail.gmail.com","subject":"Re: Fwd: Runaway \"git remote\" if group definition contains a remote by the same name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-29T07:58:38Z","receivedAt":"2013-12-29T07:58:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 28, 2013 at 03:56:55PM +0100, Alex Riesen wrote:\n\n> it is also a way to create a fork bomb out of the innocent tool on platforms\n> where pressing Ctrl-C does not terminate subprocesses of the foreground\n> process (like, of course, Windows).\n> \n> To reproduce, run\n> \n>    git -c remotes.origin='origin other' remote update origin\n\nHmm. This is a pretty straightforward reference cycle. We expand origin\nto contain itself, so it recurses forever on expansion. As with most\nsuch problems, the cycle path may be longer than one:\n\n  git -c remotes.foo='bar baz' -c remotes.bar='foo baz' fetch foo\n\nDetecting the cycle can be done by keeping track of which names we've\nseen, or just by putting in a depth limit that no sane setup would hit.\nIn either case, it's complicated slightly by the fact that we pass the\nexpanded list to a sub-process (which then recurses on the expansion).\nSo we'd have to communicate the depth (or the list of seen remotes) via\nthe command line or the environment.\n\nOne alternative would be to have the parent \"git fetch\" recursively\nexpand the list itself down to scalar entries, and then invoke\nsub-processes on the result (and tell them not to expand at all). That\nwould also let us cull duplicates if a remote is found via multiple\ngroups.\n\nInterestingly, the problem does not happen with this:\n\n  git -c remotes.foo=foo fetch foo\n\nFetch sees that foo expands only to a single item and says \"oh, that\nmust not be a group\". And then treats it like a regular remote, rather\nthan recursing. So it's not clear to me whether groups are meant to be\nrecursive or not. They are in some cases:\n\n  # fetch remotes 1-4\n  git -c remotes.parent='child1 child2' \\\n      -c remotes.child1='remote1 remote2' \\\n      -c remotes.child2='remote3 remote4' \\\n      fetch parent\n\nbut not in others:\n\n  # \"foo\" should be an alias for \"bar\", but it's not\n  git -c remotes.foo=bar \\\n      -c remotes.bar='remote1 remote2' \\\n      fetch foo\n\nIf they are not allowed to recurse, the problem is much easier; the\nparent fetch simply tells all of the sub-invocations not to expand the\narguments further. However, whether it was planned or not, it has been\nthis way for a long time. I would not be surprised if somebody is\nrelying on the recursion to help organize their groups.\n\nSo I think the sanest thing is probably:\n\n  1. Teach \"fetch\" to expand recursively in a single process, and then\n     tell sub-processes (via a new command-line option) not to expand\n     any further.\n\n  2. Teach \"fetch\" to detect cycles (probably just by a simple depth\n     counter).\n\n  3. Teach the group-reading code to detect groups more robustly, so\n     that a single-item group like \"remotes.foo=bar\" correctly recurses\n     to \"bar\".\n\n  4. (Optional) Teach the expansion code from step 1 to cull duplicates,\n     so that we do not try to fetch from the same remote twice (e.g., if\n     it is mentioned as part of two groups, and both are specified on\n     the command line).\n\nI do not plan to work on this myself in the immediate future, but\nperhaps it is an interesting low-hanging fruit for somebody else.\n\n-Peff\n"},{"id":"232518","messageId":"xmqqlhz2rw7s.fsf@gitster.dls.corp.google.com","threadId":"25772","inReplyTo":"20131229075838.GC31788@sigill.intra.peff.net","subject":"Re: Fwd: Runaway \"git remote\" if group definition contains a remote by the same name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-30T19:10:31Z","receivedAt":"2013-12-30T19:10:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> If they are not allowed to recurse, the problem is much easier; the\n> parent fetch simply tells all of the sub-invocations not to expand the\n> arguments further. However, whether it was planned or not, it has been\n> this way for a long time. I would not be surprised if somebody is\n> relying on the recursion to help organize their groups.\n>\n> So I think the sanest thing is probably:\n>\n>   1. Teach \"fetch\" to expand recursively in a single process, and then\n>      tell sub-processes (via a new command-line option) not to expand\n>      any further.\n>\n>   2. Teach \"fetch\" to detect cycles (probably just by a simple depth\n>      counter).\n\nI suspect that the expansion code will just accumulate remotes found\ninto a string-list (as part of 4. below), so deduping would be\nfairly easily done without a depth counter.\n\n>   3. Teach the group-reading code to detect groups more robustly, so\n>      that a single-item group like \"remotes.foo=bar\" correctly recurses\n>      to \"bar\".\n\nA single-item remote group is somewhat annoying, but expanding it\nonly at some places while ignoring it at other places is even more\nannoying, so this sounds like a right thing to do.\n\n>   4. (Optional) Teach the expansion code from step 1 to cull duplicates,\n>      so that we do not try to fetch from the same remote twice (e.g., if\n>      it is mentioned as part of two groups, and both are specified on\n>      the command line).\n>\n> I do not plan to work on this myself in the immediate future, but\n> perhaps it is an interesting low-hanging fruit for somebody else.\n>\n> -Peff\n"},{"id":"232538","messageId":"20131231080647.GA25838@sigill.intra.peff.net","threadId":"25772","inReplyTo":"xmqqlhz2rw7s.fsf@gitster.dls.corp.google.com","subject":"Re: Fwd: Runaway \"git remote\" if group definition contains a remote by the same name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-31T08:06:47Z","receivedAt":"2013-12-31T08:06:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 30, 2013 at 11:10:31AM -0800, Junio C Hamano wrote:\n\n> > So I think the sanest thing is probably:\n> >\n> >   1. Teach \"fetch\" to expand recursively in a single process, and then\n> >      tell sub-processes (via a new command-line option) not to expand\n> >      any further.\n> >\n> >   2. Teach \"fetch\" to detect cycles (probably just by a simple depth\n> >      counter).\n> \n> I suspect that the expansion code will just accumulate remotes found\n> into a string-list (as part of 4. below), so deduping would be\n> fairly easily done without a depth counter.\n\nI don't think that will work (at least not naively). The end-product of\nstep 1, and the string_list that is de-duped in step 4, is a list of the\nconcrete remotes. The cycles occur between groups, which are not\nmentioned in the final list.\n\nYou can keep a separate list of the groups we visit, of course, but we\ndo not otherwise need it.\n\nOne thing that does make such a list easier is that we do not need to\ncare about order. E.g., in config like this:\n\n  [remotes]\n  a = c\n  b = c\n  c = d e\n\nyou can mark \"c\" as seen after visiting it via \"a\". It is not\ntechnically a cycle, but since we would want to suppress duplicates\nanyway, we can be overly broad.\n\n> >   3. Teach the group-reading code to detect groups more robustly, so\n> >      that a single-item group like \"remotes.foo=bar\" correctly recurses\n> >      to \"bar\".\n> \n> A single-item remote group is somewhat annoying, but expanding it\n> only at some places while ignoring it at other places is even more\n> annoying, so this sounds like a right thing to do.\n\nThe only configuration that I think would be negatively affected is\nsomething like:\n\n  [remote]\n  foo = foo\n  [remote \"foo\"]\n  url = ...\n\nthat silently works now, but would become broken (because we would\ncomplain about the cycle). I think that's OK; that config is clearly\nstupid and broken. If it were \"remote.foo = foo bar\", trying to expand\nthe concrete \"foo\" and \"bar\", that might make some sense, but then it is\nalready broken in the current code (that is the example that started the\ndiscussion).\n\n-Peff\n"}]}