Skip to content

Commit

Permalink
Binstub: set GEM_PATH to '', not nil.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
krallin committed Oct 17, 2017
1 parent 6582b68 commit ad981ba
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 4 deletions.
5 changes: 4 additions & 1 deletion lib/appbundler/app.rb
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,10 @@ def file_format_comment
# exists to make tests run correctly on travis.ci (which uses rvm).
def env_sanitizer
<<-EOS
ENV["GEM_HOME"] = ENV["GEM_PATH"] = nil unless ENV["APPBUNDLER_ALLOW_RVM"] == "true"
unless ENV["APPBUNDLER_ALLOW_RVM"] == "true"
ENV["GEM_HOME"] = nil
ENV["GEM_PATH"] = ''
end
require "rubygems"
::Gem.clear_paths
EOS
Expand Down
15 changes: 12 additions & 3 deletions spec/appbundler/app_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,10 @@ def target_bindir

it "generates code to override GEM_HOME and GEM_PATH (e.g., rvm)" do
expected = <<-EOS
ENV["GEM_HOME"] = ENV["GEM_PATH"] = nil unless ENV["APPBUNDLER_ALLOW_RVM"] == "true"
unless ENV["APPBUNDLER_ALLOW_RVM"] == "true"
ENV["GEM_HOME"] = nil
ENV["GEM_PATH"] = ''
end
require "rubygems"
::Gem.clear_paths
EOS
Expand Down Expand Up @@ -239,7 +242,10 @@ def target_bindir
it "generates runtime activation code for the app" do
expected_gem_activates = if windows?
<<-E
ENV["GEM_HOME"] = ENV["GEM_PATH"] = nil unless ENV["APPBUNDLER_ALLOW_RVM"] == "true"
unless ENV["APPBUNDLER_ALLOW_RVM"] == "true"
ENV["GEM_HOME"] = nil
ENV["GEM_PATH"] = ''
end
require "rubygems"
::Gem.clear_paths
Expand Down Expand Up @@ -304,7 +310,10 @@ def target_bindir
E
else
<<-E
ENV["GEM_HOME"] = ENV["GEM_PATH"] = nil unless ENV["APPBUNDLER_ALLOW_RVM"] == "true"
unless ENV["APPBUNDLER_ALLOW_RVM"] == "true"
ENV["GEM_HOME"] = nil
ENV["GEM_PATH"] = ''
end
require "rubygems"
::Gem.clear_paths
Expand Down

0 comments on commit ad981ba

Please sign in to comment.