{"action":"created","issue":{"url":"https://api.github.com/repos/Homebrew/brew/issues/46","repository_url":"https://api.github.com/repos/Homebrew/brew","labels_url":"https://api.github.com/repos/Homebrew/brew/issues/46/labels{/name}","comments_url":"https://api.github.com/repos/Homebrew/brew/issues/46/comments","events_url":"https://api.github.com/repos/Homebrew/brew/issues/46/events","html_url":"https://github.com/Homebrew/brew/pull/46","id":146735556,"number":46,"title":"scm/git: prevent fork bomb with 'env :userpaths'","user":{"login":"UniqMartin","id":11892150,"avatar_url":"https://avatars.githubusercontent.com/u/11892150?v=3","gravatar_id":"","url":"https://api.github.com/users/UniqMartin","html_url":"https://github.com/UniqMartin","followers_url":"https://api.github.com/users/UniqMartin/followers","following_url":"https://api.github.com/users/UniqMartin/following{/other_user}","gists_url":"https://api.github.com/users/UniqMartin/gists{/gist_id}","starred_url":"https://api.github.com/users/UniqMartin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/UniqMartin/subscriptions","organizations_url":"https://api.github.com/users/UniqMartin/orgs","repos_url":"https://api.github.com/users/UniqMartin/repos","events_url":"https://api.github.com/users/UniqMartin/events{/privacy}","received_events_url":"https://api.github.com/users/UniqMartin/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2016-04-07T20:37:50Z","updated_at":"2016-04-07T21:22:03Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/Homebrew/brew/pulls/46","html_url":"https://github.com/Homebrew/brew/pull/46","diff_url":"https://github.com/Homebrew/brew/pull/46.diff","patch_url":"https://github.com/Homebrew/brew/pull/46.patch"},"body":"- [x] Have you followed the guidelines in our [Contributing](https://github.com/Homebrew/brew/blob/master/.github/CONTRIBUTING.md) document?\r\n- [x] Have you checked to ensure there aren't other open [Pull Requests](https://github.com/Homebrew/brew/pulls) for the same change?\r\n- [x] Have you added an explanation of what your changes do and why you'd like us to include them?\r\n- [ ] ~~Have you written new tests for your changes?~~\r\n- [x] Have you successfully ran `brew tests` with your changes locally?\r\n\r\n----\r\n\r\nUsing `git` from `Formula#install` can cause a fork bomb if used in a formula with `env :userpaths` because that causes both `Library/ENV/4.3` and `Library/ENV/scm` to be in PATH, both of which contain a `git` binary that is the same SCM wrapper. Those will mutually fork each other indefinitely as they fail to detect that they are the same wrapper.\r\n\r\nExtend the fork-bomb protection to check the paths after all symbolic links have been expanded to prevent this situation. (This incurs a small performance penalty because we need to rely on `Pathname` for Ruby 1.8.7 compatibility, but this wrapper isn't performance critical at all.)\r\n\r\n*Note:* This must have happened in the past occasionally. Commit 8ca79a6df59a7d02ee379e5a766e9170bb26c797 just made it much more likely to happen due to a subtle change. And `boost` is a formula that triggers this behavior.\r\n\r\nFixes #43.\r\nFixes Homebrew/homebrew-core#133."},"comment":{"url":"https://api.github.com/repos/Homebrew/brew/issues/comments/207092841","html_url":"https://github.com/Homebrew/brew/pull/46#issuecomment-207092841","issue_url":"https://api.github.com/repos/Homebrew/brew/issues/46","id":207092841,"user":{"login":"UniqMartin","id":11892150,"avatar_url":"https://avatars.githubusercontent.com/u/11892150?v=3","gravatar_id":"","url":"https://api.github.com/users/UniqMartin","html_url":"https://github.com/UniqMartin","followers_url":"https://api.github.com/users/UniqMartin/followers","following_url":"https://api.github.com/users/UniqMartin/following{/other_user}","gists_url":"https://api.github.com/users/UniqMartin/gists{/gist_id}","starred_url":"https://api.github.com/users/UniqMartin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/UniqMartin/subscriptions","organizations_url":"https://api.github.com/users/UniqMartin/orgs","repos_url":"https://api.github.com/users/UniqMartin/repos","events_url":"https://api.github.com/users/UniqMartin/events{/privacy}","received_events_url":"https://api.github.com/users/UniqMartin/received_events","type":"User","site_admin":false},"created_at":"2016-04-07T21:22:03Z","updated_at":"2016-04-07T21:22:03Z","body":"> Minor technical nitpick: it's not a fork bomb, because it's exec-ing without forking. That's not just a pedantic terminology niggle: it means you can't see the recursion in ps because there's only ever the one process (i.e. it's basically doing tail call optimization :wink:), […]\r\n\r\nTrue; pardon my use of incorrect terminology. Should I fix my commit subject/message?\r\n\r\n> I don't understand why this self-recursion is happening, and I don't really understand why this change would fix it.\r\n\r\nI tried to explain the scenario in my top comment. It's basically `Libraray/ENV/4.3/git` and `Libraray/ENV/scm/git` mutually `exec`-ing each other [in this loop](https://github.com/Homebrew/brew/blob/d5085edce0b5482771c66cfe3e2c013ee3ca90f6/Library/ENV/scm/git#L37-L39) as they are both in PATH, not knowing that they are the same. (They don't invoke themselves because that's prevented by the fork-bomb protection [in this part of the code](https://github.com/Homebrew/brew/blob/d5085edce0b5482771c66cfe3e2c013ee3ca90f6/Library/ENV/scm/git#L22).)\r\n\r\n> More importantly, I found that just adding a debugging call to the `ENV/scm/git` wrapper like this is *also* enough to fix it for me.\r\n\r\nMy guess would be that you tried running this with the sandbox enabled, so that the write to the log file would have killed the wrapper process due to a sandbox violation. (But could be something else; it's really just the first thing that comes to my mind.)\r\n\r\n----\r\n\r\nHere are step-by-step instructions to reproduce the issue locally *before applying my fix*:\r\n\r\n1. Remove/unlink a brewed `git` as that will be in PATH and probably will prevent the bug from happening. Confirmed to happen if the wrapper is forced to fall back to `/usr/bin/git` (typical user installation) that is later resolved to the active developer directory.\r\n\r\n2. Apply this patch to the main Homebrew repository (for debug output):\r\n\r\n  ```diff\r\n  diff --git i/Library/ENV/scm/git w/Library/ENV/scm/git\r\n  index 05148339..019a57c6 100755\r\n  --- i/Library/ENV/scm/git\r\n  +++ w/Library/ENV/scm/git\r\n  @@ -20,6 +20,10 @@ def exec(*args)\r\n     # prevent fork-bombs\r\n     arg0 = args.first\r\n     return if arg0 =~ /^#{F}/i || File.expand_path(arg0) == File.expand_path(__FILE__)\r\n  +  $stderr.puts caller\r\n  +  $stderr.puts \"-- arg0 = #{arg0} => #{File.expand_path(arg0)}\"\r\n  +  $stderr.puts \"-- __FILE__ = #{__FILE__} => #{File.expand_path(__FILE__)}\"\r\n  +  $stderr.puts \">> #{args.inspect}\"\r\n     super\r\n   end\r\n  ```\r\n\r\n3. Apply this patch to the `xz` formula (random choice, something that downloads and extracts quickly):\r\n\r\n  ```diff\r\n  diff --git i/Formula/xz.rb w/Formula/xz.rb\r\n  index 1e27e044..a9908005 100644\r\n  --- i/Formula/xz.rb\r\n  +++ w/Formula/xz.rb\r\n  @@ -16,7 +16,10 @@ class Xz < Formula\r\n\r\n     option :universal\r\n\r\n  +  env :userpaths\r\n     def install\r\n  +    puts ENV[\"PATH\"].inspect\r\n  +    system \"git\", \"--version\"\r\n       ENV.universal_binary if build.universal?\r\n       system \"./configure\", \"--disable-debug\",\r\n                             \"--disable-dependency-tracking\",\r\n  ```\r\n\r\n4. Run `brew install xz`, abort after a few seconds (excerpt of output, shows PATH):\r\n\r\n  ```\r\n  \"/opt/brewery/dummy/Library/ENV/4.3:/opt/brewery/dummy/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/homebrew/Cellar/poison-ruby/1.0/bin::/usr/local/bin:/opt/brewery/dummy/Library/ENV/scm\"\r\n  ==> git --version\r\n  ```\r\n\r\n5. Inspect `HOMEBREW_LOGS/xz/01.git`. Its contents should be pretty convincing (excerpt):\r\n\r\n  ```\r\n  2016-04-07 23:05:36 +0200\r\n\r\n  git\r\n  --version\r\n\r\n  /opt/brewery/dummy/Library/ENV/4.3/git:42:in `block in <main>'\r\n  /opt/brewery/dummy/Library/ENV/4.3/git:41:in `each'\r\n  /opt/brewery/dummy/Library/ENV/4.3/git:41:in `<main>'\r\n  -- arg0 = /opt/brewery/dummy/Library/ENV/scm/git => /opt/brewery/dummy/Library/ENV/scm/git\r\n  -- __FILE__ = /opt/brewery/dummy/Library/ENV/4.3/git => /opt/brewery/dummy/Library/ENV/4.3/git\r\n  >> [\"/opt/brewery/dummy/Library/ENV/scm/git\", \"--version\"]\r\n  /opt/brewery/dummy/Library/ENV/scm/git:42:in `block in <main>'\r\n  /opt/brewery/dummy/Library/ENV/scm/git:41:in `each'\r\n  /opt/brewery/dummy/Library/ENV/scm/git:41:in `<main>'\r\n  -- arg0 = /opt/brewery/dummy/Library/ENV/4.3/git => /opt/brewery/dummy/Library/ENV/4.3/git\r\n  -- __FILE__ = /opt/brewery/dummy/Library/ENV/scm/git => /opt/brewery/dummy/Library/ENV/scm/git\r\n  >> [\"/opt/brewery/dummy/Library/ENV/4.3/git\", \"--version\"]\r\n  /opt/brewery/dummy/Library/ENV/4.3/git:42:in `block in <main>'\r\n  /opt/brewery/dummy/Library/ENV/4.3/git:41:in `each'\r\n  /opt/brewery/dummy/Library/ENV/4.3/git:41:in `<main>'\r\n  -- arg0 = /opt/brewery/dummy/Library/ENV/scm/git => /opt/brewery/dummy/Library/ENV/scm/git\r\n  -- __FILE__ = /opt/brewery/dummy/Library/ENV/4.3/git => /opt/brewery/dummy/Library/ENV/4.3/git\r\n  >> [\"/opt/brewery/dummy/Library/ENV/scm/git\", \"--version\"]\r\n  […]\r\n  ```"}}