Reduce unsafe code

Author: MikailBag

.github/workflows/rust.yml

@@ -63,7 +63,18 @@ jobs:
           cargo build -Zunstable-options --out-dir=../out --target=$TARGET
       - name: Run tests
         run: |
-          sudo ./out/minion-tests
+          sudo ./out/minion-tests --trace
+        timeout-minutes: 3 
+      - name: Collect logs
+        if: always()
+        run: |
+          mkdir /tmp/logs
+          cp ./strace* /tmp/logs
+      - uses: actions/upload-artifact@v1
+        if: always()
+        with:
+          path: /tmp/logs
+          name: tests-trace
   nightly-checks:
     runs-on: ubuntu-latest
     steps:

minion-ffi/src/lib.rs

@@ -40,9 +40,11 @@ pub enum WaitOutcome {
     Timeout,
 }
 
+/// # Safety
+/// `buf` must be valid, readable pointer
 unsafe fn get_string(buf: *const c_char) -> OsString {
     use std::os::unix::ffi::OsStrExt;
-    let buf = CStr::from_ptr(buf);
+    let buf = unsafe { CStr::from_ptr(buf) };
     let buf = buf.to_bytes();
     let s = OsStr::from_bytes(buf);
     s.to_os_string()
@@ -79,7 +81,7 @@ pub extern "C" fn minion_backend_create(out: &mut *mut Backend) -> ErrorCode {
 #[no_mangle]
 #[must_use]
 pub unsafe extern "C" fn minion_backend_free(b: *mut Backend) -> ErrorCode {
-    let b = Box::from_raw(b);
+    let b = unsafe { Box::from_raw(b) };
     mem::drop(b);
     ErrorCode::Ok
 }
@@ -112,7 +114,9 @@ pub unsafe extern "C" fn minion_dominion_check_cpu_tle(
 ) -> ErrorCode {
     match dominion.0.check_cpu_tle() {
         Ok(st) => {
-            out.write(st);
+            unsafe {
+                out.write(st);
+            }
             ErrorCode::Ok
         }
         Err(_) => ErrorCode::Unknown,
@@ -128,7 +132,9 @@ pub unsafe extern "C" fn minion_dominion_check_real_tle(
 ) -> ErrorCode {
     match dominion.0.check_real_tle() {
         Ok(st) => {
-            out.write(st);
+            unsafe {
+                out.write(st);
+            }
             ErrorCode::Ok
         }
         Err(_) => ErrorCode::Unknown,
@@ -153,7 +159,7 @@ pub unsafe extern "C" fn minion_dominion_create(
     out: &mut *mut Dominion,
 ) -> ErrorCode {
     let mut exposed_paths = Vec::new();
-    {
+    unsafe {
         let mut p = options.shared_directories;
         while !(*p).host_path.is_null() {
             let opt = minion::PathExpositionOptions {
@@ -168,6 +174,7 @@ pub unsafe extern "C" fn minion_dominion_create(
             p = p.offset(1);
         }
     }
+    let isolation_root = unsafe { get_string(options.isolation_root) }.into();
     let opts = minion::DominionOptions {
         max_alive_process_count: options.process_limit as _,
         memory_limit: u64::from(options.memory_limit),
@@ -179,7 +186,7 @@ pub unsafe extern "C" fn minion_dominion_create(
             options.real_time_limit.seconds.into(),
             options.real_time_limit.nanoseconds,
         ),
-        isolation_root: get_string(options.isolation_root).into(),
+        isolation_root,
         exposed_paths,
     };
     let d = backend.0.new_dominion(opts);
@@ -195,7 +202,7 @@ pub unsafe extern "C" fn minion_dominion_create(
 #[no_mangle]
 #[must_use]
 pub unsafe extern "C" fn minion_dominion_free(dominion: *mut Dominion) -> ErrorCode {
-    let b = Box::from_raw(dominion);
+    let b = unsafe { Box::from_raw(dominion) };
     mem::drop(b);
     ErrorCode::Ok
 }
@@ -274,15 +281,15 @@ pub unsafe extern "C" fn minion_cp_spawn(
     out: &mut *mut ChildProcess,
 ) -> ErrorCode {
     let mut arguments = Vec::new();
-    {
+    unsafe {
         let mut p = options.argv;
         while !(*p).is_null() {
             arguments.push(get_string(*p));
             p = p.offset(1);
         }
     }
     let mut environment = Vec::new();
-    {
+    unsafe {
         let mut p = options.envp;
         while !(*p).name.is_null() {
             let name = get_string((*p).name);
@@ -295,18 +302,22 @@ pub unsafe extern "C" fn minion_cp_spawn(
             p = p.offset(1);
         }
     }
-    let stdio = minion::StdioSpecification {
-        stdin: minion::InputSpecification::handle(options.stdio.stdin),
-        stdout: minion::OutputSpecification::handle(options.stdio.stdout),
-        stderr: minion::OutputSpecification::handle(options.stdio.stderr),
+    let stdio = unsafe {
+        minion::StdioSpecification {
+            stdin: minion::InputSpecification::handle(options.stdio.stdin),
+            stdout: minion::OutputSpecification::handle(options.stdio.stdout),
+            stderr: minion::OutputSpecification::handle(options.stdio.stderr),
+        }
     };
-    let options = minion::ChildProcessOptions {
-        path: get_string(options.image_path).into(),
-        arguments,
-        environment,
-        dominion: (*options.dominion).0.clone(),
-        stdio,
-        pwd: get_string(options.workdir).into(),
+    let options = unsafe {
+        minion::ChildProcessOptions {
+            path: get_string(options.image_path).into(),
+            arguments,
+            environment,
+            dominion: (*options.dominion).0.clone(),
+            stdio,
+            pwd: get_string(options.workdir).into(),
+        }
     };
     let cp = backend.0.spawn(options).unwrap();
     let cp = ChildProcess(cp);
@@ -336,7 +347,9 @@ pub unsafe extern "C" fn minion_cp_wait(
                 minion::WaitOutcome::AlreadyFinished => WaitOutcome::AlreadyFinished,
                 minion::WaitOutcome::Timeout => WaitOutcome::Timeout,
             };
-            out.write(outcome);
+            unsafe {
+                out.write(outcome);
+            }
             ErrorCode::Ok
         }
         Result::Err(_) => ErrorCode::Unknown,
@@ -346,24 +359,33 @@ pub unsafe extern "C" fn minion_cp_wait(
 #[no_mangle]
 pub static EXIT_CODE_STILL_RUNNING: i64 = 1234_4321;
 
+/// Returns child process (pointed by `cp`) exit code.
+///
+/// `out` will contain exit code. If child is still running,
+/// `out` will not be written to.
+///
+/// if `finish_flag` is non-null it will be written 0/1 flag:
+/// has child finished.
 /// # Safety
 /// Provided pointers must be valid
 #[no_mangle]
 #[must_use]
 pub unsafe extern "C" fn minion_cp_exitcode(
-    cp: &mut ChildProcess,
+    cp: &ChildProcess,
     out: *mut i64,
-    finish_flag: *mut bool,
+    finish_flag: *mut u8,
 ) -> ErrorCode {
     match cp.0.get_exit_code() {
         Result::Ok(exit_code) => {
             if let Some(code) = exit_code {
-                out.write(code);
+                unsafe {
+                    out.write(code);
+                }
             } else {
-                out.write(EXIT_CODE_STILL_RUNNING)
+                unsafe { out.write(EXIT_CODE_STILL_RUNNING) }
             }
             if !finish_flag.is_null() {
-                finish_flag.write(exit_code.is_some());
+                unsafe { finish_flag.write(exit_code.is_some() as u8) };
             }
             ErrorCode::Ok
         }
@@ -376,6 +398,6 @@ pub unsafe extern "C" fn minion_cp_exitcode(
 #[no_mangle]
 #[must_use]
 pub unsafe extern "C" fn minion_cp_free(cp: *mut ChildProcess) -> ErrorCode {
-    mem::drop(Box::from_raw(cp));
+    mem::drop(unsafe { Box::from_raw(cp) });
     ErrorCode::Ok
 }

src/lib.rs

@@ -213,7 +213,7 @@ impl InputSpecification {
     /// # Safety
     /// See requirements of `handle`
     pub unsafe fn handle_of<T: std::os::unix::io::IntoRawFd>(obj: T) -> Self {
-        Self::handle(obj.into_raw_fd() as u64)
+        unsafe { Self::handle(obj.into_raw_fd() as u64) }
     }
 }
 
@@ -258,7 +258,7 @@ impl OutputSpecification {
     /// # Safety
     /// See requirements of `handle`
     pub unsafe fn handle_of<T: std::os::unix::io::IntoRawFd>(obj: T) -> Self {
-        Self::handle(obj.into_raw_fd() as u64)
+        unsafe { Self::handle(obj.into_raw_fd() as u64) }
     }
 }
 

src/linux/dominion.rs

@@ -2,7 +2,7 @@ use crate::{
     linux::{
         jail_common,
         pipe::setup_pipe,
-        util::{err_exit, ExitCode, Handle, IpcSocketExt, Pid},
+        util::{ExitCode, Handle, IpcSocketExt, Pid},
         zygote,
     },
     Dominion, DominionOptions,
@@ -164,9 +164,11 @@ impl LinuxDominion {
         let mut read_end = 0;
         let mut write_end = 0;
         setup_pipe(&mut read_end, &mut write_end)?;
-        if -1 == libc::fcntl(read_end, libc::F_SETFL, libc::O_NONBLOCK) {
-            err_exit("fcntl");
-        }
+        nix::fcntl::fcntl(
+            read_end,
+            nix::fcntl::FcntlArg::F_SETFL(nix::fcntl::OFlag::O_NONBLOCK),
+        )?;
+
         let jail_options = jail_common::JailOptions {
             max_alive_process_count: options.max_alive_process_count,
             memory_limit: options.memory_limit,

src/linux/util.rs

@@ -7,6 +7,7 @@ use std::{
 use tiny_nix_ipc::{self, Socket};
 
 pub type Handle = RawFd;
+
 pub type Pid = libc::pid_t;
 pub type ExitCode = i64;
 pub type Uid = libc::uid_t;
@@ -27,7 +28,7 @@ pub fn err_exit(syscall_name: &str) -> ! {
     }
 }
 
-unsafe fn sock_lock(sock: &mut Socket, expected_class: &'static [u8]) -> crate::Result<()> {
+fn sock_lock(sock: &mut Socket, expected_class: &'static [u8]) -> crate::Result<()> {
     use std::io::Write;
     let mut logger = strace_logger();
     let mut recv_buf = vec![0; expected_class.len()];
@@ -51,41 +52,41 @@ unsafe fn sock_lock(sock: &mut Socket, expected_class: &'static [u8]) -> crate::
     Ok(())
 }
 
-unsafe fn sock_wake(sock: &mut Socket, wake_class: &'static [u8]) -> crate::Result<()> {
+fn sock_wake(sock: &mut Socket, wake_class: &'static [u8]) -> crate::Result<()> {
     match sock.send_slice(&wake_class, None) {
         Ok(_) => Ok(()),
         Err(_) => Err(crate::Error::Sandbox),
     }
 }
 
 pub trait IpcSocketExt {
-    unsafe fn lock(&mut self, expected_class: &'static [u8]) -> crate::Result<()>;
-    unsafe fn wake(&mut self, wake_class: &'static [u8]) -> crate::Result<()>;
+    fn lock(&mut self, expected_class: &'static [u8]) -> crate::Result<()>;
+    fn wake(&mut self, wake_class: &'static [u8]) -> crate::Result<()>;
 
-    unsafe fn send<T: serde::ser::Serialize>(&mut self, data: &T) -> crate::Result<()>;
-    unsafe fn recv<T: serde::de::DeserializeOwned>(&mut self) -> crate::Result<T>;
+    fn send<T: serde::ser::Serialize>(&mut self, data: &T) -> crate::Result<()>;
+    fn recv<T: serde::de::DeserializeOwned>(&mut self) -> crate::Result<T>;
 }
 
 const MAX_MSG_SIZE: usize = 16384;
 
 impl IpcSocketExt for Socket {
-    unsafe fn lock(&mut self, expected_class: &'static [u8]) -> crate::Result<()> {
+    fn lock(&mut self, expected_class: &'static [u8]) -> crate::Result<()> {
         sock_lock(self, expected_class)
     }
 
-    unsafe fn wake(&mut self, wake_class: &'static [u8]) -> crate::Result<()> {
+    fn wake(&mut self, wake_class: &'static [u8]) -> crate::Result<()> {
         sock_wake(self, wake_class)
     }
 
-    unsafe fn send<T: serde::ser::Serialize>(&mut self, data: &T) -> crate::Result<()> {
+    fn send<T: serde::ser::Serialize>(&mut self, data: &T) -> crate::Result<()> {
         let data = serde_json::to_vec(data).unwrap();
         assert!(data.len() <= MAX_MSG_SIZE);
         self.send_slice(&data, None)
             .map(|_num_written| ())
             .map_err(|_e| crate::errors::Error::Sandbox)
     }
 
-    unsafe fn recv<T: serde::de::DeserializeOwned>(&mut self) -> crate::Result<T> {
+    fn recv<T: serde::de::DeserializeOwned>(&mut self) -> crate::Result<T> {
         use std::io::Write;
         let mut logger = StraceLogger::new();
         let mut buf = vec![0; MAX_MSG_SIZE];