{"thread":{"id":"20140","subject":"infinite loop in git-send-email with alias files","startedAt":"2009-07-17T01:10:00Z","lastAt":"2009-07-23T13:54:09Z","messageCount":4,"participants":["Mike Frysinger","Jeff King","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"118148","messageId":"8bd0f97a0907161810w22726ffdye5c8d64719b77b53@mail.gmail.com","threadId":"20140","inReplyTo":null,"subject":"infinite loop in git-send-email with alias files","fromName":"Mike Frysinger","fromEmail":"vapier.adi@gmail.com","sentAt":"2009-07-17T01:10:00Z","receivedAt":"2009-07-17T01:10:00Z","isPatch":false,"sender":{"key":"vapier.adi@gmail.com","avatar":null},"body":"i was setting up an aliasesfile for git-send-email, but in doing so, i\ninadvertently made a typo creating an infinite loop.  i didnt notice\nright away, but i did notice when `git-send-email` hung using 100% of\na cpu.\n\nsimple way to reproduce:\n$ $ git config sendemail.aliasesfile\n.git/mail\n$ git config sendemail.aliasfiletype\nmutt\n$ cat .git/mail\nalias a b\nalias b a\n$ git send-email HEAD^ --to a\n<hit enter a few times and watch it hang>\n\nnoticed with 1.6.3.3, but seems to be in 1.6.4-rc1 too\n-mike\n"},{"id":"118575","messageId":"20090723110928.GC4247@coredump.intra.peff.net","threadId":"20140","inReplyTo":"8bd0f97a0907161810w22726ffdye5c8d64719b77b53@mail.gmail.com","subject":"Re: infinite loop in git-send-email with alias files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-07-23T11:09:29Z","receivedAt":"2009-07-23T11:09:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 16, 2009 at 09:10:00PM -0400, Mike Frysinger wrote:\n\n> i was setting up an aliasesfile for git-send-email, but in doing so, i\n> inadvertently made a typo creating an infinite loop.  i didnt notice\n> right away, but i did notice when `git-send-email` hung using 100% of\n> a cpu.\n\nYep, we don't do cycle detection on alias expansion. It is easy enough\nto do, though:\n\n-- >8 --\nSubject: [PATCH] send-email: detect cycles in alias expansion\n\nWith the previous code, an alias cycle like:\n\n  $ echo 'alias a b' >aliases\n  $ echo 'alias b a' >aliases\n  $ git config sendemail.aliasesfile aliases\n  $ git config sendemail.aliasfiletype mutt\n\nwould put send-email into an infinite loop. This patch\ndetects the situation and complains to the user.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nTwo notes on this patch:\n\n  1. I ended up rewriting the iterative expansion as a recursive\n     function, because it makes the code simpler (doing proper detection\n     in the iterative loop means you have to check where each expansion\n     came from. That is, expanding \"a\" to \"b\" and \"c\" to \"b\" is OK, but\n     expanding \"a\" to \"b\" to \"c\" is not. So you end up having to\n     implement a stack, anyway. Much easier to let perl do it for us).\n\n     But this also raises the likelihood that I screwed something up, so\n     I would appreciate an extra set of eyes.\n\n  2. It just barfs. I figure such a situation is a sign of a problem\n     that the user should address. But we could also just stop expansion\n     (so \"a\" => \"b\" => \"a\" simply expands to \"a\"); this is more like how\n     bash aliases work, but I don't know if there is any benefit to that\n     here.\n\n     We could also print out the cycle, but I doubt it is worth the\n     trouble.\n\n git-send-email.perl |   18 +++++++++++-------\n 1 files changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 8ce6f1f..d508f83 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -654,13 +654,17 @@ if (!@to) {\n }\n \n sub expand_aliases {\n-\tmy @cur = @_;\n-\tmy @last;\n-\tdo {\n-\t\t@last = @cur;\n-\t\t@cur = map { $aliases{$_} ? @{$aliases{$_}} : $_ } @last;\n-\t} while (join(',',@cur) ne join(',',@last));\n-\treturn @cur;\n+\treturn map { expand_one_alias($_) } @_;\n+}\n+\n+my %EXPANDED_ALIASES;\n+sub expand_one_alias {\n+\tmy $alias = shift;\n+\tif ($EXPANDED_ALIASES{$alias}) {\n+\t\tdie \"fatal: alias '$alias' expands to itself\\n\";\n+\t}\n+\tlocal $EXPANDED_ALIASES{$alias} = 1;\n+\treturn $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;\n }\n \n @to = expand_aliases(@to);\n-- \n1.6.4.rc1.190.g0a8d.dirty\n"},{"id":"118578","messageId":"4A6857FF.5070401@viscovery.net","threadId":"20140","inReplyTo":"20090723110928.GC4247@coredump.intra.peff.net","subject":"Re: infinite loop in git-send-email with alias files","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-07-23T12:30:55Z","receivedAt":"2009-07-23T12:30:55Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> +my %EXPANDED_ALIASES;\n> +sub expand_one_alias {\n> +\tmy $alias = shift;\n> +\tif ($EXPANDED_ALIASES{$alias}) {\n> +\t\tdie \"fatal: alias '$alias' expands to itself\\n\";\n> +\t}\n> +\tlocal $EXPANDED_ALIASES{$alias} = 1;\n> +\treturn $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;\n\nWhat does 'local' make local here? Only the assignment of the slot\n$EXPANDED_ALIASES{$alias}? Or the whole %EXPANDED_ALIASES? If the latter,\ndoes this copy the existing %EXPANDED_ALIASES before the assignment is\nmade; otherwise, how can this work if only ever a single slot of\n%EXPANDED_ALIASES is filled in?\n\n(Disclaimer: I'm not a perl expert, obviously, and I didn't test your patch.)\n\n-- Hannes\n"},{"id":"118580","messageId":"20090723135408.GA22317@coredump.intra.peff.net","threadId":"20140","inReplyTo":"4A6857FF.5070401@viscovery.net","subject":"Re: infinite loop in git-send-email with alias files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-07-23T13:54:09Z","receivedAt":"2009-07-23T13:54:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 23, 2009 at 02:30:55PM +0200, Johannes Sixt wrote:\n\n> Jeff King schrieb:\n> > +my %EXPANDED_ALIASES;\n> > +sub expand_one_alias {\n> > +\tmy $alias = shift;\n> > +\tif ($EXPANDED_ALIASES{$alias}) {\n> > +\t\tdie \"fatal: alias '$alias' expands to itself\\n\";\n> > +\t}\n> > +\tlocal $EXPANDED_ALIASES{$alias} = 1;\n> > +\treturn $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;\n> \n> What does 'local' make local here? Only the assignment of the slot\n> $EXPANDED_ALIASES{$alias}? Or the whole %EXPANDED_ALIASES? If the latter,\n> does this copy the existing %EXPANDED_ALIASES before the assignment is\n> made; otherwise, how can this work if only ever a single slot of\n> %EXPANDED_ALIASES is filled in?\n\nIt localizes just that slot. But remember that 'local' is about\n_dynamic_ scoping, not _lexical_ scoping. So that slot is now set for\nthe duration of the expand_one_alias call, and is visible to its\nsubroutines (i.e., the recursive calls). So each level of recursion sets\none more field in $EXPANDED_ALIASES, and when we leave the function,\nperl automatically restores it to its previous value.\n\n-Peff\n"}]}