diff --git a/src/lib.rs b/src/lib.rs index cd0cdd7..5b680ee 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -151,6 +151,44 @@ struct HelperInner { consumer_done: bool, } +/// Error type for `from_env` function. +#[derive(Debug)] +pub enum ErrFromEnv { + /// There isn't env var, that describes jobserver to inherit. + IsNotConfigured, + /// Cannot connect following this process's environment. + PlatformSpecific { + /// Error. + err: io::Error, + /// Name of gotten env var. + env: &'static str, + /// Value of gotten env var. + var: String, + }, +} + +impl std::fmt::Display for ErrFromEnv { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ErrFromEnv::IsNotConfigured => { + write!(f, "couldn't find relevant environment variable") + } + ErrFromEnv::PlatformSpecific { err, env, var } => { + write!(f, "{err} ({env}={var}") + } + } + } +} + +impl std::error::Error for ErrFromEnv { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + ErrFromEnv::IsNotConfigured => None, + ErrFromEnv::PlatformSpecific { err, .. } => Some(err), + } + } +} + impl Client { /// Creates a new jobserver initialized with the given parallelism limit. /// @@ -197,8 +235,9 @@ impl Client { /// # Return value /// /// If a jobserver was found in the environment and it looks correct then - /// `Some` of the connected client will be returned. If no jobserver was - /// found then `None` will be returned. + /// `Ok` of the connected client will be returned. If no relevant env var + /// was found then `Err(IsNotConfigured)` will be returned. In other cases + /// `Err(PlatformSpecific)` will be returned. /// /// Note that on Unix the `Client` returned **takes ownership of the file /// descriptors specified in the environment**. Jobservers on Unix are @@ -211,6 +250,9 @@ impl Client { /// with `CLOEXEC` so they're not automatically inherited by spawned /// children. /// + /// On unix if `unix_check_is_pipe` enabled this function will check if + /// provided files are actually pipes. + /// /// # Safety /// /// This function is `unsafe` to call on Unix specifically as it @@ -227,28 +269,36 @@ impl Client { /// /// Note, though, that on Windows it should be safe to call this function /// any number of times. - pub unsafe fn from_env() -> Option { - let var = match env::var("CARGO_MAKEFLAGS") - .or_else(|_| env::var("MAKEFLAGS")) - .or_else(|_| env::var("MFLAGS")) - { - Ok(s) => s, - Err(_) => return None, - }; - let mut arg = "--jobserver-fds="; - let pos = match var.find(arg) { - Some(i) => i, - None => { - arg = "--jobserver-auth="; - match var.find(arg) { - Some(i) => i, - None => return None, - } - } - }; + pub unsafe fn from_env_ext(check_pipe: bool) -> Result { + let (env, var) = ["CARGO_MAKEFLAGS", "MAKEFLAGS", "MFLAGS"] + .iter() + .map(|&env| env::var(env).map(|var| (env, var))) + .find_map(|p| p.ok()) + .ok_or(ErrFromEnv::IsNotConfigured)?; + + let (arg, pos) = ["--jobserver-fds=", "--jobserver-auth="] + .iter() + .map(|&arg| var.find(arg).map(|pos| (arg, pos))) + .find_map(|pos| pos) + .ok_or(ErrFromEnv::IsNotConfigured)?; let s = var[pos + arg.len()..].split(' ').next().unwrap(); - imp::Client::open(s).map(|c| Client { inner: Arc::new(c) }) + #[cfg(unix)] + let imp_client = imp::Client::open(s, check_pipe); + #[cfg(not(unix))] + let imp_client = imp::Client::open(s); + match imp_client { + Ok(c) => Ok(Client { inner: Arc::new(c) }), + Err(err) => Err(ErrFromEnv::PlatformSpecific { err, env, var }), + } + } + + /// Attempts to connect to the jobserver specified in this process's + /// environment. + /// + /// Wraps `from_env_ext` and discards error details. + pub unsafe fn from_env() -> Option { + Self::from_env_ext(false).ok() } /// Acquires a token from this jobserver client. diff --git a/src/unix.rs b/src/unix.rs index e4b1435..8991825 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -81,62 +81,63 @@ impl Client { Ok(Client::from_fds(pipes[0], pipes[1])) } - pub unsafe fn open(s: &str) -> Option { - Client::from_fifo(s).or_else(|| Client::from_pipe(s)) + pub unsafe fn open(s: &str, check_pipe: bool) -> io::Result { + if let Some(client) = Self::from_fifo(s)? { + return Ok(client); + } + if let Some(client) = Self::from_pipe(s, check_pipe)? { + return Ok(client); + } + Err(io::Error::new( + io::ErrorKind::InvalidInput, + "unrecognized format of environment variable", + )) } /// `--jobserver-auth=fifo:PATH` - fn from_fifo(s: &str) -> Option { + fn from_fifo(s: &str) -> io::Result> { let mut parts = s.splitn(2, ':'); if parts.next().unwrap() != "fifo" { - return None; + return Ok(None); } - let path = match parts.next() { - Some(p) => Path::new(p), - None => return None, - }; - let file = match OpenOptions::new().read(true).write(true).open(path) { - Ok(f) => f, - Err(_) => return None, - }; - Some(Client::Fifo { + let path = Path::new(parts.next().ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidInput, "expected ':' after `fifo`") + })?); + let file = OpenOptions::new().read(true).write(true).open(path)?; + Ok(Some(Client::Fifo { file, path: path.into(), - }) + })) } /// `--jobserver-auth=R,W` - unsafe fn from_pipe(s: &str) -> Option { + unsafe fn from_pipe(s: &str, check_pipe: bool) -> io::Result> { let mut parts = s.splitn(2, ','); let read = parts.next().unwrap(); let write = match parts.next() { - Some(s) => s, - None => return None, - }; - - let read = match read.parse() { - Ok(n) => n, - Err(_) => return None, - }; - let write = match write.parse() { - Ok(n) => n, - Err(_) => return None, + Some(w) => w, + None => return Ok(None), }; + let read = read + .parse() + .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?; + let write = write + .parse() + .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?; // Ok so we've got two integers that look like file descriptors, but // for extra sanity checking let's see if they actually look like - // instances of a pipe before we return the client. + // instances of a pipe if feature enabled or valid files otherwise + // before we return the client. // // If we're called from `make` *without* the leading + on our rule // then we'll have `MAKEFLAGS` env vars but won't actually have // access to the file descriptors. - if is_valid_fd(read) && is_valid_fd(write) { - drop(set_cloexec(read, true)); - drop(set_cloexec(write, true)); - Some(Client::from_fds(read, write)) - } else { - None - } + check_fd(read, check_pipe)?; + check_fd(write, check_pipe)?; + drop(set_cloexec(read, true)); + drop(set_cloexec(write, true)); + Ok(Some(Client::from_fds(read, write))) } unsafe fn from_fds(read: c_int, write: c_int) -> Client { @@ -207,7 +208,7 @@ impl Client { return Err(io::Error::new( io::ErrorKind::Other, "early EOF on jobserver pipe", - )) + )); } Err(e) => match e.kind() { io::ErrorKind::WouldBlock => { /* fall through to polling */ } @@ -326,7 +327,7 @@ pub(crate) fn spawn_helper( client: client.inner.clone(), data, disabled: false, - })) + })); } Err(e) => break f(Err(e)), Ok(None) if helper.producer_done() => break, @@ -385,8 +386,32 @@ impl Helper { } } -fn is_valid_fd(fd: c_int) -> bool { - unsafe { libc::fcntl(fd, libc::F_GETFD) != -1 } +unsafe fn check_fd(fd: c_int, check_pipe: bool) -> io::Result<()> { + if check_pipe { + let mut stat = mem::zeroed(); + if libc::fstat(fd, &mut stat) == -1 { + Err(io::Error::last_os_error()) + } else { + // On android arm and i686 mode_t is u16 and st_mode is u32, + // this generates a type mismatch when S_IFIFO (declared as mode_t) + // is used in operations with st_mode, so we use this workaround + // to get the value of S_IFIFO with the same type of st_mode. + let mut s_ififo = stat.st_mode; + s_ififo = libc::S_IFIFO as _; + if stat.st_mode & s_ififo == s_ififo { + return Ok(()); + } + Err(io::Error::last_os_error()) // + } + } else { + match libc::fcntl(fd, libc::F_GETFD) { + r if r == -1 => Err(io::Error::new( + io::ErrorKind::InvalidData, + format!("{fd} is not a pipe"), + )), + _ => Ok(()), + } + } } fn set_cloexec(fd: c_int, set: bool) -> io::Result<()> { diff --git a/src/wasm.rs b/src/wasm.rs index 3793bd6..a542fcf 100644 --- a/src/wasm.rs +++ b/src/wasm.rs @@ -27,8 +27,8 @@ impl Client { }) } - pub unsafe fn open(_s: &str) -> Option { - None + pub unsafe fn open(_s: &str) -> io::Result { + Err(io::ErrorKind::Unsupported.into()) } pub fn acquire(&self) -> io::Result { diff --git a/src/windows.rs b/src/windows.rs index 6791efe..bea8c05 100644 --- a/src/windows.rs +++ b/src/windows.rs @@ -127,17 +127,14 @@ impl Client { )) } - pub unsafe fn open(s: &str) -> Option { - let name = match CString::new(s) { - Ok(s) => s, - Err(_) => return None, - }; + pub unsafe fn open(s: &str) -> io::Result { + let name = CString::new(s).map_err(|e| io::Error::new(io::ErrorKind::Other, e))?; let sem = OpenSemaphoreA(SYNCHRONIZE | SEMAPHORE_MODIFY_STATE, FALSE, name.as_ptr()); if sem.is_null() { - None + Err(io::Error::last_os_error()) } else { - Some(Client { + Ok(Client { sem: Handle(sem), name: s.to_string(), })