Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

std.process.environment is NOT SAFE #10580

Open
schveiguy opened this issue Dec 10, 2024 · 1 comment · May be fixed by #10611
Open

std.process.environment is NOT SAFE #10580

schveiguy opened this issue Dec 10, 2024 · 1 comment · May be fixed by #10611

Comments

@schveiguy
Copy link
Member

Yet, all the members of the environment class are marked @safe or @trusted.

As an example, setting an environment variable on one thread, and reading it on another thread, could call setenv and getenv respectively. These functions are not thread safe, and getenv can easily return memory that is deallocated. I've even had getenv itself segfault, because it is iterating the environment pointer as it is being realloc'd in another thread.

We should lock around any reading or manipulation of environment. We can't mark these functions as not safe at this point.
@0xEAB
Copy link
Member

0xEAB commented Dec 10, 2024

We are not alone. PHP suffers from this as well. A popular environment file loader library in their ecosystem specifically warns about this problem:

Using getenv() and putenv() is strongly discouraged due to the fact that these functions are not thread safe

ntrel added a commit to ntrel/phobos that referenced this issue Jan 6, 2025
@ntrel
[std.process] Make environment use ReadWriteMutex
ba0de15 
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.
@ntrel ntrel linked a pull request Jan 6, 2025 that will close this issue
Draft
0xEAB pushed a commit to 0xEAB/phobos that referenced this issue Jan 9, 2025
@ntrel @0xEAB
[std.process] Make environment use ReadWriteMutex
71babd4 
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.
0xEAB pushed a commit to ntrel/phobos that referenced this issue Jan 9, 2025
@ntrel @0xEAB
[std.process] Make environment use ReadWriteMutex
811dacc 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[std.process] Make environment use a ReadWriteMutex ntrel/phobos
2 participants
@schveiguy @0xEAB