Skip to content

Commit

Permalink
Auto merge of #83121 - the8472:env-rwlock-2, r=joshtriplett
Browse files Browse the repository at this point in the history
use RWlock when accessing os::env (take 2)

This reverts commit acdca31 (#82877) i.e. redoes #81850 since the invalid unlock attempts in the child process have been fixed in #82949

r? `@joshtriplett`
  • Loading branch information
bors committed Mar 15, 2021
2 parents 2ccf063 + e22143c commit 107896c
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 12 deletions.
19 changes: 10 additions & 9 deletions library/std/src/sys/unix/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{RWLockReadGuard, StaticRWLock};
use crate::vec;

use libc::{c_char, c_int, c_void};
Expand Down Expand Up @@ -490,20 +491,20 @@ 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_lock() -> StaticMutexGuard {
// It is UB to attempt to acquire this mutex reentrantly!
static ENV_LOCK: StaticMutex = StaticMutex::new();
ENV_LOCK.lock()
static ENV_LOCK: StaticRWLock = StaticRWLock::new();

pub fn env_read_lock() -> RWLockReadGuard {
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_lock();
let _guard = env_read_lock();
let mut environ = *environ();
let mut result = Vec::new();
if !environ.is_null() {
Expand Down Expand Up @@ -540,7 +541,7 @@ pub fn getenv(k: &OsStr) -> io::Result<Option<OsString>> {
// always None as well
let k = CString::new(k.as_bytes())?;
unsafe {
let _guard = env_lock();
let _guard = env_read_lock();
let s = libc::getenv(k.as_ptr()) as *const libc::c_char;
let ret = if s.is_null() {
None
Expand All @@ -556,7 +557,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_LOCK.write_with_guard();
cvt(libc::setenv(k.as_ptr(), v.as_ptr(), 1)).map(drop)
}
}
Expand All @@ -565,7 +566,7 @@ pub fn unsetenv(n: &OsStr) -> io::Result<()> {
let nbuf = CString::new(n.as_bytes())?;

unsafe {
let _guard = env_lock();
let _guard = ENV_LOCK.write_with_guard();
cvt(libc::unsetenv(nbuf.as_ptr())).map(drop)
}
}
Expand Down
6 changes: 3 additions & 3 deletions library/std/src/sys/unix/process/process_unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl Command {
// a lock any more because the parent won't do anything and the child is
// in its own process. Thus the parent drops the lock guard while the child
// forgets it to avoid unlocking it on a new thread, which would be invalid.
let (env_lock, result) = unsafe { (sys::os::env_lock(), cvt(libc::fork())?) };
let (env_lock, result) = unsafe { (sys::os::env_read_lock(), cvt(libc::fork())?) };

let pid = unsafe {
match result {
Expand Down Expand Up @@ -127,7 +127,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_read_lock();

let Err(e) = self.do_exec(theirs, envp.as_ref());
e
Expand Down Expand Up @@ -407,7 +407,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_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,
Expand Down
59 changes: 59 additions & 0 deletions library/std/src/sys_common/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,62 @@ 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) -> RWLockReadGuard {
// SAFETY: All methods require static references, therefore self
// cannot be moved between invocations.
unsafe {
self.0.read();
}
RWLockReadGuard(&self.0)
}

/// 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) -> RWLockWriteGuard {
// SAFETY: All methods require static references, therefore self
// cannot be moved between invocations.
unsafe {
self.0.write();
}
RWLockWriteGuard(&self.0)
}
}

#[cfg(unix)]
pub struct RWLockReadGuard(&'static RWLock);

#[cfg(unix)]
impl Drop for RWLockReadGuard {
fn drop(&mut self) {
unsafe { self.0.read_unlock() }
}
}

#[cfg(unix)]
pub struct RWLockWriteGuard(&'static RWLock);

#[cfg(unix)]
impl Drop for RWLockWriteGuard {
fn drop(&mut self) {
unsafe { self.0.write_unlock() }
}
}

0 comments on commit 107896c

Please sign in to comment.