Improve thread safety of ENV on UNIX#16448
Improve thread safety of ENV on UNIX#16448ysbaddaden wants to merge 4 commits intocrystal-lang:masterfrom
ENV on UNIX#16448Conversation
Reduces the thread safety issues of ENV usages in crystal code. It doesn't fix the thread safety issue of the libc getenv, setenv and unsetenv implementations. It is entirely possible for external libraries, including libc calls made by the stdlib itself, to call getenv directly while another thread in Crystal calls setenv while holding the write lock.
|
CI is broken 😞 In fact Environ is parsed, Why? Because we require
|
14cdf76 to
ae5e178
Compare
|
I modified We'll only need the internal collection if we decide to have |
| # Skip overrides in `env` | ||
| next if env.has_key?(key) | ||
| # Dup LibC.environ, skipping overrides in env. | ||
| each do |key, value| |
There was a problem hiding this comment.
This is expensive and since we can't return environ anymore, this happens all the time.
I have a simple patch that yields slices to the kv pointer instead of strings, which avoids the intermediary string allocations and directly builds the "key=value" string. I'll push it in a follow up.
The
environlibc pointer is thread unsafe. Thegetenv,setenvandunsetenvlibc calls are also thread unsafe. At best they libc implementations will leak memory and at worst will segfault on a meregetenvcall.I propose to parseSee comments for details.environonce at program startup into an internal hash. This happens before we start threads that may mutateenvironin parallel and is safe; thenenvironis never accessed again (andgetenvnever called).I added a readers-writer lock to protect the internal hash and libc function calls. Any calls to read and mutate the environment should be safe...
...but it's still thread unsafe in practice because crystal code might get the writer lock and call
setenvwhile another thread executes an external lib function (including libc calls made by crystal stdlib) that internally callsgetenv💣For this reason, the documentation has been updated to insist that
ENVshould be considered an immutable global collection.