Write integration tests and resolve issues found

.github/workflows/rust.yml → .github/workflows/tests.yml

@@ -1,16 +1,14 @@
-name: Rust
+name: Tests
 
 on: [push, pull_request]
 
 env:
   CARGO_TERM_COLOR: always
 
 jobs:
-  build:
-
+  lints:
     runs-on: ubuntu-latest
     timeout-minutes: 10
-
     steps:
     - uses: actions/checkout@v2
       with:
@@ -57,12 +55,27 @@ jobs:
 
         # Run clippy check
         cargo clippy
+
+  tests:
+    runs-on: ubuntu-latest
+    timeout-minutes: 10
+    steps:
+    - uses: actions/checkout@v2
     - name: Build
       run: cargo build --verbose
-    - name: Run tests
+    - name: Setup rootfs
       shell: bash
       run: |
-        # Setup rootfs
         bash scripts/mkrootfs.sh
-        # Run cargo test
+    - name: Run unit tests
+      shell: bash
+      run: |
         PROOT_TEST_ROOTFS=./rootfs cargo test --verbose -- --nocapture
+    - name: Setup bats-core
+      uses: mig4/setup-bats@v1
+      with:
+        bats-version: 1.3.0
+    - name: Run integration tests
+      shell: bash
+      run: |
+        PROOT_TEST_ROOTFS=./rootfs bats -r tests

Dockerfile

@@ -2,6 +2,7 @@ FROM rust:alpine as build
 
 RUN apk update && \
     apk add bash \
+            bats \
             curl \
             shellcheck
 

README.md

@@ -1,6 +1,6 @@
 # proot-rs
 
-[![](https://github.com/proot-me/proot-rs/workflows/Rust/badge.svg)](https://github.com/proot-me/proot-rs/actions)
+[![Tests](https://github.com/proot-me/proot-rs/actions/workflows/tests.yml/badge.svg)](https://github.com/proot-me/proot-rs/actions/workflows/tests.yml)
 
 _Rust implementation of PRoot, a ptrace-based sandbox._
 
@@ -65,6 +65,8 @@ cargo build --release
 
 ## Tests
 
+### Setup new rootfs for testing
+
 Typically, we need to specify a new rootfs path for testing proot-rs.
 
 This script provided below can be used to create one:
@@ -86,12 +88,20 @@ If you want to use the same rootfs as the host, just set it to `/`:
 export PROOT_TEST_ROOTFS=/
 ```
 
+### Unit testing
+
+> Note: When running unit tests, it is required that `PROOT_TEST_ROOTFS` must be set
+
 Start running tests:
 
 ```shell
 cargo test
 ```
 
+### Integration testing
+
+For the section on running integration tests, please read the [Integration Testing documentation](./tests/README.md)
+
 ## Contributing
 
 We use git hooks to check files staged for commit to ensure the consistency of Rust code style.

docker-compose.yml

@@ -1,10 +1,17 @@
 version: "3"
 services:
-  proot-rs:
-    container_name: proot-rs
-    hostname: proot-rs
+  proot-rs-sdk:
+    container_name: proot-rs-sdk
+    hostname: proot-rs-sdk
     build: .
-    image: proot/proot-rs:latest
+    image: proot/proot-rs-sdk:latest
+    volumes:
+      - ~/src/proot-rs/:/usr/src/proot-rs
+  proot-rs-test:
+    container_name: proot-rs-test
+    hostname: proot-rs-test
+    build: ./tests
+    image: proot/proot-rs-test:latest
     volumes:
       - ~/src/proot-rs/:/usr/src/proot-rs
 

scripts/mkrootfs.sh

@@ -41,7 +41,7 @@ trap 'rm -f "${rootfs_archive}"' EXIT
 
 rootfs_archive="$(mktemp)" || { echo "Failed to create temp file"; exit 1; }
 
-curl -o "${rootfs_archive}" -L -C - "https://github.com/docker-library/busybox/raw/dist-${arch}/stable/glibc/busybox.tar.xz" || { echo "Failed to download busybox archive"; exit 1; }
+wget -O "${rootfs_archive}" "https://github.com/docker-library/busybox/raw/dist-${arch}/stable/glibc/busybox.tar.xz" || { echo "Failed to download busybox archive"; exit 1; }
 
 tar -C "${PROOT_TEST_ROOTFS}" -xf "${rootfs_archive}" || { echo "Failed to unpack busybox tarball. Maybe the file is broken"; exit 1; }
 

src/build_loader.rs

@@ -1,10 +1,42 @@
 extern crate gcc;
 
+#[cfg(target_arch = "x86")]
+mod config {
+    pub const LOADER_ADDRESS: u64 = 0xa0000000;
+    pub const LOADER_ARCH_CFLAGS: &[&'static str] = &["-mregparm=3"];
+}
+
+#[cfg(target_arch = "x86_64")]
+mod arch {
+    /// The virtual address(p_vaddr) of `.text` section of our custom loader.
+    pub const LOADER_ADDRESS: u64 = 0x600000000000;
+    /// Additional flags to be passed when compiling the loader.
+    pub const LOADER_ARCH_CFLAGS: &[&'static str] = &[];
+}
+
+#[cfg(target_arch = "arm")]
+mod arch {
+    pub const LOADER_ADDRESS: u64 = 0x10000000;
+    pub const LOADER_ARCH_CFLAGS: &[&'static str] = &[];
+}
+
+#[cfg(target_arch = "aarch64")]
+mod arch {
+    pub const LOADER_ADDRESS: u64 = 0x2000000000;
+    pub const LOADER_ARCH_CFLAGS: &[&'static str] = &[];
+}
+
 fn main() {
-    gcc::Config::new()
+    let mut config = gcc::Config::new();
+    config
         .flag("-static")
         .flag("-nostdlib")
-        .flag("-ffreestanding")
+        .flag("-ffreestanding");
+    arch::LOADER_ARCH_CFLAGS.iter().for_each(|flag| {
+        config.flag(flag);
+    });
+    config.flag(&format!("-Wl,-Ttext=0x{:x}", arch::LOADER_ADDRESS));
+    config
         .file("src/kernel/execve/loader/loader.c")
         .out_dir("src/kernel/execve/loader")
         .compile_binary("binary_loader_exe");

src/filesystem/binding.rs

@@ -18,7 +18,6 @@ impl Side {
     }
 }
 
-// TODO: Maybe we should canonicalize guest path during initialization
 #[derive(Debug, Clone)]
 pub struct Binding {
     /// Host side path of this binding in canonical form.
@@ -78,7 +77,16 @@ impl Binding {
         })?;
 
         // and then add what remains of the path when removing the old prefix
-        new_path.push(stripped_path);
+        if !stripped_path.is_empty() {
+            // If the `stripped_path` is empty, we will not call `.push("")`, to avoid
+            // adding the extra "/" at the end of the path.
+            //
+            // Note: As mentioned in the document of `std::path::PathBuf::components()`, "A
+            // trailing slash is normalized away" in a path. And it means `foo/bar` is the
+            // same as `foo/bar/` . However, many Linux system call are sensitive to
+            // trailing slash, and they assume a path with a trailing slash as a directory.
+            new_path.push(stripped_path);
+        }
 
         if new_path.len() >= PATH_MAX as usize {
             return Err(Error::errno_with_msg(

src/filesystem/canonicalization.rs

@@ -1,9 +1,9 @@
+use std::path::{Component, Path, PathBuf};
+
 use crate::errors::*;
+use crate::filesystem::binding::Side;
 use crate::filesystem::substitution::Substitutor;
 use crate::filesystem::FileSystem;
-use std::path::{Component, Path, PathBuf};
-
-use super::binding::Side;
 
 pub trait Canonicalizer {
     fn canonicalize<P: AsRef<Path>>(&self, path: P, deref_final: bool) -> Result<PathBuf>;
@@ -92,16 +92,20 @@ impl Canonicalizer for FileSystem {
                     let host_path = self.substitute(&guest_path_new, Side::Guest)?;
 
                     let metadata = host_path.symlink_metadata();
-                    // `metadata` is error if we cannot access this file or file is not exist.
-                    // However, we can accept this path because some syscall (e.g. mkdir, mknod)
-                    // allow final component not exist.
-                    if is_last_component && metadata.is_err() {
-                        continue;
-                    }
-                    // We can continue if we are now on the last component and are explicitly asked
-                    // not to dereference 'user_path'.
-                    if is_last_component && !deref_final {
-                        continue;
+
+                    if is_last_component {
+                        // `metadata` is error if we cannot access this file or file is not exist.
+                        // However, we can accept this path because some syscall (e.g. mkdir, mknod)
+                        // allow final component not exist.
+                        if metadata.is_err() {
+                            continue;
+                        }
+
+                        // We can continue if we are now on the last component and are explicitly
+                        // asked not to dereference 'user_path'.
+                        if !deref_final {
+                            continue;
+                        }
                     }
 
                     let file_type = metadata?.file_type();
@@ -129,7 +133,7 @@ impl Canonicalizer for FileSystem {
                         if let Some(comp) = next_comp {
                             new_user_path.push(comp);
                         }
-                        it.for_each(|comp| new_user_path.push(comp));
+                        new_user_path.push(it);
                         // use new_user_path to call this function again and return
                         // TODO: Can be optimized by replacing `it`
                         return self.canonicalize(&new_user_path, deref_final);
@@ -151,11 +155,14 @@ impl Canonicalizer for FileSystem {
 
 #[cfg(test)]
 mod tests {
+    use std::path::PathBuf;
+
+    use nix::sys::stat::Mode;
+
     use super::*;
+    use crate::filesystem::ext::PathExt;
     use crate::filesystem::FileSystem;
     use crate::utils::tests::get_test_rootfs_path;
-    use nix::sys::stat::Mode;
-    use std::path::PathBuf;
 
     #[test]
     fn test_canonicalize_invalid_path() {
@@ -264,4 +271,21 @@ mod tests {
             PathBuf::from("/lib")
         );
     }
+
+    #[test]
+    fn test_canonicalize_trailing_slash() {
+        let fs = FileSystem::with_root(get_test_rootfs_path()).unwrap();
+
+        let path = fs.canonicalize(&PathBuf::from("/lib64"), true).unwrap();
+        assert!(!path.with_trailing_slash());
+
+        let path = fs.canonicalize(&PathBuf::from("/lib64/"), true).unwrap();
+        assert!(!path.with_trailing_slash());
+
+        let path = fs.canonicalize(&PathBuf::from("/lib64"), false).unwrap();
+        assert!(!path.with_trailing_slash());
+
+        let path = fs.canonicalize(&PathBuf::from("/lib64/"), false).unwrap();
+        assert!(!path.with_trailing_slash());
+    }
 }