Project

General

Profile

Actions

Feature #15920

closed

Check frozen state of ENV

Added by kachick (Kenichi Kamiya) almost 5 years ago. Updated over 4 years ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:93104]

Description

github: https://github.com/ruby/ruby/pull/2234

Is this an intentional behavior?

ENV.freeze
ENV.clear #=> No exception happen

raising FronzenError sounds reasonable to me


Files

env-frozen.patch (3.9 KB) env-frozen.patch jeremyevans0 (Jeremy Evans), 06/13/2019 07:45 PM
env-no-freeze.patch (1.78 KB) env-no-freeze.patch jeremyevans0 (Jeremy Evans), 06/14/2019 11:55 PM

Updated by jeremyevans0 (Jeremy Evans) almost 5 years ago

I think this is a bug and we should fix it. I took a slightly differently approach in the attached patch, using rb_check_frozen directly instead of introducing a new function, and combining all of the tests for behavior when frozen into a single method.

Updated by kachick (Kenichi Kamiya) almost 5 years ago

I didn't know assert_separately, thank you!

Updated by shevegen (Robert A. Heiler) almost 5 years ago

Agree - looks like a bug to me as well.

Updated by nobu (Nobuyoshi Nakada) almost 5 years ago

I don't think this behavior a bug.
ENV is an interface to the system environment variables, so freezing it doesn't affect the underlying system.
And be careful that an object can never be unfrozen anymore, once it got frozen.
For what purpose do you need to prohibit modifying environment variables, and is ENV.freeze good for it?

Updated by jeremyevans0 (Jeremy Evans) almost 5 years ago

nobu (Nobuyoshi Nakada) wrote:

I don't think this behavior a bug.
ENV is an interface to the system environment variables, so freezing it doesn't affect the underlying system.

I think the current situation of ENV.freeze not raising an exception but still allowing mutation is a bug. We have two ways to fix it:

  • Disallow modifying ENV after ENV.freeze (my original patch).
  • Disallow ENV.freeze. Attached is a another patch that makes ENV.freeze raise an exception (I chose TypeError).

And be careful that an object can never be unfrozen anymore, once it got frozen.
For what purpose do you need to prohibit modifying environment variables, and is ENV.freeze good for it?

In general, unless you expect to modify the environment, it would be best to freeze it in production applications. I actually think it is best to freeze as much as possible. Any object where modification is not required after startup should be frozen as a general principle, in my opinion. This practice is not yet common in Ruby, though.

Updated by Eregon (Benoit Daloze) almost 5 years ago

One potential concern here is that even if ENV is frozen, C extensions, or native libraries linked by C extensions, might still call e.g., setenv() and effectively modify values returned by ENV (and even add new environment variables).

Raising on freeze seems uncommon, but it might make sense here since there is not real way to completely freeze ENV.

Updated by jeremyevans0 (Jeremy Evans) almost 5 years ago

Eregon (Benoit Daloze) wrote:

One potential concern here is that even if ENV is frozen, C extensions, or native libraries linked by C extensions, might still call e.g., setenv() and effectively modify values returned by ENV (and even add new environment variables).

C extensions and natively libraries can modify frozen Ruby objects (even unfreeze objects), so I don't see the how ENV is special.

Raising on freeze seems uncommon, but it might make sense here since there is not real way to completely freeze ENV.

My preference would be to support ENV.freeze, and disallow modifications to ENV afterward, but I'm fine with ENV.freeze raising. I think the current behavior in this area is a bug, though.

Updated by nobu (Nobuyoshi Nakada) almost 5 years ago

jeremyevans0 (Jeremy Evans) wrote:

  • Disallow modifying ENV after ENV.freeze (my original patch).

I don't object this behavior itself, just don't consider the current behavior a bug, that is I don't think this should be backported.

Updated by Eregon (Benoit Daloze) almost 5 years ago

jeremyevans0 (Jeremy Evans) wrote:

Eregon (Benoit Daloze) wrote:

One potential concern here is that even if ENV is frozen, C extensions, or native libraries linked by C extensions, might still call e.g., setenv() and effectively modify values returned by ENV (and even add new environment variables).

C extensions and natively libraries can modify frozen Ruby objects (even unfreeze objects), so I don't see the how ENV is special.

I would call that entirely unsupported, and if there are ways to detect this, we should probably raise an exception.
Basically, I believe it's undefined behavior to unfreeze an object or to change its internals.
Doing that breaks the most fundamental guarantee of Kernel#freeze, i.e., that the object is immutable and therefore free from data races.

OTOH, calling setenv() in a native library is fairly well-defined and there is nothing we can do to prevent that.
ENV is special because it's basically just a wrapper of environ, and we can't control if that gets modified.

Freezing ENV cannot be done reliably, so I believe it should raise and not pretend it's frozen when it's not.

Actions #10

Updated by jeremyevans (Jeremy Evans) over 4 years ago

  • Status changed from Open to Closed

Applied in changeset git|f53d7e4bfd604be6f8131c5460c29f4af16da117.


Raise TypeError if calling ENV.freeze

Previously, you could call ENV.freeze, but it would not have
the desired effect, as you could still modify ENV.

Fixes [Bug #15920]

Actions #11

Updated by nobu (Nobuyoshi Nakada) over 4 years ago

  • Tracker changed from Bug to Feature
  • ruby -v deleted (ruby 2.6.3p62 (2019-04-16 revision 67580) [x86_64-darwin18])
  • Backport deleted (2.4: UNKNOWN, 2.5: UNKNOWN, 2.6: UNKNOWN)
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0