Akira, isn't it obvious this patch introduce a backward incompatibility?

I'd be surprised if it could passed the tests, because that means our tests are insufficient.  Is it that important to pass make check at this point?

I don't think it's an "at least" thing for this rather conceptual patch.  Is it your only concern?

Of course tests shall be modified to fit this when it is going to land.

On 10/10/2014 02:02 AM, akr / fsij.org wrote:
> Issue #10351 has been updated by Akira Tanaka.
> 
> 
> It is very aggressive.
> 
> At least, you should try "make check".
> 
> ```
> % make check
> ...
> ./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems "./test/runner.rb" --ruby="./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems"  
> mkmf.rb can't find header files for ruby at /home/ruby/tst1/lib/ruby/include/ruby.h
> uncommon.mk:549: recipe for target 'yes-test-all' failed
> make: *** [yes-test-all] Error 1
> ```
> 
> ----------------------------------------
> Feature #10351: [PATCH] prevent CVE-2014-6277
> https://bugs.ruby-lang.org/issues/10351#change-49322
> 
> * Author: Shyouhei Urabe
> * Status: Open
> * Priority: Normal
> * Assignee: 
> * Category: core
> * Target version: current: 2.2.0
> ----------------------------------------
> ~~~
>>From 4636ca0308f1933c9b191f36e808a8d3bcf5e88e Mon Sep 17 00:00:00 2001
> From: "Urabe, Shyouhei" <shyouhei / ruby-lang.org>
> Date: Wed, 8 Oct 2014 15:44:27 +0900
> Subject: [PATCH] prevent CVE-2014-6277
> 
> ShellShock was about bash.  I think ruby is torellant for that kind of
> attack.  However the concept was that environment variables can
> potentially contain malicious data.  To pass them to victim
> subprocecsses can sometimes cause catastrophic situation, like
> arbitrary code execution.  Even though ruby itself is not affected, it
> can be an accomplice in attacks by blindly passing through any
> uncontrolled info through.  Let's just change this, more secure by
> default.
> 
> This patch does not add a new feature, nor delete anything.  It just
> changes the default behaviour when ruby spawns subprocesses.
> 
>     Process.spawn('/usr/bin/printenv') # -> prints nothing
> 
> You can explicitly pass what you need:
> 
>     Process.spawn({'FOO'=>'BAR'}, '/usr/bin/printenv')
> 
> Or if you are 128% sure what you are doing, can pass everything.
> 
>     ENV['FOO'] = 'BAR'
>     Process.spawn('/usr/bin/printenv', unsetenv_others: false)
> 
> I don't intend to make things impossible; just give it a better
> default.  It doesn't ultimately solve everything but it should prevent
> casual faults.
> 
> ---
>  process.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/process.c b/process.c
> index f9bc01c..395333d 100644
> --- a/process.c
> +++ b/process.c
> @@ -2295,7 +2295,7 @@ rb_execarg_fixup(VALUE execarg_obj)
>          eargp->dup2_tmpbuf = tmpbuf;
>      }
>  
> -    unsetenv_others = eargp->unsetenv_others_given && eargp->unsetenv_others_do;
> +    unsetenv_others = !eargp->unsetenv_others_given || eargp->unsetenv_others_do;
>      envopts = eargp->env_modification;
>      if (unsetenv_others || envopts != Qfalse) {
>          VALUE envtbl, envp_str, envp_buf;
> @@ -2936,7 +2936,7 @@ rb_execarg_run_options(const struct rb_execarg *eargp, struct rb_execarg *sargp,
>  #endif
>  
>  #if !defined(HAVE_WORKING_FORK)
> -    if (eargp->unsetenv_others_given && eargp->unsetenv_others_do) {
> +    if (!eargp->unsetenv_others_given || eargp->unsetenv_others_do) {
>          save_env(sargp);
>          rb_env_clear();
>      }
> @@ -3998,8 +3998,8 @@ rb_f_system(int argc, VALUE *argv)
>   *      [cmdname, argv0], arg1, ... : command name, argv[0] and zero or more arguments (no shell)
>   *    options: hash
>   *      clearing environment variables:
> - *        :unsetenv_others => true   : clear environment variables except specified by env
> - *        :unsetenv_others => false  : don't clear (default)
> + *        :unsetenv_others => true   : clear environment variables except specified by env (default)
> + *        :unsetenv_others => false  : don't clear
>   *      process group:
>   *        :pgroup => true or 0 : make a new process group
>   *        :pgroup => pgid      : join to specified process group
>