From 266f15b672633226edc4196ba7a94dd23385f5b2 Mon Sep 17 00:00:00 2001 From: Feoramund <161657516+Feoramund@users.noreply.github.com> Date: Tue, 4 Mar 2025 19:11:32 -0500 Subject: [PATCH 1/3] Fix indentation --- core/os/os2/env_posix.odin | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/os/os2/env_posix.odin b/core/os/os2/env_posix.odin index 3db8d817a..9661768b4 100644 --- a/core/os/os2/env_posix.odin +++ b/core/os/os2/env_posix.odin @@ -57,7 +57,7 @@ _clear_env :: proc() { } _environ :: proc(allocator: runtime.Allocator) -> (environ: []string, err: Error) { - n := 0 + n := 0 for entry := posix.environ[0]; entry != nil; n, entry = n+1, posix.environ[n] {} r := make([dynamic]string, 0, n, allocator) or_return From 179e5b9266a35abaa859492bda7106e177f1afa4 Mon Sep 17 00:00:00 2001 From: Feoramund <161657516+Feoramund@users.noreply.github.com> Date: Tue, 4 Mar 2025 19:12:28 -0500 Subject: [PATCH 2/3] Fix typo --- core/os/os2/env_linux.odin | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/os/os2/env_linux.odin b/core/os/os2/env_linux.odin index f24bd8d84..332bf4dc0 100644 --- a/core/os/os2/env_linux.odin +++ b/core/os/os2/env_linux.odin @@ -126,7 +126,7 @@ _unset_env :: proc(key: string) -> bool { return true } - // if we got this far, the envrionment variable + // if we got this far, the environment variable // existed AND was allocated by us. k_addr, _ := _kv_addr_from_val(v, key) runtime.heap_free(k_addr) From 2ab1ca29e6c9f07c4fe3c8cd36be07431431de54 Mon Sep 17 00:00:00 2001 From: Feoramund <161657516+Feoramund@users.noreply.github.com> Date: Tue, 4 Mar 2025 19:12:02 -0500 Subject: [PATCH 3/3] Fix data races in `os2/env_linux.odin` Switched to a recursive mutex so that procedures which need to perform lookups can do so while also maintaining the lock across their entire body in order to guarantee atomicity for each environment operation. --- core/os/os2/env_linux.odin | 48 ++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/core/os/os2/env_linux.odin b/core/os/os2/env_linux.odin index 332bf4dc0..2ed43dd91 100644 --- a/core/os/os2/env_linux.odin +++ b/core/os/os2/env_linux.odin @@ -20,19 +20,18 @@ NOT_FOUND :: -1 // the environment is a 0 delimited list of = strings _env: [dynamic]string -_env_mutex: sync.Mutex +_env_mutex: sync.Recursive_Mutex // We need to be able to figure out if the environment variable // is contained in the original environment or not. This also // serves as a flag to determine if we have built _env. -_org_env_begin: uintptr -_org_env_end: uintptr +_org_env_begin: uintptr // atomic +_org_env_end: uintptr // guarded by _env_mutex // Returns value + index location into _env // or -1 if not found _lookup :: proc(key: string) -> (value: string, idx: int) { - sync.mutex_lock(&_env_mutex) - defer sync.mutex_unlock(&_env_mutex) + sync.guard(&_env_mutex) for entry, i in _env { if k, v := _kv_from_entry(entry); k == key { @@ -43,7 +42,7 @@ _lookup :: proc(key: string) -> (value: string, idx: int) { } _lookup_env :: proc(key: string, allocator: runtime.Allocator) -> (value: string, found: bool) { - if _org_env_begin == 0 { + if intrinsics.atomic_load_explicit(&_org_env_begin, .Acquire) == 0 { _build_env() } @@ -55,9 +54,10 @@ _lookup_env :: proc(key: string, allocator: runtime.Allocator) -> (value: string } _set_env :: proc(key, v_new: string) -> Error { - if _org_env_begin == 0 { + if intrinsics.atomic_load_explicit(&_org_env_begin, .Acquire) == 0 { _build_env() } + sync.guard(&_env_mutex) // all key values are stored as "key=value\x00" kv_size := len(key) + len(v_new) + 2 @@ -65,8 +65,6 @@ _set_env :: proc(key, v_new: string) -> Error { if v_curr == v_new { return nil } - sync.mutex_lock(&_env_mutex) - defer sync.mutex_unlock(&_env_mutex) unordered_remove(&_env, idx) @@ -101,16 +99,15 @@ _set_env :: proc(key, v_new: string) -> Error { intrinsics.mem_copy_non_overlapping(&val_slice[0], raw_data(v_new), len(v_new)) val_slice[len(v_new)] = 0 - sync.mutex_lock(&_env_mutex) append(&_env, string(k_addr[:kv_size - 1])) - sync.mutex_unlock(&_env_mutex) return nil } _unset_env :: proc(key: string) -> bool { - if _org_env_begin == 0 { + if intrinsics.atomic_load_explicit(&_org_env_begin, .Acquire) == 0 { _build_env() } + sync.guard(&_env_mutex) v: string i: int @@ -118,9 +115,7 @@ _unset_env :: proc(key: string) -> bool { return false } - sync.mutex_lock(&_env_mutex) unordered_remove(&_env, i) - sync.mutex_unlock(&_env_mutex) if _is_in_org_env(v) { return true @@ -134,8 +129,7 @@ _unset_env :: proc(key: string) -> bool { } _clear_env :: proc() { - sync.mutex_lock(&_env_mutex) - defer sync.mutex_unlock(&_env_mutex) + sync.guard(&_env_mutex) for kv in _env { if !_is_in_org_env(kv) { @@ -145,14 +139,16 @@ _clear_env :: proc() { clear(&_env) // nothing resides in the original environment either - _org_env_begin = ~uintptr(0) + intrinsics.atomic_store_explicit(&_org_env_begin, ~uintptr(0), .Release) _org_env_end = ~uintptr(0) } _environ :: proc(allocator: runtime.Allocator) -> (environ: []string, err: Error) { - if _org_env_begin == 0 { + if intrinsics.atomic_load_explicit(&_org_env_begin, .Acquire) == 0 { _build_env() } + sync.guard(&_env_mutex) + env := make([dynamic]string, 0, len(_env), allocator) or_return defer if err != nil { for e in env { @@ -161,8 +157,6 @@ _environ :: proc(allocator: runtime.Allocator) -> (environ: []string, err: Error delete(env) } - sync.mutex_lock(&_env_mutex) - defer sync.mutex_unlock(&_env_mutex) for entry in _env { s := clone_string(entry, allocator) or_return append(&env, s) @@ -174,7 +168,7 @@ _environ :: proc(allocator: runtime.Allocator) -> (environ: []string, err: Error // The entire environment is stored as 0 terminated strings, // so there is no need to clone/free individual variables export_cstring_environment :: proc(allocator: runtime.Allocator) -> []cstring { - if _org_env_begin == 0 { + if intrinsics.atomic_load_explicit(&_org_env_begin, .Acquire) == 0 { // The environment has not been modified, so we can just // send the original environment org_env := _get_original_env() @@ -182,12 +176,11 @@ export_cstring_environment :: proc(allocator: runtime.Allocator) -> []cstring { for ; org_env[n] != nil; n += 1 {} return slice.clone(org_env[:n + 1], allocator) } + sync.guard(&_env_mutex) // NOTE: already terminated by nil pointer via + 1 env := make([]cstring, len(_env) + 1, allocator) - sync.mutex_lock(&_env_mutex) - defer sync.mutex_unlock(&_env_mutex) for entry, i in _env { env[i] = cstring(raw_data(entry)) } @@ -195,15 +188,14 @@ export_cstring_environment :: proc(allocator: runtime.Allocator) -> []cstring { } _build_env :: proc() { - sync.mutex_lock(&_env_mutex) - defer sync.mutex_unlock(&_env_mutex) - if _org_env_begin != 0 { + sync.guard(&_env_mutex) + if intrinsics.atomic_load_explicit(&_org_env_begin, .Acquire) != 0 { return } _env = make(type_of(_env), runtime.heap_allocator()) cstring_env := _get_original_env() - _org_env_begin = uintptr(rawptr(cstring_env[0])) + intrinsics.atomic_store_explicit(&_org_env_begin, uintptr(rawptr(cstring_env[0])), .Release) for i := 0; cstring_env[i] != nil; i += 1 { bytes := ([^]u8)(cstring_env[i]) n := len(cstring_env[i]) @@ -235,5 +227,5 @@ _kv_addr_from_val :: #force_inline proc(val: string, key: string) -> ([^]u8, [^] _is_in_org_env :: #force_inline proc(env_data: string) -> bool { addr := uintptr(raw_data(env_data)) - return addr >= _org_env_begin && addr < _org_env_end + return addr >= intrinsics.atomic_load_explicit(&_org_env_begin, .Acquire) && addr < _org_env_end }