Bug #3812: (mswin|mingw) ENV[]= fails silently to set huge values

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: ruby 1.9.3dev (2010-09-08 trunk 29195) [i386-mswin32_100]

[pweldon (Peter Weldon)]

=begin
On mswin&mingw the size of an environment key, value pair is bounded by _MAX_ENV. Calls to putenv() or _putenv() that exceed this will fail and not set the value.

Trivial patch that adds test case and error checking to raise Errno::EINVAL on failure.
=end

[nobu (Nobuyoshi Nakada)]

=begin
Hi,

At Fri, 10 Sep 2010 02:52:07 +0900,
Peter Weldon wrote in [ruby-core:32250]:

On mswin&mingw the size of an environment key, value pair is
bounded by _MAX_ENV. Calls to putenv() or _putenv() that
exceed this will fail and not set the value.

Thank you, but alloca is not safe for such huge values. IIRC,
it could return invalid address silently.

diff --git a/hash.c b/hash.c
index 8bba586..638f493 100644
--- a/hash.c
+++ b/hash.c
@@ -2139,28 +2139,24 @@ void
ruby_setenv(const char *name, const char *value)
{
#if defined(_WIN32)

• int len;
• char *buf;

• VALUE buf;
  if (strchr(name, '=')) {

•  fail:
  

  errno = EINVAL;
  rb_sys_fail("ruby_setenv");
  }
• len = strlen(name) + 1 + 1;
  if (value) {

• len = strlen(name) + 1 + strlen(value) + 1;
• buf = ALLOCA_N(char, len);
• snprintf(buf, len, "%s=%s", name, value);
• putenv(buf);
• /* putenv() doesn't handle empty value */
• if (!*value)

•  SetEnvironmentVariable(name,value);
  

• buf = rb_sprintf("%s=%s", name, value);
  }
  else {

• len = strlen(name) + 1 + 1;
• buf = ALLOCA_N(char, len);
• snprintf(buf, len, "%s=", name);
• putenv(buf);
• SetEnvironmentVariable(name, 0);

• buf = rb_sprintf("%s=", name);
• }
• if (putenv(RSTRING_PTR(buf))) goto fail;
• rb_str_resize(buf);
• if (!value || !*value) {
• /* putenv() doesn't handle empty value */
• if (SetEnvironmentVariable(name,value)) goto fail;
  }
  #elif defined(HAVE_SETENV) && defined(HAVE_UNSETENV)
  #undef setenv
  

--
Nobu Nakada

=end

[pweldon (Peter Weldon)]

=begin
Thank you for the better patch. Fixes to make it compile, new test and fixed broken test:

---

hash.c | 25 ++++++++++---------------
test/ruby/test_env.rb | 13 +++++++++++++
test/ruby/test_require.rb | 12 ++++--------
3 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/hash.c b/hash.c
index 8bba586..1e3b30b 100644
--- a/hash.c
+++ b/hash.c
@@ -2139,28 +2139,23 @@ void
ruby_setenv(const char *name, const char *value)
{
#if defined(_WIN32)

• int len;
• char *buf;

• VALUE buf;
  if (strchr(name, '=')) {

•  fail:
  

  errno = EINVAL;
  rb_sys_fail("ruby_setenv");
  }
  if (value) {

• len = strlen(name) + 1 + strlen(value) + 1;
• buf = ALLOCA_N(char, len);
• snprintf(buf, len, "%s=%s", name, value);
• putenv(buf);
• /* putenv() doesn't handle empty value */
• if (!*value)

•  SetEnvironmentVariable(name,value);
  

• buf = rb_sprintf("%s=%s", name, value);
  }
  else {

• len = strlen(name) + 1 + 1;
• buf = ALLOCA_N(char, len);
• snprintf(buf, len, "%s=", name);
• putenv(buf);
• SetEnvironmentVariable(name, 0);

• buf = rb_sprintf("%s=", name);
• }
• if (putenv(RSTRING_PTR(buf))) goto fail;
• rb_str_resize(buf,0);
• if (!value || !*value) {
• /* putenv() doesn't handle empty value */
• if (!SetEnvironmentVariable(name,value)) goto fail;
  }
  #elif defined(HAVE_SETENV) && defined(HAVE_UNSETENV)
  #undef setenv
  diff --git a/test/ruby/test_env.rb b/test/ruby/test_env.rb
  index 785b6bb..52b95e3 100644
  --- a/test/ruby/test_env.rb
  +++ b/test/ruby/test_env.rb
  @@ -1,4 +1,5 @@
  require 'test/unit'
  +require 'rbconfig'

class TestEnv < Test::Unit::TestCase
IGNORE_CASE = /bccwin|mswin|mingw/ =~ RUBY_PLATFORM
@@ -374,4 +375,16 @@ class TestEnv < Test::Unit::TestCase
ENV.update({"baz"=>"quux","a"=>"b"}) {|k, v1, v2| v1 ? k + "" + v1 + "" + v2 : v2 }
check(ENV.to_hash.to_a, [%w(foo bar), %w(baz baz_qux_quux), %w(a b)])
end
+

• def test_huge_value
• huge_value = "bar" * 40960
• ENV["foo"] = "bar"
• if Config::CONFIG['host_os'] =~ /mswin|mingw/

•  assert_raise(Errno::EINVAL) { ENV["foo"] = huge_value }
  

•  assert_equal("bar", ENV["foo"])
  

• else

•  assert_nothing_raised { ENV["foo"] = huge_value }
  

•  assert_equal(huge_value, ENV["foo"])
  

• end
• end
  end
  diff --git a/test/ruby/test_require.rb b/test/ruby/test_require.rb
  index f87de90..f8e8edd 100644
  --- a/test/ruby/test_require.rb
  +++ b/test/ruby/test_require.rb
  @@ -49,18 +49,14 @@ class TestRequire < Test::Unit::TestCase
  def test_require_path_home
  env_rubypath, env_home = ENV["RUBYPATH"], ENV["HOME"]

• ENV["RUBYPATH"] = "~"
• ENV["HOME"] = "/foo" * 10000
• assert_in_out_err(%w(-S test_ruby_test_require), "", [], /^.+$/)
• ENV["RUBYPATH"] = "~" + "/foo" * 10000
• ENV["HOME"] = "/foo"
• assert_in_out_err(%w(-S test_ruby_test_require), "", [], /^.+$/)
• t = Tempfile.new(["test_ruby_test_require", ".rb"])
  t.puts "p :ok"
  t.close

• ENV["RUBYPATH"] = "~"
• ENV["HOME"] = t.path
• assert_in_out_err(%w(-S test_ruby_test_require), "", [], /(LoadError)/)
• ENV["HOME"], name = File.split(t.path)
  assert_in_out_err(["-S", name], "", %w(:ok), [])

--

=end

[luislavena (Luis Lavena)]

=begin
I would recommend usage of RbConfig::CONFIG instead of Config::CONFIG due it's deprecation warning.

=end

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed
• % Done changed from 0 to 100

=begin
This issue was solved with changeset r29225.
Peter, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

=end

[nobu (Nobuyoshi Nakada)]

• Status changed from Closed to Assigned
• Assignee set to yugui (Yuki Sonoda)

=begin

=end

[nobu (Nobuyoshi Nakada)]

• Status changed from Assigned to Closed
• Assignee deleted (yugui (Yuki Sonoda))

=begin

=end