Feature #15920: Check frozen state of ENV

• Status: Closed
• Assignee: -
• Target version: -

[kachick (Kenichi Kamiya)]

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

[jeremyevans0 (Jeremy Evans)]

• File env-frozen.patch env-frozen.patch added

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.

[kachick (Kenichi Kamiya)]

I didn't know assert_separately, thank you!

[shevegen (Robert A. Heiler)]

Agree - looks like a bug to me as well.

[nobu (Nobuyoshi Nakada)]

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?

[jeremyevans0 (Jeremy Evans)]

• File env-no-freeze.patch env-no-freeze.patch added

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.

[Eregon (Benoit Daloze)]

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.

[jeremyevans0 (Jeremy Evans)]

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.

[nobu (Nobuyoshi Nakada)]

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.

[Eregon (Benoit Daloze)]

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.

• 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]

[nobu (Nobuyoshi Nakada)]

• 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)