Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion library/std/src/sys/unix/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ pub fn errno() -> i32 {
}

/// Sets the platform-specific value of errno
#[cfg(all(not(target_os = "linux"), not(target_os = "dragonfly")))] // needed for readdir and syscall!
#[cfg(not(target_os = "dragonfly"))] // needed for readdir and syscall!
#[allow(dead_code)] // but not all target cfgs actually end up using it
pub fn set_errno(e: i32) {
unsafe { *errno_location() = e as c_int }
Expand Down
105 changes: 90 additions & 15 deletions library/std/src/sys/unix/process/process_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::sys_common::process::CommandEnv;
#[cfg(not(target_os = "fuchsia"))]
use crate::sys::fs::OpenOptions;

use libc::{c_char, c_int, gid_t, uid_t, EXIT_FAILURE, EXIT_SUCCESS};
use libc::{c_char, c_int, gid_t, strlen, uid_t, EXIT_FAILURE, EXIT_SUCCESS};

cfg_if::cfg_if! {
if #[cfg(target_os = "fuchsia")] {
Expand Down Expand Up @@ -75,11 +75,12 @@ pub struct Command {
args: Vec<CString>,
argv: Argv,
env: CommandEnv,
arg_max: Option<isize>,
Copy link
Contributor

@pickfire pickfire Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can arg_max possibly be Some(0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't make sense to be that. Are you suggesting a naked isize?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either that or NonZero types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks cool. Let me get it changed.


cwd: Option<CString>,
uid: Option<uid_t>,
gid: Option<gid_t>,
saw_nul: bool,
problem: Problem,
closures: Vec<Box<dyn FnMut() -> io::Result<()> + Send + Sync>>,
stdin: Option<Stdio>,
stdout: Option<Stdio>,
Expand Down Expand Up @@ -128,19 +129,27 @@ pub enum Stdio {
Fd(FileDesc),
}

#[derive(Copy, Clone)]
pub enum Problem {
Ok,
SawNul,
Oversized,
}

impl Command {
pub fn new(program: &OsStr) -> Command {
let mut saw_nul = false;
let program = os2c(program, &mut saw_nul);
let mut problem = Problem::Ok;
let program = os2c(program, &mut problem);
Command {
argv: Argv(vec![program.as_ptr(), ptr::null()]),
args: vec![program.clone()],
program,
env: Default::default(),
arg_max: Default::default(),
cwd: None,
uid: None,
gid: None,
saw_nul,
problem,
closures: Vec::new(),
stdin: None,
stdout: None,
Expand All @@ -150,16 +159,25 @@ impl Command {

pub fn set_arg_0(&mut self, arg: &OsStr) {
// Set a new arg0
let arg = os2c(arg, &mut self.saw_nul);
let arg = os2c(arg, &mut self.problem);
debug_assert!(self.argv.0.len() > 1);
self.argv.0[0] = arg.as_ptr();
self.args[0] = arg;
}

pub fn maybe_arg(&mut self, arg: &OsStr) -> io::Result<()> {
self.arg(arg);
self.problem.as_result()?;
if self.check_size(false)? == false {
self.problem = Problem::Oversized;
}
self.problem.as_result()
}

pub fn arg(&mut self, arg: &OsStr) {
// Overwrite the trailing NULL pointer in `argv` and then add a new null
// pointer.
let arg = os2c(arg, &mut self.saw_nul);
let arg = os2c(arg, &mut self.problem);
self.argv.0[self.args.len()] = arg.as_ptr();
self.argv.0.push(ptr::null());

Expand All @@ -169,7 +187,7 @@ impl Command {
}

pub fn cwd(&mut self, dir: &OsStr) {
self.cwd = Some(os2c(dir, &mut self.saw_nul));
self.cwd = Some(os2c(dir, &mut self.problem));
}
pub fn uid(&mut self, id: uid_t) {
self.uid = Some(id);
Expand All @@ -178,8 +196,8 @@ impl Command {
self.gid = Some(id);
}

pub fn saw_nul(&self) -> bool {
self.saw_nul
pub fn problem(&self) -> Problem {
self.problem
}
pub fn get_argv(&self) -> &Vec<*const c_char> {
&self.argv.0
Expand All @@ -202,6 +220,49 @@ impl Command {
self.gid
}

pub fn get_size(&mut self) -> io::Result<usize> {
use crate::mem;
let argv = &self.argv.0;
let argv_size: usize = argv.iter().map(|x| unsafe { strlen(*x) + 1 }).sum::<usize>()
Copy link
Contributor Author

@Artoria2e5 Artoria2e5 Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh great I forgot that the last element is a nullptr!

But seriously I should this without strlen... like accumulating a length in add when it's still an OsStr.

+ (argv.len() + 1) * mem::size_of::<*const u8>();

// Envp size calculation is approximate.
let env = self.env.capture();
let env_size: usize = env
.iter()
.map(|(k, v)| {
os2c(k.as_ref(), &mut self.problem).to_bytes().len()
+ os2c(v.as_ref(), &mut self.problem).to_bytes().len()
+ 2
})
.sum::<usize>()
+ (env.len() + 1) * mem::size_of::<*const u8>();

Ok(argv_size + env_size)
}

pub fn check_size(&mut self, refresh: bool) -> io::Result<bool> {
use crate::sys;
use core::convert::TryInto;
if refresh || self.arg_max.is_none() {
let (limit, errno) = unsafe {
let old_errno = sys::os::errno();
sys::os::set_errno(0);
Comment on lines +292 to +293
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should we clear existing errno?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the no-error case automatically clears errno. Does it?

let limit = libc::sysconf(libc::_SC_ARG_MAX);
let errno = sys::os::errno();
sys::os::set_errno(old_errno);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary, right? I think it is expected that any function in libstd that interacts with the OS in some way can clobber errno.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I didn't know that!

(limit, errno)
};

if errno != 0 {
return Err(io::Error::from_raw_os_error(errno));
} else {
self.arg_max = limit.try_into().ok();
}
}
Ok(self.arg_max.unwrap() < 0 || self.get_size()? < (self.arg_max.unwrap() as usize))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't arg_max be usize so it won't be less than zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, a negative value indicates that the arg size is unlimited. If my reading of POSIX is right, that is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC -0 of usize is still -1 for unlimited.

}

pub fn get_closures(&mut self) -> &mut Vec<Box<dyn FnMut() -> io::Result<()> + Send + Sync>> {
&mut self.closures
}
Expand All @@ -228,7 +289,7 @@ impl Command {

pub fn capture_env(&mut self) -> Option<CStringArray> {
let maybe_env = self.env.capture_if_changed();
maybe_env.map(|env| construct_envp(env, &mut self.saw_nul))
maybe_env.map(|env| construct_envp(env, &mut self.problem))
}
#[allow(dead_code)]
pub fn env_saw_path(&self) -> bool {
Expand All @@ -254,9 +315,9 @@ impl Command {
}
}

fn os2c(s: &OsStr, saw_nul: &mut bool) -> CString {
fn os2c(s: &OsStr, problem: &mut Problem) -> CString {
CString::new(s.as_bytes()).unwrap_or_else(|_e| {
*saw_nul = true;
*problem = Problem::SawNul;
CString::new("<string-with-nul>").unwrap()
})
}
Expand Down Expand Up @@ -287,7 +348,7 @@ impl CStringArray {
}
}

fn construct_envp(env: BTreeMap<OsString, OsString>, saw_nul: &mut bool) -> CStringArray {
fn construct_envp(env: BTreeMap<OsString, OsString>, problem: &mut Problem) -> CStringArray {
let mut result = CStringArray::with_capacity(env.len());
for (mut k, v) in env {
// Reserve additional space for '=' and null terminator
Expand All @@ -299,7 +360,7 @@ fn construct_envp(env: BTreeMap<OsString, OsString>, saw_nul: &mut bool) -> CStr
if let Ok(item) = CString::new(k.into_vec()) {
result.push(item);
} else {
*saw_nul = true;
*problem = Problem::SawNul;
}
}

Expand Down Expand Up @@ -373,6 +434,20 @@ impl ChildStdio {
}
}

impl Problem {
pub fn as_result(&self) -> io::Result<()> {
match *self {
Problem::Ok => Ok(()),
Problem::SawNul => {
Err(io::Error::new(io::ErrorKind::InvalidInput, "nul byte found in provided data"))
}
Problem::Oversized => {
Err(io::Error::new(io::ErrorKind::InvalidInput, "Oversized command"))
}
}
}
}

impl fmt::Debug for Command {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
if self.program != self.args[0] {
Expand Down
11 changes: 3 additions & 8 deletions library/std/src/sys/unix/process/process_fuchsia.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,7 @@ impl Command {
) -> io::Result<(Process, StdioPipes)> {
let envp = self.capture_env();

if self.saw_nul() {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"nul byte found in provided data",
));
}
self.problem().as_result()?;

let (ours, theirs) = self.setup_io(default, needs_stdin)?;

Expand All @@ -36,8 +31,8 @@ impl Command {
}

pub fn exec(&mut self, default: Stdio) -> io::Error {
if self.saw_nul() {
return io::Error::new(io::ErrorKind::InvalidInput, "nul byte found in provided data");
if let Err(err) = self.problem().as_result() {
return err;
}

match self.setup_io(default, true) {
Expand Down
26 changes: 18 additions & 8 deletions library/std/src/sys/unix/process/process_unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ impl Command {

let envp = self.capture_env();

if self.saw_nul() {
return Err(io::Error::new(ErrorKind::InvalidInput, "nul byte found in provided data"));
}
self.problem().as_result()?;

let (ours, theirs) = self.setup_io(default, needs_stdin)?;

Expand Down Expand Up @@ -110,8 +108,8 @@ impl Command {
pub fn exec(&mut self, default: Stdio) -> io::Error {
let envp = self.capture_env();

if self.saw_nul() {
return io::Error::new(ErrorKind::InvalidInput, "nul byte found in provided data");
if let Err(err) = self.problem().as_result() {
return err;
}

match self.setup_io(default, true) {
Expand Down Expand Up @@ -389,7 +387,11 @@ impl Command {
self.get_argv().as_ptr() as *const _,
envp as *const _,
);
if ret == 0 { Ok(Some(p)) } else { Err(io::Error::from_raw_os_error(ret)) }
if ret == 0 {
Ok(Some(p))
} else {
Err(io::Error::from_raw_os_error(ret))
}
}
}
}
Expand Down Expand Up @@ -467,11 +469,19 @@ impl ExitStatus {
}

pub fn code(&self) -> Option<i32> {
if self.exited() { Some(unsafe { libc::WEXITSTATUS(self.0) }) } else { None }
if self.exited() {
Some(unsafe { libc::WEXITSTATUS(self.0) })
} else {
None
}
}

pub fn signal(&self) -> Option<i32> {
if !self.exited() { Some(unsafe { libc::WTERMSIG(self.0) }) } else { None }
if !self.exited() {
Some(unsafe { libc::WTERMSIG(self.0) })
} else {
None
}
}
}

Expand Down
Loading