Snap for 7298338 from 82c0f84acd62abe0e038758f24ebe27cfc4e17e4 to sc-release
Change-Id: I6a3f799cf039d04e1349a8bc01d846e5c648e018
diff --git a/OWNERS b/OWNERS
index 9615a27..ebadeca 100644
--- a/OWNERS
+++ b/OWNERS
@@ -3,5 +3,3 @@
[email protected]
[email protected]
[email protected]
-
-per-file rust/** = file:/OWNERS.rust
diff --git a/libminijail.c b/libminijail.c
index 582554a..e0d9d54 100644
--- a/libminijail.c
+++ b/libminijail.c
@@ -8,6 +8,7 @@
#define _GNU_SOURCE
#include <asm/unistd.h>
+#include <assert.h>
#include <dirent.h>
#include <errno.h>
#include <fcntl.h>
@@ -26,6 +27,7 @@
#include <sys/param.h>
#include <sys/prctl.h>
#include <sys/resource.h>
+#include <sys/select.h>
#include <sys/stat.h>
#include <sys/sysmacros.h>
#include <sys/types.h>
@@ -2592,8 +2594,74 @@
return 0;
}
+/* Return true if the specified file descriptor is already open. */
+static int fd_is_open(int fd)
+{
+ return fcntl(fd, F_GETFD) != -1 || errno != EBADF;
+}
+
+static_assert(FD_SETSIZE >= MAX_PRESERVED_FDS * 2 - 1,
+ "If true, ensure_no_fd_conflict will always find an unused fd.");
+
+/* If p->parent_fd will be used by a child_fd, move it to an unused fd. */
+static int ensure_no_fd_conflict(const fd_set* child_fds,
+ struct preserved_fd* p)
+{
+ if (!FD_ISSET(p->parent_fd, child_fds)){
+ return 0;
+ }
+
+ /*
+ * If no other parent_fd matches the child_fd then use it instead of a
+ * temporary.
+ */
+ int fd = p->child_fd;
+ if (fd_is_open(fd)) {
+ fd = FD_SETSIZE - 1;
+ while (FD_ISSET(fd, child_fds) || fd_is_open(fd)) {
+ --fd;
+ if (fd < 0) {
+ die("failed to find an unused fd");
+ }
+ }
+ }
+
+ int ret = dup2(p->parent_fd, fd);
+ /*
+ * warn() opens a file descriptor so it needs to happen after dup2 to
+ * avoid unintended side effects. This can be avoided by reordering the
+ * mapping requests so that the source fds with overlap are mapped
+ * first (unless there are cycles).
+ */
+ warn("mapped fd overlap: moving %d to %d", p->parent_fd, fd);
+ if (ret == -1) {
+ return -1;
+ }
+
+ p->parent_fd = fd;
+ return 0;
+}
+
static int redirect_fds(struct minijail *j)
{
+ fd_set child_fds;
+ FD_ZERO(&child_fds);
+
+ /* Relocate parent_fds that would be replaced by a child_fd. */
+ for (size_t i = 0; i < j->preserved_fd_count; i++) {
+ int child_fd = j->preserved_fds[i].child_fd;
+ if (FD_ISSET(child_fd, &child_fds)) {
+ die("fd %d is mapped more than once", child_fd);
+ }
+
+ if (ensure_no_fd_conflict(&child_fds,
+ &j->preserved_fds[i]) == -1) {
+ return -1;
+ }
+
+ FD_SET(child_fd, &child_fds);
+ }
+
for (size_t i = 0; i < j->preserved_fd_count; i++) {
if (j->preserved_fds[i].parent_fd ==
j->preserved_fds[i].child_fd) {
@@ -2609,13 +2677,10 @@
* fds that are *not* child fds.
*/
for (size_t i = 0; i < j->preserved_fd_count; i++) {
- int closeable = true;
- for (size_t i2 = 0; i2 < j->preserved_fd_count; i2++) {
- closeable &= j->preserved_fds[i].parent_fd !=
- j->preserved_fds[i2].child_fd;
+ int parent_fd = j->preserved_fds[i].parent_fd;
+ if (!FD_ISSET(parent_fd, &child_fds)) {
+ close(parent_fd);
}
- if (closeable)
- close(j->preserved_fds[i].parent_fd);
}
return 0;
}
diff --git a/OWNERS.rust b/rust/OWNERS
similarity index 100%
rename from OWNERS.rust
rename to rust/OWNERS
diff --git a/rust/minijail/src/lib.rs b/rust/minijail/src/lib.rs
index 289668e..d4f5787 100644
--- a/rust/minijail/src/lib.rs
+++ b/rust/minijail/src/lib.rs
@@ -9,7 +9,7 @@
use std::fs;
use std::io;
use std::os::raw::{c_char, c_ulong, c_ushort};
-use std::os::unix::io::{AsRawFd, RawFd};
+use std::os::unix::io::{AsRawFd, IntoRawFd, RawFd};
use std::path::{Path, PathBuf};
use std::ptr::{null, null_mut};
@@ -733,6 +733,10 @@
/// could cause a lot of trouble if not handled carefully. FDs 0, 1, and 2
/// are overwritten with /dev/null FDs unless they are included in the
/// inheritable_fds list.
+ ///
+ /// Also, any Rust objects that own fds may try to close them after the fork. If they belong
+ /// to a fd number that was mapped to, the mapped fd will be closed instead.
+ ///
/// This Function may abort in the child on error because a partially
/// entered jail isn't recoverable.
pub unsafe fn fork(&self, inheritable_fds: Option<&[RawFd]>) -> Result<pid_t> {
@@ -787,6 +791,10 @@
if ret < 0 {
return Err(Error::ForkingMinijail(ret));
}
+ if ret == 0 {
+ // Safe because dev_null was remapped.
+ dev_null.into_raw_fd();
+ }
Ok(ret as pid_t)
}
diff --git a/rust/minijail/tests/fork_remap.rs b/rust/minijail/tests/fork_remap.rs
index 8ace83b..6cf3415 100644
--- a/rust/minijail/tests/fork_remap.rs
+++ b/rust/minijail/tests/fork_remap.rs
@@ -10,40 +10,51 @@
use std::fs::{read_link, File, OpenOptions};
use std::io::{self, Read};
-use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
+use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd};
use std::path::Path;
use minijail::Minijail;
const DEV_NULL: &str = "/dev/null";
const DEV_ZERO: &str = "/dev/zero";
+const PROC_CMDLINE: &str = "/proc/self/cmdline";
-const DEST_FD1: RawFd = 7;
-const DEST_FD2: RawFd = 8;
-
-fn open_dev_zero() -> Result<File, io::Error> {
+fn open_path(path: &str) -> Result<File, io::Error> {
OpenOptions::new()
.read(true)
- .write(true)
- .open(Path::new(DEV_ZERO))
+ .write(false)
+ .open(Path::new(path))
}
fn main() {
- let mut check_file1 = open_dev_zero().unwrap();
- let mut check_file2 = open_dev_zero().unwrap();
+ let mut check_file1 = open_path(DEV_ZERO).unwrap();
+ let mut check_file2 = open_path(PROC_CMDLINE).unwrap();
let j = Minijail::new().unwrap();
- for p in &[0, 1, check_file1.as_raw_fd(), check_file2.as_raw_fd()] {
+ let mut stdio_expected = String::new();
+ let mut file2_expected = String::new();
+ for &p in &[0, 1, 2, check_file1.as_raw_fd(), check_file2.as_raw_fd()] {
let path = format!("/proc/self/fd/{}", p);
let target = read_link(Path::new(&path));
eprintln!("P: {} -> {:?}", p, &target);
+ if p == 2 {
+ stdio_expected = target.unwrap().to_string_lossy().to_string();
+ } else if p == check_file2.as_raw_fd() {
+ file2_expected = target.unwrap().to_string_lossy().to_string();
+ }
}
+ // Swap fd1 and fd2.
+ let dest_fd1: RawFd = check_file2.as_raw_fd();
+ let dest_fd2: RawFd = check_file1.as_raw_fd();
+
if unsafe {
j.fork_remap(&[
- (2, 2),
- (check_file1.as_raw_fd(), DEST_FD1),
- (check_file2.as_raw_fd(), DEST_FD2),
+ // fd 0 tests stdio mapped to /dev/null.
+ (2, 1), // One-to-many.
+ (2, 2), // Identity.
+ (check_file1.as_raw_fd(), dest_fd1), // Cross-over.
+ (check_file2.as_raw_fd(), dest_fd2), // Cross-over.
])
}
.unwrap()
@@ -54,21 +65,27 @@
return;
}
- // Safe because we are re-taking ownership of a remapped fd after forking.
- check_file1 = unsafe { File::from_raw_fd(DEST_FD1) };
- check_file2 = unsafe { File::from_raw_fd(DEST_FD2) };
+ // Safe because we are re-taking ownership of remapped fds after forking.
+ unsafe {
+ check_file1.into_raw_fd();
+ check_file1 = File::from_raw_fd(dest_fd1);
+
+ check_file2.into_raw_fd();
+ check_file2 = File::from_raw_fd(dest_fd2);
+ }
for (p, expected) in &[
(0, DEV_NULL),
- (1, DEV_NULL),
- (DEST_FD1, DEV_ZERO),
- (DEST_FD2, DEV_ZERO),
+ (1, &stdio_expected),
+ (2, &stdio_expected),
+ (dest_fd1, DEV_ZERO),
+ (dest_fd2, &file2_expected),
] {
let path = format!("/proc/self/fd/{}", p);
let target = read_link(Path::new(&path));
- eprintln!("C: {} -> {:?}", p, &target);
+ eprintln!(" C: {} -> {:?}", p, &target);
if !matches!(&target, Ok(p) if p == Path::new(expected)) {
- panic!("C: got {:?}; expected Ok({:?})", target, expected);
+ panic!(" C: got {:?}; expected Ok({:?})", target, expected);
}
}
@@ -77,5 +94,8 @@
check_file1.read_exact(&mut buffer).unwrap();
assert_eq!(&buffer, &[0u8; BUFFER_LEN]);
- eprintln!("Child done.");
+ let mut file2_contents = Vec::<u8>::new();
+ check_file2.read_to_end(&mut file2_contents).unwrap();
+
+ eprintln!(" Child done.");
}
