From 55ca27faa78434d81d3f59535c96bbb2a08f0d5c Mon Sep 17 00:00:00 2001 From: The8472 Date: Sat, 6 Feb 2021 22:38:16 +0100 Subject: [PATCH 1/5] use rwlock for accessing ENV --- library/std/src/sys/unix/os.rs | 16 +++---- .../std/src/sys/unix/process/process_unix.rs | 6 +-- library/std/src/sys_common/rwlock.rs | 44 +++++++++++++++++++ 3 files changed, 55 insertions(+), 11 deletions(-) diff --git a/library/std/src/sys/unix/os.rs b/library/std/src/sys/unix/os.rs index d5e14bec76572..33921180cb17c 100644 --- a/library/std/src/sys/unix/os.rs +++ b/library/std/src/sys/unix/os.rs @@ -22,6 +22,7 @@ use crate::str; use crate::sys::cvt; use crate::sys::fd; use crate::sys_common::mutex::{StaticMutex, StaticMutexGuard}; +use crate::sys_common::rwlock::{RWLock, RWLockGuard}; use crate::vec; use libc::{c_char, c_int, c_void}; @@ -493,17 +494,16 @@ pub unsafe fn environ() -> *mut *const *const c_char { &mut environ } -pub unsafe fn env_lock() -> StaticMutexGuard { - // It is UB to attempt to acquire this mutex reentrantly! - static ENV_LOCK: StaticMutex = StaticMutex::new(); - ENV_LOCK.lock() +pub unsafe fn env_rwlock(readonly: bool) -> RWLockGuard { + static ENV_LOCK: RWLock = RWLock::new(); + if readonly { ENV_LOCK.read_with_guard() } else { ENV_LOCK.write_with_guard() } } /// Returns a vector of (variable, value) byte-vector pairs for all the /// environment variables of the current process. pub fn env() -> Env { unsafe { - let _guard = env_lock(); + let _guard = env_rwlock(true); let mut environ = *environ(); let mut result = Vec::new(); if !environ.is_null() { @@ -540,7 +540,7 @@ pub fn getenv(k: &OsStr) -> io::Result> { // always None as well let k = CString::new(k.as_bytes())?; unsafe { - let _guard = env_lock(); + let _guard = env_rwlock(true); let s = libc::getenv(k.as_ptr()) as *const libc::c_char; let ret = if s.is_null() { None @@ -556,7 +556,7 @@ pub fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> { let v = CString::new(v.as_bytes())?; unsafe { - let _guard = env_lock(); + let _guard = env_rwlock(false); cvt(libc::setenv(k.as_ptr(), v.as_ptr(), 1)).map(drop) } } @@ -565,7 +565,7 @@ pub fn unsetenv(n: &OsStr) -> io::Result<()> { let nbuf = CString::new(n.as_bytes())?; unsafe { - let _guard = env_lock(); + let _guard = env_rwlock(false); cvt(libc::unsetenv(nbuf.as_ptr())).map(drop) } } diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index 2746f87468dca..0e4e66389fb3b 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -47,7 +47,7 @@ impl Command { // a lock any more because the parent won't do anything and the child is // in its own process. let result = unsafe { - let _env_lock = sys::os::env_lock(); + let _env_lock = sys::os::env_rwlock(true); cvt(libc::fork())? }; @@ -124,7 +124,7 @@ impl Command { // Similar to when forking, we want to ensure that access to // the environment is synchronized, so make sure to grab the // environment lock before we try to exec. - let _lock = sys::os::env_lock(); + let _lock = sys::os::env_rwlock(true); let Err(e) = self.do_exec(theirs, envp.as_ref()); e @@ -404,7 +404,7 @@ impl Command { cvt_nz(libc::posix_spawnattr_setflags(attrs.0.as_mut_ptr(), flags as _))?; // Make sure we synchronize access to the global `environ` resource - let _env_lock = sys::os::env_lock(); + let _env_lock = sys::os::env_rwlock(true); let envp = envp.map(|c| c.as_ptr()).unwrap_or_else(|| *sys::os::environ() as *const _); cvt_nz(libc::posix_spawnp( &mut p.pid, diff --git a/library/std/src/sys_common/rwlock.rs b/library/std/src/sys_common/rwlock.rs index 3705d641a1be6..1f2765ffbcca6 100644 --- a/library/std/src/sys_common/rwlock.rs +++ b/library/std/src/sys_common/rwlock.rs @@ -1,5 +1,23 @@ use crate::sys::rwlock as imp; +enum GuardType { + Read, + Write, +} + +pub struct RWLockGuard(&'static RWLock, GuardType); + +impl Drop for RWLockGuard { + fn drop(&mut self) { + unsafe { + match &self.1 { + GuardType::Read => self.0.read_unlock(), + GuardType::Write => self.0.write_unlock(), + } + } + } +} + /// An OS-based reader-writer lock. /// /// This structure is entirely unsafe and serves as the lowest layer of a @@ -26,6 +44,19 @@ impl RWLock { self.0.read() } + /// Acquires shared access to the underlying lock, blocking the current + /// thread to do so. + /// + /// The lock is automatically unlocked when the returned guard is dropped. + /// + /// Behavior is undefined if the rwlock has been moved between this and any + /// previous method call. + #[inline] + pub unsafe fn read_with_guard(&'static self) -> RWLockGuard { + self.read(); + RWLockGuard(&self, GuardType::Read) + } + /// Attempts to acquire shared access to this lock, returning whether it /// succeeded or not. /// @@ -48,6 +79,19 @@ impl RWLock { self.0.write() } + /// Acquires write access to the underlying lock, blocking the current thread + /// to do so. + /// + /// The lock is automatically unlocked when the returned guard is dropped. + /// + /// Behavior is undefined if the rwlock has been moved between this and any + /// previous method call. + #[inline] + pub unsafe fn write_with_guard(&'static self) -> RWLockGuard { + self.write(); + RWLockGuard(&self, GuardType::Write) + } + /// Attempts to acquire exclusive access to this lock, returning whether it /// succeeded or not. /// From 406fd3a2772e62ff1b466e59de45d5caa4cfd975 Mon Sep 17 00:00:00 2001 From: The8472 Date: Sun, 7 Feb 2021 09:45:49 +0100 Subject: [PATCH 2/5] silence dead code warnings on windows --- library/std/src/sys_common/rwlock.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/library/std/src/sys_common/rwlock.rs b/library/std/src/sys_common/rwlock.rs index 1f2765ffbcca6..7fd902ee0fe2e 100644 --- a/library/std/src/sys_common/rwlock.rs +++ b/library/std/src/sys_common/rwlock.rs @@ -1,12 +1,15 @@ use crate::sys::rwlock as imp; +#[cfg(unix)] enum GuardType { Read, Write, } +#[cfg(unix)] pub struct RWLockGuard(&'static RWLock, GuardType); +#[cfg(unix)] impl Drop for RWLockGuard { fn drop(&mut self) { unsafe { @@ -52,6 +55,7 @@ impl RWLock { /// Behavior is undefined if the rwlock has been moved between this and any /// previous method call. #[inline] + #[cfg(unix)] pub unsafe fn read_with_guard(&'static self) -> RWLockGuard { self.read(); RWLockGuard(&self, GuardType::Read) @@ -87,6 +91,7 @@ impl RWLock { /// Behavior is undefined if the rwlock has been moved between this and any /// previous method call. #[inline] + #[cfg(unix)] pub unsafe fn write_with_guard(&'static self) -> RWLockGuard { self.write(); RWLockGuard(&self, GuardType::Write) From 2200cf10d8051f535f726f2800b935a527696de8 Mon Sep 17 00:00:00 2001 From: The8472 Date: Mon, 8 Feb 2021 23:31:49 +0100 Subject: [PATCH 3/5] avoid &mut on the read path since it now allows concurrent readers --- library/std/src/sys/unix/os.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sys/unix/os.rs b/library/std/src/sys/unix/os.rs index 33921180cb17c..4f42ce8eafcbc 100644 --- a/library/std/src/sys/unix/os.rs +++ b/library/std/src/sys/unix/os.rs @@ -491,7 +491,7 @@ pub unsafe fn environ() -> *mut *const *const c_char { extern "C" { static mut environ: *const *const c_char; } - &mut environ + ptr::addr_of_mut!(environ) } pub unsafe fn env_rwlock(readonly: bool) -> RWLockGuard { From 44abad5b12afa58b9f495593f1c8b090e644fd7e Mon Sep 17 00:00:00 2001 From: The8472 Date: Mon, 8 Feb 2021 23:34:23 +0100 Subject: [PATCH 4/5] introduce StaticRWLock wrapper to make methods safe --- library/std/src/sys/unix/os.rs | 17 +-- .../std/src/sys/unix/process/process_unix.rs | 6 +- library/std/src/sys_common/rwlock.rs | 109 ++++++++++-------- 3 files changed, 72 insertions(+), 60 deletions(-) diff --git a/library/std/src/sys/unix/os.rs b/library/std/src/sys/unix/os.rs index 4f42ce8eafcbc..f5a607561c0f7 100644 --- a/library/std/src/sys/unix/os.rs +++ b/library/std/src/sys/unix/os.rs @@ -22,7 +22,7 @@ use crate::str; use crate::sys::cvt; use crate::sys::fd; use crate::sys_common::mutex::{StaticMutex, StaticMutexGuard}; -use crate::sys_common::rwlock::{RWLock, RWLockGuard}; +use crate::sys_common::rwlock::{RWLockGuard, StaticRWLock}; use crate::vec; use libc::{c_char, c_int, c_void}; @@ -494,16 +494,17 @@ pub unsafe fn environ() -> *mut *const *const c_char { ptr::addr_of_mut!(environ) } -pub unsafe fn env_rwlock(readonly: bool) -> RWLockGuard { - static ENV_LOCK: RWLock = RWLock::new(); - if readonly { ENV_LOCK.read_with_guard() } else { ENV_LOCK.write_with_guard() } +static ENV_LOCK: StaticRWLock = StaticRWLock::new(); + +pub fn env_read_lock() -> RWLockGuard { + ENV_LOCK.read_with_guard() } /// Returns a vector of (variable, value) byte-vector pairs for all the /// environment variables of the current process. pub fn env() -> Env { unsafe { - let _guard = env_rwlock(true); + let _guard = env_read_lock(); let mut environ = *environ(); let mut result = Vec::new(); if !environ.is_null() { @@ -540,7 +541,7 @@ pub fn getenv(k: &OsStr) -> io::Result> { // always None as well let k = CString::new(k.as_bytes())?; unsafe { - let _guard = env_rwlock(true); + let _guard = env_read_lock(); let s = libc::getenv(k.as_ptr()) as *const libc::c_char; let ret = if s.is_null() { None @@ -556,7 +557,7 @@ pub fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> { let v = CString::new(v.as_bytes())?; unsafe { - let _guard = env_rwlock(false); + let _guard = ENV_LOCK.write_with_guard(); cvt(libc::setenv(k.as_ptr(), v.as_ptr(), 1)).map(drop) } } @@ -565,7 +566,7 @@ pub fn unsetenv(n: &OsStr) -> io::Result<()> { let nbuf = CString::new(n.as_bytes())?; unsafe { - let _guard = env_rwlock(false); + let _guard = ENV_LOCK.write_with_guard(); cvt(libc::unsetenv(nbuf.as_ptr())).map(drop) } } diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index 0e4e66389fb3b..9e82df7755e89 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -47,7 +47,7 @@ impl Command { // a lock any more because the parent won't do anything and the child is // in its own process. let result = unsafe { - let _env_lock = sys::os::env_rwlock(true); + let _env_lock = sys::os::env_read_lock(); cvt(libc::fork())? }; @@ -124,7 +124,7 @@ impl Command { // Similar to when forking, we want to ensure that access to // the environment is synchronized, so make sure to grab the // environment lock before we try to exec. - let _lock = sys::os::env_rwlock(true); + let _lock = sys::os::env_read_lock(); let Err(e) = self.do_exec(theirs, envp.as_ref()); e @@ -404,7 +404,7 @@ impl Command { cvt_nz(libc::posix_spawnattr_setflags(attrs.0.as_mut_ptr(), flags as _))?; // Make sure we synchronize access to the global `environ` resource - let _env_lock = sys::os::env_rwlock(true); + let _env_lock = sys::os::env_read_lock(); let envp = envp.map(|c| c.as_ptr()).unwrap_or_else(|| *sys::os::environ() as *const _); cvt_nz(libc::posix_spawnp( &mut p.pid, diff --git a/library/std/src/sys_common/rwlock.rs b/library/std/src/sys_common/rwlock.rs index 7fd902ee0fe2e..cc13771009fab 100644 --- a/library/std/src/sys_common/rwlock.rs +++ b/library/std/src/sys_common/rwlock.rs @@ -1,26 +1,5 @@ use crate::sys::rwlock as imp; -#[cfg(unix)] -enum GuardType { - Read, - Write, -} - -#[cfg(unix)] -pub struct RWLockGuard(&'static RWLock, GuardType); - -#[cfg(unix)] -impl Drop for RWLockGuard { - fn drop(&mut self) { - unsafe { - match &self.1 { - GuardType::Read => self.0.read_unlock(), - GuardType::Write => self.0.write_unlock(), - } - } - } -} - /// An OS-based reader-writer lock. /// /// This structure is entirely unsafe and serves as the lowest layer of a @@ -47,20 +26,6 @@ impl RWLock { self.0.read() } - /// Acquires shared access to the underlying lock, blocking the current - /// thread to do so. - /// - /// The lock is automatically unlocked when the returned guard is dropped. - /// - /// Behavior is undefined if the rwlock has been moved between this and any - /// previous method call. - #[inline] - #[cfg(unix)] - pub unsafe fn read_with_guard(&'static self) -> RWLockGuard { - self.read(); - RWLockGuard(&self, GuardType::Read) - } - /// Attempts to acquire shared access to this lock, returning whether it /// succeeded or not. /// @@ -83,20 +48,6 @@ impl RWLock { self.0.write() } - /// Acquires write access to the underlying lock, blocking the current thread - /// to do so. - /// - /// The lock is automatically unlocked when the returned guard is dropped. - /// - /// Behavior is undefined if the rwlock has been moved between this and any - /// previous method call. - #[inline] - #[cfg(unix)] - pub unsafe fn write_with_guard(&'static self) -> RWLockGuard { - self.write(); - RWLockGuard(&self, GuardType::Write) - } - /// Attempts to acquire exclusive access to this lock, returning whether it /// succeeded or not. /// @@ -135,3 +86,63 @@ impl RWLock { self.0.destroy() } } + +// the cfg annotations only exist due to dead code warnings. the code itself is portable +#[cfg(unix)] +pub struct StaticRWLock(RWLock); + +#[cfg(unix)] +impl StaticRWLock { + pub const fn new() -> StaticRWLock { + StaticRWLock(RWLock::new()) + } + + /// Acquires shared access to the underlying lock, blocking the current + /// thread to do so. + /// + /// The lock is automatically unlocked when the returned guard is dropped. + #[inline] + pub fn read_with_guard(&'static self) -> RWLockGuard { + // Safety: All methods require static references, therefore self + // cannot be moved between invocations. + unsafe { + self.0.read(); + } + RWLockGuard(&self.0, GuardType::Read) + } + + /// Acquires write access to the underlying lock, blocking the current thread + /// to do so. + /// + /// The lock is automatically unlocked when the returned guard is dropped. + #[inline] + pub fn write_with_guard(&'static self) -> RWLockGuard { + // Safety: All methods require static references, therefore self + // cannot be moved between invocations. + unsafe { + self.0.write(); + } + RWLockGuard(&self.0, GuardType::Write) + } +} + +#[cfg(unix)] +enum GuardType { + Read, + Write, +} + +#[cfg(unix)] +pub struct RWLockGuard(&'static RWLock, GuardType); + +#[cfg(unix)] +impl Drop for RWLockGuard { + fn drop(&mut self) { + unsafe { + match &self.1 { + GuardType::Read => self.0.read_unlock(), + GuardType::Write => self.0.write_unlock(), + } + } + } +} From 4fc181dd62f38c7b424e5261756a2f01ded68a5b Mon Sep 17 00:00:00 2001 From: The8472 Date: Tue, 9 Feb 2021 18:49:29 +0100 Subject: [PATCH 5/5] split guard into read and write types --- library/std/src/sys/unix/os.rs | 4 ++-- library/std/src/sys_common/rwlock.rs | 29 ++++++++++++++-------------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/library/std/src/sys/unix/os.rs b/library/std/src/sys/unix/os.rs index f5a607561c0f7..1d1118aa69434 100644 --- a/library/std/src/sys/unix/os.rs +++ b/library/std/src/sys/unix/os.rs @@ -22,7 +22,7 @@ use crate::str; use crate::sys::cvt; use crate::sys::fd; use crate::sys_common::mutex::{StaticMutex, StaticMutexGuard}; -use crate::sys_common::rwlock::{RWLockGuard, StaticRWLock}; +use crate::sys_common::rwlock::{RWLockReadGuard, StaticRWLock}; use crate::vec; use libc::{c_char, c_int, c_void}; @@ -496,7 +496,7 @@ pub unsafe fn environ() -> *mut *const *const c_char { static ENV_LOCK: StaticRWLock = StaticRWLock::new(); -pub fn env_read_lock() -> RWLockGuard { +pub fn env_read_lock() -> RWLockReadGuard { ENV_LOCK.read_with_guard() } diff --git a/library/std/src/sys_common/rwlock.rs b/library/std/src/sys_common/rwlock.rs index cc13771009fab..41e8ad7729463 100644 --- a/library/std/src/sys_common/rwlock.rs +++ b/library/std/src/sys_common/rwlock.rs @@ -102,13 +102,13 @@ impl StaticRWLock { /// /// The lock is automatically unlocked when the returned guard is dropped. #[inline] - pub fn read_with_guard(&'static self) -> RWLockGuard { + pub fn read_with_guard(&'static self) -> RWLockReadGuard { // Safety: All methods require static references, therefore self // cannot be moved between invocations. unsafe { self.0.read(); } - RWLockGuard(&self.0, GuardType::Read) + RWLockReadGuard(&self.0) } /// Acquires write access to the underlying lock, blocking the current thread @@ -116,33 +116,32 @@ impl StaticRWLock { /// /// The lock is automatically unlocked when the returned guard is dropped. #[inline] - pub fn write_with_guard(&'static self) -> RWLockGuard { + pub fn write_with_guard(&'static self) -> RWLockWriteGuard { // Safety: All methods require static references, therefore self // cannot be moved between invocations. unsafe { self.0.write(); } - RWLockGuard(&self.0, GuardType::Write) + RWLockWriteGuard(&self.0) } } #[cfg(unix)] -enum GuardType { - Read, - Write, +pub struct RWLockReadGuard(&'static RWLock); + +#[cfg(unix)] +impl Drop for RWLockReadGuard { + fn drop(&mut self) { + unsafe { self.0.read_unlock() } + } } #[cfg(unix)] -pub struct RWLockGuard(&'static RWLock, GuardType); +pub struct RWLockWriteGuard(&'static RWLock); #[cfg(unix)] -impl Drop for RWLockGuard { +impl Drop for RWLockWriteGuard { fn drop(&mut self) { - unsafe { - match &self.1 { - GuardType::Read => self.0.read_unlock(), - GuardType::Write => self.0.write_unlock(), - } - } + unsafe { self.0.write_unlock() } } }