Improve thread safety of ENV on UNIX

src/crystal/system/unix/env.cr

@@ -1,12 +1,15 @@
 require "c/stdlib"
+require "sync/rw_lock"
 
 module Crystal::System::Env
+  @@lock = Sync::RWLock.new(:unchecked)
+
   # Sets an environment variable.
   def self.set(key : String, value : String) : Nil
     key.check_no_null_byte("key")
     value.check_no_null_byte("value")
 
-    if LibC.setenv(key, value, 1) != 0
+    if @@lock.write { LibC.setenv(key, value, 1) } != 0
       raise RuntimeError.from_errno("setenv")
     end
   end
@@ -15,7 +18,7 @@ module Crystal::System::Env
   def self.set(key : String, value : Nil) : Nil
     key.check_no_null_byte("key")
 
-    if LibC.unsetenv(key) != 0
+    if @@lock.write { LibC.unsetenv(key) } != 0
       raise RuntimeError.from_errno("unsetenv")
     end
   end
@@ -24,32 +27,48 @@ module Crystal::System::Env
   def self.get(key : String) : String?
     key.check_no_null_byte("key")
 
-    if value = LibC.getenv(key)
-      String.new(value)
+    @@lock.read do
+      if value = LibC.getenv(key)
+        String.new(value)
+      end
     end
   end
 
   # Returns `true` if environment variable is set.
   def self.has_key?(key : String) : Bool
     key.check_no_null_byte("key")
 
-    !!LibC.getenv(key)
+    !!@@lock.read { LibC.getenv(key) }
   end
 
   # Iterates all environment variables.
   def self.each(&block : String, String ->)
-    each_pointer do |kv_pointer|
-      # this does `String.new(kv_pointer).partition('=')` without an intermediary string
-      key_value = Slice.new(kv_pointer, LibC.strlen(kv_pointer))
-      split_index = key_value.index!(0x3d_u8) # '='
-      key = String.new(key_value[0, split_index])
-      value = String.new(key_value[split_index + 1..])
+    # Collect variables while holding the lock because we can't trust
+    # LibC.environ to be stable and don't control what &block does: it might
+    # yield the current fiber while holding the lock, deadlock if it calls
+    # Env.set, ...
+    env = Array({String, String}).new
+
+    @@lock.read do
+      each_pointer do |kv_pointer|
+        # this does `String.new(kv_pointer).partition('=')` without an intermediary string
+        key_value = Slice.new(kv_pointer, LibC.strlen(kv_pointer))
+        split_index = key_value.index!(0x3d_u8) # '='
+        key = key_value[0, split_index]
+        value = key_value[split_index + 1..]
+        yield String.new(key), String.new(value)
+      end
+    end
+
+    # now we can safely iterate
+    env.each do |key, value|
       yield key, value
     end
   end
 
-  # Iterates all environment variables as a char pointer to a "KEY=VALUE" string.
-  def self.each_pointer(&block : LibC::Char* ->)
+  # Iterates all environment variables as a char pointer to a "KEY=VALUE"
+  # pointer.
+  private def self.each_pointer(&block : LibC::Char* ->)
     environ_ptr = LibC.environ
     while environ_ptr
       environ_value = environ_ptr.value
@@ -74,29 +93,29 @@ module Crystal::System::Env
   # buffer and `envp` upfront to reduce overall allocations.
   def self.make_envp(env, clear_env) : LibC::Char**
     # When there are no adjustments in `env`, we can take a short cut and return
-    # an empty pointer or the current environment.
-    if env.nil? || env.empty?
-      return clear_env ? Pointer(LibC::Char*).malloc(1) : LibC.environ
+    # an empty pointer.
+    if clear_env && (env.nil? || env.empty?)
+      return Pointer(LibC::Char*).malloc(1)
     end
 
     envp = Array(LibC::Char*).new
 
     unless clear_env
-      Env.each do |key, value|
-        # Skip overrides in `env`
-        next if env.has_key?(key)
+      # Dup LibC.environ, skipping overrides in env.
+      each do |key, value|
+        next if env.try(&.has_key?(key))
 
         envp << "#{key}=#{value}".to_unsafe
       end
     end
 
-    env.each do |key, value|
+    env.try(&.each do |key, value|
       # `nil` value means deleting the key from the inherited environment
       next unless value
 
       raise ArgumentError.new("Invalid env key #{key.inspect}") if key.empty? || key.includes?('=')
       envp << "#{key.check_no_null_byte("key")}=#{value.check_no_null_byte("value")}".to_unsafe
-    end
+    end)
 
     envp << Pointer(LibC::Char).null
 

src/env.cr

@@ -13,6 +13,31 @@ require "crystal/system/env"
 #
 # NOTE: All keys and values are strings. You must take care to cast other types
 # at runtime, e.g. integer port numbers.
+#
+# ### Safety
+#
+# Modifying the environment in single-threaded programs is safe. Modifying the
+# environment is also always safe on Windows.
+#
+# Modifying the environment in multi-threaded programs on other targets is
+# always unsafe, and can cause a mere read to segfault! At best, memory will be
+# leaked every time the environment is modified.
+#
+# The problem is that POSIX systems don't guarantee a thread safe implementation
+# of the `getenv`, `setenv` and `putenv` libc functions. Any thread that gets an
+# environment variable while another thread sets an environment variable may
+# segfault. The `ENV` object itself is internally protected by a readers-writer
+# lock, but we can't protect against external libraries, including libc calls
+# made by the stdlib. They might call `getenv` internally without holding the
+# read lock while a crystal fiber with the write lock calls `setenv`.
+#
+# The only safe solution is to consider `ENV` to be immutable, and to never call
+# `ENV.[]=`, `ENV.delete` or `ENV.clear` in your program. If you really need to,
+# you must make sure that no other thread has been started (beware of libraries
+# that may start threads).
+#
+# NOTE: Passing environment variables to a child process should use the `env`
+# arg of `Process.run` and `Process.new`.
 module ENV
   extend Enumerable({String, String})
 
@@ -34,6 +59,9 @@ module ENV
   # If *value* is `nil`, the environment variable is deleted.
   #
   # If *key* or *value* contains a null-byte an `ArgumentError` is raised.
+  #
+  # WARNING: It is recommended to never set environment variables. See the
+  # Safety section of `ENV` for details.
   def self.[]=(key : String, value : String?)
     Crystal::System::Env.set(key, value)
 
@@ -90,6 +118,9 @@ module ENV
 
   # Removes the environment variable named *key*. Returns the previous value if
   # the environment variable existed, otherwise returns `nil`.
+  #
+  # WARNING: It is recommended to never delete environment variables. See the
+  # Safety section of `ENV` for details.
   def self.delete(key : String) : String?
     if value = self[key]?
       Crystal::System::Env.set(key, nil)
@@ -113,6 +144,8 @@ module ENV
     end
   end
 
+  # WARNING: It is recommended to never delete environment variables. See the
+  # Safety section of `ENV` for details.
   def self.clear : Nil
     keys.each { |k| delete k }
   end