`getEnv` + friends gives SIGSEGV or wrong results; osenv should not try to keep track of `environment`

[timotheecour]

Example 1

import os

proc c_getenv(env: cstring): cstring {.importc: "getenv", header: "<stdlib.h>".}
var thr: Thread[void]
proc threadFunc {.thread.} =
  putEnv("foo", "fooVal2")
proc main()=
  putEnv("foo", "fooVal1")
  echo getEnv("foo")
  createThread(thr, threadFunc)
  joinThreads(thr)
  echo getEnv("foo")
  echo $c_getenv("foo")
main()

Current Output

before #18530:

fooVal1
fooVal1
Traceback (most recent call last)
/Users/timothee/git_clone/nim/timn/tests/nim/all/t12606.nim(18) t12606
/Users/timothee/git_clone/nim/timn/tests/nim/all/t12606.nim(17) main
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

after #18530: it improves, but still wrong (/cc @brightly-salty):
fooVal1
fooVal1
fooVal2

Expected Output

fooVal1
fooVal2
fooVal2

Example 2

delEnv has similar bug (since it also uses environment)

this should raise an OSError, but doesn't, because the call is intercepted by 1st checking environment

import std/os
delEnv("foo=bar")

Possible Solution

osenv should not try to keep track of environment; it's not thread-safe, and not safe against other modifications of environment variables (eg via importc procs etc)

Additional Information

1.5.1 25efb53

[brightly-salty]

I can take a look at this since its similar in the files to be changed as #18530

[timotheecour]

yes please! you can start a separate PR on top of the 1st one without waiting for 1st one to be merged

[brightly-salty]

According to internal docs for lib/pure/includes/osenv.nim, I see that

Environment handling cannot be put into RTL, because the envPairs
iterator depends on environment.

It looks like if we factor out tracking of environment from osenv, envPairs will not work because we do not have a record of that. Is this grounds for the closing of this issue or should I remove os.envPairs entirely?

The source of envPairs is

    getEnvVarsC()
    for i in 0..high(environment):
      var p = find(environment[i], '=')
      yield (substr(environment[i], 0, p-1),
             substr(environment[i], p+1))

It might also be possible to inline getEnvVarsC so that environment is local to envPairs, if that's what is wanted.

[timotheecour]

envPairs shouldn't require environment either, eg, smthg like this should work:

iterator envPairs*(): tuple[key, value: string] {.tags: [ReadEnvEffect].} =
  when useNSGetEnviron:
    var gEnv = NSGetEnviron()[]
   # else: gEnv is already declared
   while true:
     let ei = gEnv[i]
     if kv == nil: break
     let kv2 = $kv
     var p = find(kv2, '=')
     yield (substr(kv2, 0, p-1), substr(kv2, p+1))

[timotheecour]

fixed by #18575 and the tests in the PR exercise the 2 bugs