Remove tracking of environment from osenv.nim v2 (#18575)

* Remove unnecessary environment tracking

* try to fix windows

* fix delEnv

* make putEnv work on windows even with empty values; improve tests: add tests, add js, vm testing

* [skip ci] fix changelog

Co-authored-by: Caden Haustein <code@brightlysalty.33mail.com>

changelog.md

@@ -97,11 +97,14 @@
   The downside is that these defines now have custom logic that doesn't apply for
   other defines.
 
+- `std/os`: `putEnv` now raises if the 1st argument contains a `=`
+
 - Renamed `-d:nimCompilerStackraceHints` to `-d:nimCompilerStacktraceHints`.
 
 - In `std/dom`, `Interval` is now a `ref object`, same as `Timeout`. Definitions of `setTimeout`,
   `clearTimeout`, `setInterval`, `clearInterval` were updated.
 
+
 ## Standard library additions and changes
 
 - `strformat`:

lib/pure/includes/osenv.nim

@@ -40,112 +40,15 @@ when defined(nodejs):
 #   {.error: "requires -d:nodejs".}
 
 else:
-  when defined(windows):
-    from parseutils import skipIgnoreCase
 
   proc c_getenv(env: cstring): cstring {.
     importc: "getenv", header: "<stdlib.h>".}
-  when defined(vcc):
+  when defined(windows):
     proc c_putenv_s(envname: cstring, envval: cstring): cint {.importc: "_putenv_s", header: "<stdlib.h>".}
+    from std/private/win_setenv import setEnvImpl
   else:
     proc c_setenv(envname: cstring, envval: cstring, overwrite: cint): cint {.importc: "setenv", header: "<stdlib.h>".}
-  proc c_unsetenv(env: cstring): cint {.
+  proc c_unsetenv(env: cstring): cint {.importc: "unsetenv", header: "<stdlib.h>".}
-    importc: "unsetenv", header: "<stdlib.h>".}
-
-  # Environment handling cannot be put into RTL, because the `envPairs`
-  # iterator depends on `environment`.
-
-  var
-    envComputed {.threadvar.}: bool
-    environment {.threadvar.}: seq[string]
-
-  when defined(nimV2):
-    proc unpairedEnvAllocs*(): int =
-      result = environment.len
-      if result > 0: inc result
-
-  when defined(windows) and not defined(nimscript):
-    # because we support Windows GUI applications, things get really
-    # messy here...
-    when useWinUnicode:
-      when defined(cpp):
-        proc strEnd(cstr: WideCString, c = 0'i32): WideCString {.
-          importcpp: "(NI16*)wcschr((const wchar_t *)#, #)", header: "<string.h>".}
-      else:
-        proc strEnd(cstr: WideCString, c = 0'i32): WideCString {.
-          importc: "wcschr", header: "<string.h>".}
-    else:
-      proc strEnd(cstr: cstring, c = 0'i32): cstring {.
-        importc: "strchr", header: "<string.h>".}
-
-    proc getEnvVarsC() =
-      if not envComputed:
-        environment = @[]
-        when useWinUnicode:
-          var
-            env = getEnvironmentStringsW()
-            e = env
-          if e == nil: return # an error occurred
-          while true:
-            var eend = strEnd(e)
-            add(environment, $e)
-            e = cast[WideCString](cast[ByteAddress](eend)+2)
-            if eend[1].int == 0: break
-          discard freeEnvironmentStringsW(env)
-        else:
-          var
-            env = getEnvironmentStringsA()
-            e = env
-          if e == nil: return # an error occurred
-          while true:
-            var eend = strEnd(e)
-            add(environment, $e)
-            e = cast[cstring](cast[ByteAddress](eend)+1)
-            if eend[1] == '\0': break
-          discard freeEnvironmentStringsA(env)
-        envComputed = true
-
-  else:
-    const
-      useNSGetEnviron = (defined(macosx) and not defined(ios) and not defined(emscripten)) or defined(nimscript)
-
-    when useNSGetEnviron:
-      # From the manual:
-      # Shared libraries and bundles don't have direct access to environ,
-      # which is only available to the loader ld(1) when a complete program
-      # is being linked.
-      # The environment routines can still be used, but if direct access to
-      # environ is needed, the _NSGetEnviron() routine, defined in
-      # <crt_externs.h>, can be used to retrieve the address of environ
-      # at runtime.
-      proc NSGetEnviron(): ptr cstringArray {.
-        importc: "_NSGetEnviron", header: "<crt_externs.h>".}
-    elif defined(haiku):
-      var gEnv {.importc: "environ", header: "<stdlib.h>".}: cstringArray
-    else:
-      var gEnv {.importc: "environ".}: cstringArray
-
-    proc getEnvVarsC() =
-      # retrieves the variables of char** env of C's main proc
-      if not envComputed:
-        environment = @[]
-        when useNSGetEnviron:
-          var gEnv = NSGetEnviron()[]
-        var i = 0
-        while gEnv[i] != nil:
-          add environment, $gEnv[i]
-          inc(i)
-        envComputed = true
-
-  proc findEnvVar(key: string): int =
-    getEnvVarsC()
-    var temp = key & '='
-    for i in 0..high(environment):
-      when defined(windows):
-        if skipIgnoreCase(environment[i], temp) == len(temp): return i
-      else:
-        if startsWith(environment[i], temp): return i
-    return -1
 
   proc getEnv*(key: string, default = ""): string {.tags: [ReadEnvEffect].} =
     ## Returns the value of the `environment variable`:idx: named `key`.
@@ -163,16 +66,9 @@ else:
       assert getEnv("unknownEnv") == ""
       assert getEnv("unknownEnv", "doesn't exist") == "doesn't exist"
 
-    when nimvm:
+    let env = c_getenv(key)
-      discard "built into the compiler"
+    if env == nil: return default
-    else:
+    result = $env
-      var i = findEnvVar(key)
-      if i >= 0:
-        return substr(environment[i], find(environment[i], '=')+1)
-      else:
-        var env = c_getenv(key)
-        if env == nil: return default
-        result = $env
 
   proc existsEnv*(key: string): bool {.tags: [ReadEnvEffect].} =
     ## Checks whether the environment variable named `key` exists.
@@ -186,11 +82,7 @@ else:
     runnableExamples:
       assert not existsEnv("unknownEnv")
 
-    when nimvm:
+    return c_getenv(key) != nil
-      discard "built into the compiler"
-    else:
-      if c_getenv(key) != nil: return true
-      else: return findEnvVar(key) >= 0
 
   proc putEnv*(key, val: string) {.tags: [WriteEnvEffect].} =
     ## Sets the value of the `environment variable`:idx: named `key` to `val`.
@@ -201,33 +93,14 @@ else:
     ## * `existsEnv proc <#existsEnv,string>`_
     ## * `delEnv proc <#delEnv,string>`_
     ## * `envPairs iterator <#envPairs.i>`_
-
+    when defined(windows):
-    # Note: by storing the string in the environment sequence,
+      if key.len == 0 or '=' in key:
-    # we guarantee that we don't free the memory before the program
+        raise newException(OSError, "invalid key, got: " & $(key, val))
-    # ends (this is needed for POSIX compliance). It is also needed so that
+      if setEnvImpl(key, val, 1'i32) != 0'i32:
-    # the process itself may access its modified environment variables!
+        raiseOSError(osLastError(), $(key, val))
-    when nimvm:
-      discard "built into the compiler"
     else:
-      var indx = findEnvVar(key)
+      if c_setenv(key, val, 1'i32) != 0'i32:
-      if indx >= 0:
+        raiseOSError(osLastError(), $(key, val))
-        environment[indx] = key & '=' & val
-      else:
-        add environment, (key & '=' & val)
-        indx = high(environment)
-      when defined(windows) and not defined(nimscript):
-        when useWinUnicode:
-          var k = newWideCString(key)
-          var v = newWideCString(val)
-          if setEnvironmentVariableW(k, v) == 0'i32: raiseOSError(osLastError())
-        else:
-          if setEnvironmentVariableA(key, val) == 0'i32: raiseOSError(osLastError())
-      elif defined(vcc):
-        if c_putenv_s(key, val) != 0'i32:
-          raiseOSError(osLastError())
-      else:
-        if c_setenv(key, val, 1'i32) != 0'i32:
-          raiseOSError(osLastError())
 
   proc delEnv*(key: string) {.tags: [WriteEnvEffect].} =
     ## Deletes the `environment variable`:idx: named `key`.
@@ -238,21 +111,45 @@ else:
     ## * `existsEnv proc <#existsEnv,string>`_
     ## * `putEnv proc <#putEnv,string,string>`_
     ## * `envPairs iterator <#envPairs.i>`_
-    when nimvm:
+    template bail = raiseOSError(osLastError(), key)
-      discard "built into the compiler"
+    when defined(windows):
+      #[ 
+      # https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/putenv-s-wputenv-s?view=msvc-160
+      > You can remove a variable from the environment by specifying an empty string (that is, "") for value_string
+      note that nil is not legal
+      ]#
+      if key.len == 0 or '=' in key:
+        raise newException(OSError, "invalid key, got: " & key)
+      if c_putenv_s(key, "") != 0'i32: bail
     else:
-      var indx = findEnvVar(key)
+      if c_unsetenv(key) != 0'i32: bail
-      if indx < 0: return # Do nothing if the env var is not already set
+
-      when defined(windows) and not defined(nimscript):
+  when defined(windows):
-        when useWinUnicode:
+    when useWinUnicode:
-          var k = newWideCString(key)
+      when defined(cpp):
-          if setEnvironmentVariableW(k, nil) == 0'i32: raiseOSError(osLastError())
+        proc strEnd(cstr: WideCString, c = 0'i32): WideCString {.importcpp: "(NI16*)wcschr((const wchar_t *)#, #)",
-        else:
+            header: "<string.h>".}
-          if setEnvironmentVariableA(key, nil) == 0'i32: raiseOSError(osLastError())
       else:
-        if c_unsetenv(key) != 0'i32:
+        proc strEnd(cstr: WideCString, c = 0'i32): WideCString {.importc: "wcschr",
-          raiseOSError(osLastError())
+            header: "<string.h>".}
-      environment.delete(indx)
+    else:
+      proc strEnd(cstr: cstring, c = 0'i32): cstring {.importc: "strchr",
+          header: "<string.h>".}
+  elif defined(macosx) and not defined(ios) and not defined(emscripten):
+    # From the manual:
+    # Shared libraries and bundles don't have direct access to environ,
+    # which is only available to the loader ld(1) when a complete program
+    # is being linked.
+    # The environment routines can still be used, but if direct access to
+    # environ is needed, the _NSGetEnviron() routine, defined in
+    # <crt_externs.h>, can be used to retrieve the address of environ
+    # at runtime.
+    proc NSGetEnviron(): ptr cstringArray {.importc: "_NSGetEnviron",
+        header: "<crt_externs.h>".}
+  elif defined(haiku):
+    var gEnv {.importc: "environ", header: "<stdlib.h>".}: cstringArray
+  else:
+    var gEnv {.importc: "environ".}: cstringArray
 
   iterator envPairs*(): tuple[key, value: string] {.tags: [ReadEnvEffect].} =
     ## Iterate over all `environments variables`:idx:.
@@ -265,8 +162,30 @@ else:
     ## * `existsEnv proc <#existsEnv,string>`_
     ## * `putEnv proc <#putEnv,string,string>`_
     ## * `delEnv proc <#delEnv,string>`_
-    getEnvVarsC()
+    when defined(windows):
-    for i in 0..high(environment):
+      block:
-      var p = find(environment[i], '=')
+        template impl(get_fun, typ, size, zero, free_fun) =
-      yield (substr(environment[i], 0, p-1),
+          let env = get_fun()
-             substr(environment[i], p+1))
+          var e = env
+          if e == nil: break
+          while true:
+            let eend = strEnd(e)
+            let kv = $e
+            let p = find(kv, '=')
+            yield (substr(kv, 0, p-1), substr(kv, p+1))
+            e = cast[typ](cast[ByteAddress](eend)+size)
+            if typeof(zero)(eend[1]) == zero: break
+          discard free_fun(env)
+        when useWinUnicode:
+          impl(getEnvironmentStringsW, WideCString, 2, 0, freeEnvironmentStringsW)
+        else:
+          impl(getEnvironmentStringsA, cstring, 1, '\0', freeEnvironmentStringsA)
+    else:
+      var i = 0
+      when defined(macosx) and not defined(ios) and not defined(emscripten):
+        var gEnv = NSGetEnviron()[]
+      while gEnv[i] != nil:
+        let kv = $gEnv[i]
+        inc(i)
+        let p = find(kv, '=')
+        yield (substr(kv, 0, p-1), substr(kv, p+1))

lib/std/private/win_setenv.nim

@@ -0,0 +1,92 @@
+#[
+Copyright (c) Facebook, Inc. and its affiliates.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+
+Adapted `setenv` from https://github.com/facebook/folly/blob/master/folly/portability/Stdlib.cpp
+translated from C to nim.
+]#
+
+#[
+Introduced in https://github.com/facebook/folly/commit/5d8ca09a3f96afefb44e35808f03651a096ab9c7
+
+TODO:
+check errno_t vs cint
+]#
+
+when not defined(windows): discard
+else:
+  proc setEnvironmentVariableA*(lpName, lpValue: cstring): int32 {.
+    stdcall, dynlib: "kernel32", importc: "SetEnvironmentVariableA", sideEffect.}
+    # same as winlean.setEnvironmentVariableA
+
+  proc c_getenv(env: cstring): cstring {.importc: "getenv", header: "<stdlib.h>".}
+  proc c_putenv_s(envname: cstring, envval: cstring): cint {.importc: "_putenv_s", header: "<stdlib.h>".}
+  proc c_wgetenv(varname: WideCString): WideCString {.importc: "_wgetenv", header: "<stdlib.h>".}
+
+  var errno {.importc, header: "<errno.h>".}: cint
+  type wchar_t  {.importc: "wchar_t".} = int16
+  var gWenviron {.importc:"_wenviron".}: ptr ptr wchar_t
+    # xxx `ptr UncheckedArray[WideCString]` did not work
+
+  proc mbstowcs_s(pReturnValue: ptr csize_t, wcstr: WideCString, sizeInWords: csize_t, mbstr: cstring, count: csize_t): cint {.importc: "mbstowcs_s", header: "<stdlib.h>".}
+    # xxx cint vs errno_t?
+
+  proc setEnvImpl*(name: cstring, value: cstring, overwrite: cint): cint =
+    const EINVAL = cint(22)
+    const MAX_ENV = 32767
+      # xxx get it from: `var MAX_ENV {.importc: "_MAX_ENV", header:"<stdlib.h>".}: cint`
+    if overwrite == 0 and c_getenv(name) != nil: return 0
+    if value[0] != '\0':
+      let e = c_putenv_s(name, value)
+      if e != 0:
+        errno = e
+        return -1
+      return 0
+    #[
+    We are trying to set the value to an empty string, but `_putenv_s` deletes
+    entries if the value is an empty string, and just calling
+    SetEnvironmentVariableA doesn't update `_environ`,
+    so we have to do these terrible things.
+    ]#
+    if c_putenv_s(name, "  ") != 0:
+      errno = EINVAL
+      return -1
+    # Here lies the documentation we blatently ignore to make this work.
+    var s = c_getenv(name)
+    s[0] = '\0'
+    #[
+    This would result in a double null termination, which normally signifies the
+    end of the environment variable list, so we stick a completely empty
+    environment variable into the list instead.
+    ]#
+    s[1] = '='
+    #[
+    If gWenviron is null, the wide environment has not been initialized
+    yet, and we don't need to try to update it. We have to do this otherwise
+    we'd be forcing the initialization and maintenance of the wide environment
+    even though it's never actually used in most programs.
+    ]#
+    if gWenviron != nil:
+      # var buf: array[MAX_ENV + 1, WideCString]
+      var buf: array[MAX_ENV + 1, Utf16Char]
+      let buf2 = cast[WideCString](buf[0].addr)
+      var len: csize_t
+      if mbstowcs_s(len.addr, buf2, buf.len.csize_t, name, MAX_ENV) != 0:
+        errno = EINVAL
+        return -1
+      c_wgetenv(buf2)[0] = '\0'.Utf16Char
+      c_wgetenv(buf2)[1] = '='.Utf16Char
+
+    # And now, we have to update the outer environment to have a proper empty value.
+    if setEnvironmentVariableA(name, value) == 0:
+      errno = EINVAL
+      return -1
+    return 0

tests/destructor/tnewruntime_misc.nim

@@ -7,7 +7,7 @@ axc
 ...
 destroying GenericObj[T] GenericObj[system.int]
 test
-(allocCount: 13, deallocCount: 11)
+(allocCount: 12, deallocCount: 10)
 3'''
 """
 

tests/stdlib/tos.nim

@@ -331,7 +331,7 @@ block walkDirRec:
     doAssert p.startsWith("walkdir_test")
 
   var s: seq[string]
-  for p in walkDirRec("walkdir_test", {pcFile}, {pcDir}, relative=true):
+  for p in walkDirRec("walkdir_test", {pcFile}, {pcDir}, relative = true):
     s.add(p)
 
   doAssert s.len == 2
@@ -553,19 +553,19 @@ block ospaths:
   doAssert joinPath("/", "") == unixToNativePath"/"
   doAssert joinPath("/" / "") == unixToNativePath"/" # weird test case...
   doAssert joinPath("/", "/a/b/c") == unixToNativePath"/a/b/c"
-  doAssert joinPath("foo/","") == unixToNativePath"foo/"
+  doAssert joinPath("foo/", "") == unixToNativePath"foo/"
-  doAssert joinPath("foo/","abc") == unixToNativePath"foo/abc"
+  doAssert joinPath("foo/", "abc") == unixToNativePath"foo/abc"
-  doAssert joinPath("foo//./","abc/.//") == unixToNativePath"foo/abc/"
+  doAssert joinPath("foo//./", "abc/.//") == unixToNativePath"foo/abc/"
-  doAssert joinPath("foo","abc") == unixToNativePath"foo/abc"
+  doAssert joinPath("foo", "abc") == unixToNativePath"foo/abc"
-  doAssert joinPath("","abc") == unixToNativePath"abc"
+  doAssert joinPath("", "abc") == unixToNativePath"abc"
 
-  doAssert joinPath("zook/.","abc") == unixToNativePath"zook/abc"
+  doAssert joinPath("zook/.", "abc") == unixToNativePath"zook/abc"
 
   # controversial: inconsistent with `joinPath("zook/.","abc")`
   # on linux, `./foo` and `foo` are treated a bit differently for executables
   # but not `./foo/bar` and `foo/bar`
   doAssert joinPath(".", "/lib") == unixToNativePath"./lib"
-  doAssert joinPath(".","abc") == unixToNativePath"./abc"
+  doAssert joinPath(".", "abc") == unixToNativePath"./abc"
 
   # cases related to issue #13455
   doAssert joinPath("foo", "", "") == "foo"
@@ -605,24 +605,6 @@ block getTempDir:
 block: # getCacheDir
   doAssert getCacheDir().dirExists
 
-block osenv:
-  block delEnv:
-    const dummyEnvVar = "DUMMY_ENV_VAR" # This env var wouldn't be likely to exist to begin with
-    doAssert existsEnv(dummyEnvVar) == false
-    putEnv(dummyEnvVar, "1")
-    doAssert existsEnv(dummyEnvVar) == true
-    delEnv(dummyEnvVar)
-    doAssert existsEnv(dummyEnvVar) == false
-    delEnv(dummyEnvVar)         # deleting an already deleted env var
-    doAssert existsEnv(dummyEnvVar) == false
-  block: # putEnv, bug #18502
-    doAssertRaises(OSError): putEnv("DUMMY_ENV_VAR_PUT=DUMMY_VALUE", "NEW_DUMMY_VALUE")
-    doAssertRaises(OSError): putEnv("", "NEW_DUMMY_VALUE")
-  block:
-    doAssert getEnv("DUMMY_ENV_VAR_NONEXISTENT", "") == ""
-    doAssert getEnv("DUMMY_ENV_VAR_NONEXISTENT", " ") == " "
-    doAssert getEnv("DUMMY_ENV_VAR_NONEXISTENT", "Arrakis") == "Arrakis"
-
 block isRelativeTo:
   doAssert isRelativeTo("/foo", "/")
   doAssert isRelativeTo("/foo/bar", "/foo")

tests/stdlib/tosenv.nim

@@ -0,0 +1,58 @@
+discard """
+  matrix: "--threads"
+  joinable: false
+  targets: "c js cpp"
+"""
+
+import std/os
+from std/sequtils import toSeq
+import stdtest/testutils
+
+template main =
+  block: # delEnv, existsEnv, getEnv, envPairs
+    for val in ["val", ""]: # ensures empty val works too
+      const key = "NIM_TESTS_TOSENV_KEY"
+      doAssert not existsEnv(key)
+      putEnv(key, val)
+      doAssert existsEnv(key)
+      doAssert getEnv(key) == val
+      when nimvm: discard
+      else:
+        doAssert (key, val) in toSeq(envPairs())
+      delEnv(key)
+      when nimvm: discard
+      else:
+        doAssert (key, val) notin toSeq(envPairs())
+      doAssert not existsEnv(key)
+      delEnv(key) # deleting an already deleted env var
+      doAssert not existsEnv(key)
+
+    block:
+      doAssert getEnv("NIM_TESTS_TOSENV_NONEXISTENT", "") == ""
+      doAssert getEnv("NIM_TESTS_TOSENV_NONEXISTENT", " ") == " "
+      doAssert getEnv("NIM_TESTS_TOSENV_NONEXISTENT", "defval") == "defval"
+
+    whenVMorJs: discard # xxx improve
+    do:
+      doAssertRaises(OSError, putEnv("NIM_TESTS_TOSENV_PUT=DUMMY_VALUE", "NEW_DUMMY_VALUE"))
+      doAssertRaises(OSError, putEnv("", "NEW_DUMMY_VALUE"))
+      doAssert not existsEnv("")
+      doAssert not existsEnv("NIM_TESTS_TOSENV_PUT=DUMMY_VALUE")
+      doAssert not existsEnv("NIM_TESTS_TOSENV_PUT")
+
+static: main()
+main()
+
+when not defined(js):
+  block: # bug #18533
+    proc c_getenv(env: cstring): cstring {.importc: "getenv", header: "<stdlib.h>".}
+    var thr: Thread[void]
+    proc threadFunc {.thread.} = putEnv("foo", "fooVal2")
+
+    putEnv("foo", "fooVal1")
+    doAssert getEnv("foo") == "fooVal1"
+    createThread(thr, threadFunc)
+    joinThreads(thr)
+    doAssert getEnv("foo") == $c_getenv("foo")
+
+    doAssertRaises(OSError): delEnv("foo=bar")