From 88c6c77505e5d06c7139cca6088321a60f4ff415 Mon Sep 17 00:00:00 2001 From: schneems Date: Sat, 18 Mar 2023 12:58:09 -0500 Subject: [PATCH 1/2] Add ability to see inherited envs on Command This is a reference implementation for one of the solution "sketches" in https://github.com/rust-lang/libs-team/issues/194. ## Problem statement(s) - Problem 1: Cannot observe effects of `Command::env_clear` - Problem 2: The "capture" logic has no exposed single source of truth Problem 1 is a capability problem (I can't work around this). Problem 2 is a usability and stability problem. ### Problem 1: Cannot observe effects of `Command::env_clear` As documented in [my merged PR](https://github.com/rust-lang/rust/pull/109272), calling `env_clear` will cause the "capture" logic to skip inheriting from the parent process. While a developer can set this value, they cannot programmatically observe if it has been set (though they can manually observe it via "alternative" debug formatting `{:#?}`). The end goals of programmatically observing that effect is listed below in "Motivation". This problem prevents me from reproducing the capture logic in a library where I cannot observe if `env_clear` has been called. Even if this problem is solved, a related problem is detailed below. ### Problem 2: The "capture" logic has no exposed single source of truth Even if we could directly observe the effects of calling `env_clear`, every developer who needed this information would need to reproduce this "capture" logic https://github.com/rust-lang/rust/blob/3a5c8e91f094bb1cb1346651fe3512f0b603d826/library/std/src/sys_common/process.rs#L36-L51 and hope that it does not change or that their implementation does not diverge. If the outcome of this logic is exposed, then it will: - Solve problem 1 - Prevent divergent reimplementation of "capture" logic ### [sketch] Expose captured env logic via a new `capture_envs` method on `Command` We could add a method that exposes that information directly: ```rust let command = Command::new("ls"); let envs: HashMap = command.capture_envs().collect(); ``` The name `capture` matches internal naming concepts and helps to reinforce that the value method will return a point in time value. This sketch would solve problems 1 and 2. I'm open to name suggestions. --- library/std/src/process.rs | 44 ++++++++++++++++++- .../src/sys/unix/process/process_common.rs | 6 ++- library/std/src/sys_common/process.rs | 36 ++++++++++++++- 3 files changed, 82 insertions(+), 4 deletions(-) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index ad29eeb6a0bed..7622052865661 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -116,6 +116,8 @@ use crate::path::Path; use crate::str; use crate::sys::pipe::{read2, AnonPipe}; use crate::sys::process as imp; +#[unstable(feature = "command_captured_envs_access", issue = "none")] +pub use crate::sys_common::process::CapturedEnvs; #[stable(feature = "command_access", since = "1.57.0")] pub use crate::sys_common::process::CommandEnvs; use crate::sys_common::{AsInner, AsInnerMut, FromInner, IntoInner}; @@ -1048,8 +1050,9 @@ impl Command { /// Environment variables explicitly set using [`Command::env`], [`Command::envs`], and /// [`Command::env_remove`] can be retrieved with this method. /// - /// Note that this output does not include environment variables inherited from the parent - /// process. + /// Note that this output does not include environment variables inherited + /// from the parent process. To get the variables that will be set + /// when the child spawns use [`Command::capture_envs`]. /// /// Each element is a tuple key/value pair `(&OsStr, Option<&OsStr>)`. A [`None`] value /// indicates its key was explicitly removed via [`Command::env_remove`]. The associated key for @@ -1078,6 +1081,43 @@ impl Command { self.inner.get_envs() } + /// Captures a snapshot of all environment variables, inherited and explicitly set, that would + /// have been passed to the child process. + /// + /// Return value is an iterator where each element is a tuple `(OsString, OsString)`. The first + /// value is the environment variable key, and the second is its value. Unlike + /// [`Command::get_envs`], this method includes inherited and explicitly set environment + /// variables. + /// + /// Note that this method does not prevent time-of-check to time-of-use (TOCTOU) bugs. You + /// should only use it when those bugs are not an issue. If no changes occur between when this + /// method is called and when the command is spawned, the values returned will be the same as + /// those of the spawned child process. + /// + /// # Examples + /// + /// ``` + /// use std::ffi::OsString; + /// use std::process::Command; + /// use std::collections::HashMap; + /// + /// let mut cmd = Command::new("ls"); + /// cmd.env("TERM", "dumb"); + /// cmd.env_remove("TZ"); + /// + /// let envs: HashMap = cmd.capture_envs().collect(); + /// // Inherited environment variable(s) + /// assert!(envs.contains_key(&OsString::from("PATH"))); + /// + /// // Explicitly set environment variable(s) + /// assert!(envs.contains_key(&OsString::from("TERM"))); + /// assert!(!envs.contains_key(&OsString::from("TZ"))); + /// ``` + #[unstable(feature = "command_captured_envs_access", issue = "none")] + pub fn capture_envs(&self) -> CapturedEnvs { + self.inner.capture_envs() + } + /// Returns the working directory for the child process. /// /// This returns [`None`] if the working directory will not be changed. diff --git a/library/std/src/sys/unix/process/process_common.rs b/library/std/src/sys/unix/process/process_common.rs index bac32d9e60e11..010c90e9e3364 100644 --- a/library/std/src/sys/unix/process/process_common.rs +++ b/library/std/src/sys/unix/process/process_common.rs @@ -12,7 +12,7 @@ use crate::ptr; use crate::sys::fd::FileDesc; use crate::sys::fs::File; use crate::sys::pipe::{self, AnonPipe}; -use crate::sys_common::process::{CommandEnv, CommandEnvs}; +use crate::sys_common::process::{CapturedEnvs, CommandEnv, CommandEnvs}; use crate::sys_common::{FromInner, IntoInner}; #[cfg(not(target_os = "fuchsia"))] @@ -302,6 +302,10 @@ impl Command { self.env.iter() } + pub fn capture_envs(&self) -> CapturedEnvs { + self.env.capture_iter() + } + pub fn get_current_dir(&self) -> Option<&Path> { self.cwd.as_ref().map(|cs| Path::new(OsStr::from_bytes(cs.as_bytes()))) } diff --git a/library/std/src/sys_common/process.rs b/library/std/src/sys_common/process.rs index 4d295cf0f09d5..fc9f0deea1f32 100644 --- a/library/std/src/sys_common/process.rs +++ b/library/std/src/sys_common/process.rs @@ -98,9 +98,43 @@ impl CommandEnv { let iter = self.vars.iter(); CommandEnvs { iter } } + + pub fn capture_iter(&self) -> CapturedEnvs { + let iter = self.capture().into_iter(); + + CapturedEnvs { iter } + } } -/// An iterator over the command environment variables. +/// An iterator over captured [`Command`][crate::process::Command] environment variables. +/// +/// This struct is created by +/// [`Command::capture_envs`][crate::process::Command::capture_envs]. It includes inherited and +/// explicitly set environment variables. +/// +/// See [`Command::capture_envs`][crate::process::Command::capture_envs] documentation for more. +pub struct CapturedEnvs { + iter: crate::collections::btree_map::IntoIter, +} +impl Iterator for CapturedEnvs { + type Item = (OsString, OsString); + fn next(&mut self) -> Option<(OsString, OsString)> { + self.iter.next() + } + fn size_hint(&self) -> (usize, Option) { + self.iter.size_hint() + } +} + +/// An iterator over the explicitly set [`Command`][crate::process::Command] environment variables. +/// +/// Each element is a tuple key/value pair `(&OsStr, Option<&OsStr>)`. A [`None`] value +/// indicates its key was explicitly removed. The associated key for the [`None`] value will no +/// longer inherit from its parent process. +/// +/// Note that this output does not include environment variables inherited from the parent process. +/// To get the variables that will be set when the child spawns use +/// [`Command::captured_envs`][crate::process::Command::captured_envs]. /// /// This struct is created by /// [`Command::get_envs`][crate::process::Command::get_envs]. See its From 4266fead2442c8c6203d715958792a991a741c77 Mon Sep 17 00:00:00 2001 From: schneems Date: Tue, 25 Apr 2023 10:19:35 -0500 Subject: [PATCH 2/2] Windows? --- library/std/src/sys/windows/process.rs | 6 +++++- library/std/src/sys_common/process.rs | 9 ++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/library/std/src/sys/windows/process.rs b/library/std/src/sys/windows/process.rs index f4078d359448e..eefb0a482da8c 100644 --- a/library/std/src/sys/windows/process.rs +++ b/library/std/src/sys/windows/process.rs @@ -26,7 +26,7 @@ use crate::sys::handle::Handle; use crate::sys::path; use crate::sys::pipe::{self, AnonPipe}; use crate::sys::stdio; -use crate::sys_common::process::{CommandEnv, CommandEnvs}; +use crate::sys_common::process::{CapturedEnvs, CommandEnv, CommandEnvs}; use crate::sys_common::IntoInner; use core::ffi::c_void; @@ -244,6 +244,10 @@ impl Command { self.env.iter() } + pub fn capture_envs(&self) -> CapturedEnvs { + self.env.capture_iter() + } + pub fn get_current_dir(&self) -> Option<&Path> { self.cwd.as_ref().map(|cwd| Path::new(cwd)) } diff --git a/library/std/src/sys_common/process.rs b/library/std/src/sys_common/process.rs index fc9f0deea1f32..c7f5966bf21a6 100644 --- a/library/std/src/sys_common/process.rs +++ b/library/std/src/sys_common/process.rs @@ -114,13 +114,16 @@ impl CommandEnv { /// /// See [`Command::capture_envs`][crate::process::Command::capture_envs] documentation for more. pub struct CapturedEnvs { - iter: crate::collections::btree_map::IntoIter, + iter: crate::collections::btree_map::IntoIter, } + impl Iterator for CapturedEnvs { type Item = (OsString, OsString); - fn next(&mut self) -> Option<(OsString, OsString)> { - self.iter.next() + + fn next(&mut self) -> Option { + self.iter.next().into() } + fn size_hint(&self) -> (usize, Option) { self.iter.size_hint() }