Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binstub: set GEM_PATH to '', not nil. #34

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

krallin
Copy link

@krallin krallin commented Oct 17, 2017

Appbundler currently sets GEM_PATH to nil (except when running with
APPBUNDLER_ALLOW_RVM), but this does not fully achieve Appbundler's
isolation goal.

Indeed, when GEM_PATH is set to nil, Rubygems will use a default
path consisting of two locations:

  • The user gem path
  • The gem home (under the Omnibus path)

But, in an Omnibus context, the former shouldn't be there: we'd like to
have full isolation of the Omnibus gems from the rest of the system, so
only those gems from the gem home are relevant.

You can repro this using the following script (clone the ruby/rubygems
repo, then put the script at the root of the repo):

require_relative 'lib/rubygems/path_support.rb'

[
  { 'GEM_HOME' => nil, 'GEM_PATH' => nil },
  { 'GEM_HOME' => nil, 'GEM_PATH' => '' }
].each do |test_case|
  subject = ::Gem::PathSupport.new(test_case)
  puts "case: #{test_case}"
  puts "home: #{subject.home}"
  puts "path: #{subject.path.join(':')}"
  puts
end

Using Ruby from an Omnibus context, the difference is fairly clear:

case: {"GEM_HOME"=>nil, "GEM_PATH"=>nil}
home: /opt/aptible-toolbelt/embedded/lib/ruby/gems/2.3.0
path: /Users/thomas/.gem/ruby/2.3.0:/opt/aptible-toolbelt/embedded/lib/ruby/gems/2.3.0

case: {"GEM_HOME"=>nil, "GEM_PATH"=>""}
home: /opt/aptible-toolbelt/embedded/lib/ruby/gems/2.3.0
path: /opt/aptible-toolbelt/embedded/lib/ruby/gems/2.3.0

That said, this typically doesn't cause issues: the gems on the user's
gem path shouldn't ever be required precisely because Appbundler
pre-activates every gem that the Gemfile.lock predicts will be required.

But, unfortunately, the user gem path can leak through even if no gems
are ever required from it! Indeed, consider an Omnibus package with
"optional" dependencies of this form (potentially themselves located in
a dependency of the Gem being Omnibus-packaged):

begin
  require "readline"
rescue LoadError
end

Assuming readline is not installed in the Omnibus package (presumably
because it's not used there), require will fail, but it won't be the
normal Ruby require.

Instead, it'll use Rubygems' stubbed require, which will scan the Gem
path to try activate a suitable gem for what is being required. But, if
the Gem path includes the user gem path (i.e. what this PR seeks to
fix), there will be 3 problems with this:

  • It's going to be slower.
  • The behavior of the Omnibus package will depends on whether the Gem
    being required happens to be installed in the user gem path.
  • With native gems, it might generate a bunch of warnings / errors
    coming from Ruby gems. This seems to happens because Rubygems logs a
    warning when it notices a gem on the path whose native dependencies
    haven't been built, which it seems may be the case when it finds a gem
    on the user path but Ruby is run from the Omnibus context. The error
    in question looks like this: Ignoring GEM-VERSION because its extensions are not built. Try: gem pristine GEM --version VERSION
    (this doesn't break anything, but it's fairly terrible from a cosmetic
    standpoint).

Setting GEM_PATH to an empty string rather than nil appears to solve
all these issues. Alternatively, the binstub could also set both
GEM_HOME and GEM_PATH to Gem.default_dir if that's a preferred
approach.

Appbundler currently sets `GEM_PATH` to `nil` (except when running with
`APPBUNDLER_ALLOW_RVM`), but this does not fully achieve Appbundler's
isolation goal.

Indeed, when `GEM_PATH` is set to `nil`, Rubygems will use a default
path consisting of two locations:

- The user gem path
- The gem home (under the Omnibus path)

But, in an Omnibus context, the former shouldn't be there: we'd like to
have full isolation of the Omnibus gems from the rest of the system, so
only those gems from the gem home are relevant.

You can repro this using the following script (clone the ruby/rubygems
repo, then put the script at the root of the repo):

```
require_relative 'lib/rubygems/path_support.rb'

[
  { 'GEM_HOME' => nil, 'GEM_PATH' => nil },
  { 'GEM_HOME' => nil, 'GEM_PATH' => '' }
].each do |test_case|
  subject = ::Gem::PathSupport.new(test_case)
  puts "case: #{test_case}"
  puts "home: #{subject.home}"
  puts "path: #{subject.path.join(':')}"
  puts
end
```

Using Ruby from an Omnibus context, the difference is fairly clear:

```
case: {"GEM_HOME"=>nil, "GEM_PATH"=>nil}
home: /opt/aptible-toolbelt/embedded/lib/ruby/gems/2.3.0
path: /Users/thomas/.gem/ruby/2.3.0:/opt/aptible-toolbelt/embedded/lib/ruby/gems/2.3.0

case: {"GEM_HOME"=>nil, "GEM_PATH"=>""}
home: /opt/aptible-toolbelt/embedded/lib/ruby/gems/2.3.0
path: /opt/aptible-toolbelt/embedded/lib/ruby/gems/2.3.0
```

That said, this typically doesn't cause issues: the gems on the user's
gem path shouldn't ever be required precisely because Appbundler
pre-activates every gem that the Gemfile.lock predicts will be required.

But, unfortunately, the user gem path can leak through even if no gems
are ever required from it! Indeed, consider an Omnibus package with
"optional" dependencies of this form (potentially themselves located in
a dependency of the Gem being Omnibus-packaged):

```
begin
  require "readline"
rescue LoadError
end
```

Assuming `readline` is not installed in the Omnibus package (presumably
because it's not used there), `require` will fail, but it won't be the
normal Ruby require.

Instead, it'll use Rubygems' stubbed require, which will scan the Gem
path to try activate a suitable gem for what is being required. But, if
the Gem path includes the user gem path (i.e. what this PR seeks to
fix), there will be 3 problems with this:

- It's going to be slower.
- The behavior of the Omnibus package will depends on whether the Gem
  being required happens to be installed in the user gem path.
- With native gems, it might generate a bunch of warnings / errors
  coming from Ruby gems. This seems to happens because Rubygems logs a
  warning when it notices a gem on the path whose native dependencies
  haven't been built, which it seems may be the case when it finds a gem
  on the user path but Ruby is run from the Omnibus context. The error
  in question looks like this: `Ignoring GEM-VERSION because its
  extensions are not built. Try: gem pristine GEM --version VERSION`
  (this doesn't break anything, but it's fairly terrible from a cosmetic
  standpoint).

Setting `GEM_PATH` to an empty string rather than `nil` appears to solve
all these issues. Alternatively, the binstub could also set both
`GEM_HOME` and `GEM_PATH` to `Gem.default_dir` if that's a preferred
approach.

Signed-off-by: Thomas Orozco <thomas@aptible.com>
@krallin krallin force-pushed the set-gem-paths-instead-of-clear branch from ad981ba to 67a3c4d Compare October 17, 2017 12:07
@krallin
Copy link
Author

krallin commented Oct 17, 2017

I should mention that this is likely to break binstubs for users who currently expect Appbundler to load gems from the user gem path but don't use APPBUNDLER_ALLOW_RVM, but I'm not sure whether that is actually a supported use case for Appbundler.

@krallin
Copy link
Author

krallin commented Dec 4, 2017

Note that this comment i made earlier turned out to be a little optimistic:

That said, this typically doesn't cause issues: the gems on the user's
gem path shouldn't ever be required precisely because Appbundler
pre-activates every gem that the Gemfile.lock predicts will be required.

For example, the json gem might cause a crash here if it's not in Gemfile.lock: if the user has a Ruby installed that is more different from the omnibus'd Ruby (likely), loading the native extension for the json gem might cause a crash (the json gem might not be in the Gemfile since it's built in).

krallin added a commit to krallin/omnibus-aptible-toolbelt that referenced this pull request Dec 5, 2017
@skull-squadron
Copy link

This issue is causing inspec and chef omnibus to break completely on macOS. I had to uninstall them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants