Skip to content

Commit

Permalink
[std.process] Make environment use ReadWriteMutex
Browse files Browse the repository at this point in the history
Fixes dlang#10580.

Make getEnvironPtr `@system`.
Use a ReadWriteMutex to protect reading and writing to environment.
Add `scope` to `getImpl` callback parameter.

Warning 1: This (currently) removes `nothrow @nogc` from `remove`.
Warning 2: I am not that experienced with locks, so bear that in mind,
I may have done something wrong. Or there may be a better solution,
please let me know.
  • Loading branch information
ntrel authored and 0xEAB committed Jan 9, 2025
1 parent 03aeafd commit 71babd4
Showing 1 changed file with 35 additions and 8 deletions.
43 changes: 35 additions & 8 deletions std/process.d
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ $(LREF environment), $(LREF thisProcessID) and $(LREF thisThreadID).
module std.process;

import core.thread : ThreadID;
import core.sync.rwmutex;

version (Posix)
{
Expand Down Expand Up @@ -160,6 +161,13 @@ private
// Environment variable manipulation.
// =============================================================================

shared ReadWriteMutex mutex;

shared static this()
{
mutex = new shared ReadWriteMutex(mutex.Policy.PREFER_READERS);
}

/**
Manipulates _environment variables using an associative-array-like
interface.
Expand Down Expand Up @@ -259,12 +267,14 @@ static:
import std.exception : enforce, errnoEnforce;
if (value is null)
{
// Note: remove needs write lock
remove(name);
return value;
}
if (core.sys.posix.stdlib.setenv(name.tempCString(), value.tempCString(), 1) != -1)
synchronized (mutex.writer)
{
return value;
if (core.sys.posix.stdlib.setenv(name.tempCString(), value.tempCString(), 1) != -1)
return value;
}
// The default errno error message is very uninformative
// in the most common case, so we handle it manually.
Expand All @@ -277,6 +287,8 @@ static:
else version (Windows)
{
import std.windows.syserror : wenforce;

synchronized (mutex.writer)
wenforce(
SetEnvironmentVariableW(name.tempCStringW(), value.tempCStringW()),
);
Expand All @@ -296,8 +308,9 @@ static:
multi-threaded programs. See e.g.
$(LINK2 https://www.gnu.org/software/libc/manual/html_node/Environment-Access.html#Environment-Access, glibc).
*/
void remove(scope const(char)[] name) @trusted nothrow @nogc
void remove(scope const(char)[] name) @trusted //nothrow @nogc
{
synchronized (mutex.writer)
version (Windows) SetEnvironmentVariableW(name.tempCStringW(), null);
else version (Posix) core.sys.posix.stdlib.unsetenv(name.tempCString());
else static assert(0);
Expand Down Expand Up @@ -329,6 +342,8 @@ static:
{
if (name is null)
return false;

synchronized (mutex.reader)
version (Posix)
return core.sys.posix.stdlib.getenv(name.tempCString()) !is null;
else version (Windows)
Expand Down Expand Up @@ -363,6 +378,8 @@ static:
{
import std.conv : to;
string[string] aa;

synchronized (mutex.reader)
version (Posix)
{
auto environ = getEnvironPtr;
Expand Down Expand Up @@ -428,12 +445,13 @@ private:
// Retrieves the environment variable. Calls `sink` with a
// temporary buffer of OS characters, or `null` if the variable
// doesn't exist.
void getImpl(scope const(char)[] name, scope void delegate(const(OSChar)[]) @safe sink) @trusted
void getImpl(scope const(char)[] name, scope void delegate(scope const(OSChar)[]) @safe sink) @trusted
{
// fix issue https://issues.dlang.org/show_bug.cgi?id=24549
if (name is null)
return sink(null);

synchronized (mutex.reader)
version (Windows)
{
// first we ask windows how long the environment variable is,
Expand Down Expand Up @@ -1205,7 +1223,7 @@ private Pid spawnProcessPosix(scope const(char[])[] args,
}

// Execute program.
core.sys.posix.unistd.execve(argz[0], argz.ptr, envz is null ? getEnvironPtr : envz);
core.sys.posix.unistd.execve(argz[0], argz.ptr, envz);

// If execution fails, exit as quickly as possible.
abortOnError(forkPipeOut, InternalError.exec, .errno);
Expand Down Expand Up @@ -1511,7 +1529,7 @@ private Pid spawnProcessWin(scope const(char)[] commandLine,
// on the form "name=value", optionally adding those of the current process'
// environment strings that are not present in childEnv. If the parent's
// environment should be inherited without modification, this function
// returns null.
// returns environ directly.
version (Posix)
private const(char*)* createEnv(const string[string] childEnv,
bool mergeWithParentEnv)
Expand All @@ -1521,7 +1539,7 @@ private const(char*)* createEnv(const string[string] childEnv,
auto environ = getEnvironPtr;
if (mergeWithParentEnv)
{
if (childEnv.length == 0) return null;
if (childEnv.length == 0) return environ;
while (environ[parentEnvLength] != null) ++parentEnvLength;
}

Expand Down Expand Up @@ -1551,7 +1569,16 @@ version (Posix) @system unittest
assert(e1 != null && *e1 == null);

auto e2 = createEnv(null, true);
assert(e2 == null);
assert(e2 != null);
int i = 0;
auto environ = getEnvironPtr;
for (; environ[i] != null; ++i)
{
assert(e2[i] != null);
import core.stdc.string : strcmp;
assert(strcmp(e2[i], environ[i]) == 0);
}
assert(e2[i] == null);

auto e3 = createEnv(["foo" : "bar", "hello" : "world"], false);
assert(e3 != null && e3[0] != null && e3[1] != null && e3[2] == null);
Expand Down

0 comments on commit 71babd4

Please sign in to comment.