> From: Lassi Kortela <xxxxxx@lassi.io>
> Date: Sunday, December 08, 2019 1:36 PM

>>     Would you recommend we drop `set-environment-variable!` and
>>     `delete-environment-variable!` and simply provide an :environment
>>     argument in the subprocess SRFI? That may be the best call.
>>
>> [ John: ] Yes, exactly.

> Harold, what's your take on this? I'm fine with going either way.

Excellent timing, I'd just finished drafting a detailed reply:

> From: Lassi Kortela <xxxxxx@lassi.io>
> Date: Saturday, December 07, 2019 2:12 PM
>
>> 1) They inherently and unavoidably leak memory, even though setenv()
>> copies its arguments before putting them in the environment.  The
>> problem is created by the existence of putenv(), which is required by
>> both ISO C and Posix.  Putenv() deliberately does *not* copy its
>> argument; rather, the argument becomes part of the environment data
>> structure.  This means putenv() by itself is deterministic: if you
>> unsetenv() the variable, or if you are about to putenv() it again, the
>> memory used by the previous putenv can be free()d, or can even be a
>> stack variable if you are careful.
>>
>> But having both putenv() and setenv() means that setenv() and unsetenv()
>> *cannot free* the entry they are about to remove, because it may have
>> been inserted by putenv.  So if you are using setenv() freely, you have
>> a permanent (though usually small) memory leak.
>
> I've never thought of that.

I didn't even know about putenv, not at all a System V fan, but it
appears that's why its in POSIX, see this comment for POSIX setenv:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/setenv.html

> There was considerable debate as to whether the System V putenv()
> function or the BSD setenv() function should be required as a
> mandatory function. The setenv() function was chosen because it
> permitted the implementation of the unsetenv() function to delete
> environmental variables, without specifying an additional
> interface. The putenv() function is available as part of the XSI
> option.

It looks very sketchy, see this from POSIX, and there's lots more:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/putenv.html

> The putenv() function need not be thread-safe.

It doesn't even have EINVAL defined as a possible error in case you
don't pass it a string including '=' (it takes "name=value"), only
ENOMEM if you run out of memory.

Back to Lassi:

> OpenBSD libc (which is known for its attention to correctness) never
> frees any strings that are part of the environment. None of setenv(),
> putenv() and unsetenv() call free() on any string they remove from the
> environ array. environ itself it reallocarray()'d as needed.
>
> <https://github.com/openbsd/src/blob/master/lib/libc/stdlib/setenv.c>

I just checked glibc, and while the comments express concern about
running out of memory---it was started in 1987, and it's conceivable
someone might want to use it in larger memory embedded environments,
it has a feature to reuse existing strings---it too never frees, and
reallocs as needed.  See e.g. this from an unofficial repo:
https://github.com/bminor/glibc/blob/master/stdlib/setenv.c

Today this memory leaking probably isn't very important, outside of
embedded systems.

> Not sure how much the memory leak matters. If an implementation wanted
> to go to a ton of effort, it could keep a list of pointers to "putenv()
> strings managed by Scheme" and the delete-environment-variable! could
> check whether it's one of those.

That's assuming nothing called in the meanwhile made a copy of the
pointer to the putenv'ed string; again from the POSIX entry on putenv:

> Although the space used by string is no longer used once a new
> string which defines name is passed to putenv(), if any thread in
> the application has used getenv() to retrieve a pointer to this
> variable, it should not be freed by calling free(). If the changed
> environment variable is one known by the system (such as the locale
> environment variables) the application should never free the buffer
> used by earlier calls to putenv() for the same variable.

>> 2) The only reasons to modify the environment rather than changing a
>> variable initialized from the environment are these:
>>
>> 2a) to affect the behavior of some other part of the process, typically
>> some library (as TZ affects the time zone library).  The trouble here is
>> that most such libraries do *not* expect these variables to change and
>> don't check whether they should mutate their internal view, so changing
>> the environment is likely to be ineffectual.  (Fortunately, the Olson
>> time zone library provides a way to change the timezone directly without
>> changing the environment.)
>>
>> 2b) to pass a modified environment to a subprocess.  But we can already
>> get the whole environment as an alist, and the ProcessesCowan pre-SRFI
>> accepts an alist and synthesizes a replacement environment from it.
>> Leaks don't matter here because memory will be discarded by the execve()
>> call,
>
> Maybe. setenv() tends to be a "set and forget" thing done at the start
> of the program. It also potentially causes problems in multi-threaded
> implementations. The text I wrote for the SRFI has some language to warn
> about that but it's not ideal to lock the environment array separately
> for each read and write operation.

glibc locks the array; per this, which setenv refers to, it's required:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html

> Conforming multi-threaded applications shall not use the environ
> variable to access or modify any environment variable while any
> other thread is concurrently modifying any environment variable. A
> call to any function dependent on any environment variable shall be
> considered a use of the environ variable to access that environment
> variable.

All that said, except for special cases like development and debugging,
I've never had a need to set environment variables outside of process
edge conditions, which the process SRFI will take care of.  If no one
can come up with a library where you really need to set or reset an
environment variable, as John implicitly notes good time libraries
don't depend on TZ, not wrapping both setenv and unsetenv as John
suggested sounds best.

- Harold