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

Process.spawn doesn't remove nil env variables #3428

Closed
smellsblue opened this issue Oct 27, 2015 · 2 comments · Fixed by #5907
Closed

Process.spawn doesn't remove nil env variables #3428

smellsblue opened this issue Oct 27, 2015 · 2 comments · Fixed by #5907

Comments

@smellsblue
Copy link
Contributor

@smellsblue smellsblue commented Oct 27, 2015

Process.spawn is supposed to unset environment variables that are explicitly specified as nil. Instead, the environment variable is being set as "". This happens even if the environment variable didn't exist before calling Process.spawn.

Below is a sample script demonstrating the problem:

#!/usr/bin/env ruby
if ARGV.first != "spawned"
  Process.wait Process.spawn({ "SHOULD_BE_NIL" => nil }, __FILE__, "spawned")
  exit
end

if ENV["SHOULD_BE_NIL"].nil?
  puts "success!"
else
  puts "failure: #{ENV["SHOULD_BE_NIL"].inspect}"
end

When run on Ruby 2.2.3 and JRuby 1.7.22, it works as expected... I get success!, but on JRuby 9.0.3.0, I get failure: "".

I'd be happy to work on a fix for this if it hasn't already been solved (I found some similar looking issues open, but couldn't find one that quite matched it).

@mojavelinux
Copy link

@mojavelinux mojavelinux commented Apr 2, 2019

I just ran across this today. @smellsblue are you still interested in submitting a fix?

The importance of this issue is compounded by the fact that Process.spawn on JRuby doesn't support the :unsetenv_others option (we get: "clearing env in child is not supported"). So there's no way of removing an environment variable which is already set directly from Process.spawn (or any method that relies on it).

The workaround is to manipulate the ENV variable directly, though that impacts the parent process too.

@headius
Copy link
Member

@headius headius commented Apr 9, 2019

@smellsblue We'd love the help! The link I show down below in the discussion of :unsetenv_others is right in the heart of the logic, roughly "ported" from the way MRI handles the various spawn forms. This may be a simple oversight, where we should be scrubbing the env of any of these variables specified as nil. It may be something that was fixed later in CRuby, too...I did the original work on this code a couple years ago.

Relating to unsetenv, which is likely intertwined with this logic...

The logic here is largely modeled after what's in MRI, drastically modified in most places to accommodate the limitations of posix_spawn, which we use to mostly simulate fork+exec.

I did not translate any of the logic from MRI's unsetenv_others logic, and left this comment for future generations to ponder over:

// only way to do this is manually build a list of env assignments that clear all parent values

Let us wonder at the mystery.

I believe what I was saying here is that because posix_spawn only has a way to specify a full environment, the JRuby version of this logic would need to duplicate the existing env and dig out the ones that are to be cleared.

In CRuby, they do this "the easy way" by forking, altering the environment, and then execing the new process which inherits this in-place modified environment. We can't modify the in-place environment without introducing some nasty side effects for the parent JVM, so the alternative is to manually construct a completely new env and pass that to posix_spawn. At the time, that was not a critical path item for us.

And largely, I've just missed this issue. This subsystem is one of my unfortunate creations, and there's plenty of holes where I lost the tune.

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 a pull request may close this issue.

4 participants