Project

General

Profile

Actions

Bug #3812

closed

(mswin|mingw) ENV[]= fails silently to set huge values

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

Added by pweldon (Peter Weldon) about 15 years ago. Updated over 14 years ago.

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

Description

=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


Files

setenv.patch (2.28 KB) setenv.patch pweldon (Peter Weldon), 09/10/2010 02:50 AM

Updated by nobu (Nobuyoshi Nakada) about 15 years ago Actions #1

=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

Updated by pweldon (Peter Weldon) about 15 years ago Actions #2

=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

Updated by luislavena (Luis Lavena) about 15 years ago Actions #3

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

=end

Updated by nobu (Nobuyoshi Nakada) about 15 years ago Actions #4

  • 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

Updated by nobu (Nobuyoshi Nakada) about 15 years ago Actions #5

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

=begin

=end

Updated by nobu (Nobuyoshi Nakada) about 15 years ago Actions #6

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

=begin

=end

Actions

Also available in: PDF Atom