fix dangling fd with stdio

Signed-off-by: Jorge Prendes <[email protected]>

.github/workflows/action-build.yml

@@ -31,6 +31,7 @@ jobs:
       - name: Validate docs
         run: ./scripts/validate-docs.sh
       - name: Run tests
+        timeout-minutes: 5
         run: |
           make test
       - name: Package artifacts

.github/workflows/action-test-k3s.yml

@@ -15,11 +15,7 @@ jobs:
     name: e2e k3s test on ${{ inputs.os }}
     runs-on: ${{ inputs.os }}
     steps:
-      - name: "check cgroup version"
-        run: "mount | grep cgroup"
       - uses: actions/checkout@v3
-      - name: setup rust-wasm target
-        run: rustup target add wasm32-wasi
       - name: Setup build env
         run: ./scripts/setup-linux.sh
         shell: bash
@@ -39,6 +35,7 @@ jobs:
           name: test-image
           path: dist
       - name: run
+        timeout-minutes: 5
         run: make test/k3s
       - name: cleanup
         if: always()

.github/workflows/action-test-kind.yml

@@ -15,11 +15,7 @@ jobs:
     name: e2e kind test on ${{ inputs.os }}
     runs-on: ${{ inputs.os }}
     steps:
-      - name: "check cgroup version"
-        run: "mount | grep cgroup"
       - uses: actions/checkout@v3
-      - name: setup rust-wasm target
-        run: rustup target add wasm32-wasi
       - name: Setup build env
         run: ./scripts/setup-linux.sh
         shell: bash
@@ -39,6 +35,7 @@ jobs:
           name: test-image
           path: dist
       - name: run
+        timeout-minutes: 5
         run: make test/k8s
       # only runs when the previous step fails
       - name: inspect failed pods

.github/workflows/action-test-smoke.yml

@@ -0,0 +1,44 @@
+name: Run smoke tests
+
+on:
+  workflow_call:
+    inputs:
+      os:
+        required: true
+        type: string
+      runtime:
+        required: true
+        type: string
+
+jobs:
+  smoke-test:
+    name: smoke test on ${{ inputs.os }}
+    runs-on: ${{ inputs.os }}
+    steps:
+      - uses: actions/checkout@v3
+      - name: Setup build env
+        run: ./scripts/setup-linux.sh
+        shell: bash
+      - name: Download artifacts
+        uses: actions/download-artifact@master
+        with:
+          name: containerd-shim-${{ inputs.runtime }}-${{ inputs.os }}
+          path: dist
+      - name: Unpack artifats
+        shell: bash
+        run: |
+          mkdir -p dist/bin
+          tar -xzf dist/containerd-shim-${{ inputs.runtime }}-${{ inputs.os }}.tar.gz -C dist/bin
+      - name: Download test image
+        uses: actions/download-artifact@master
+        with:
+          name: test-image
+          path: dist
+      - name: run
+        timeout-minutes: 5
+        run: |
+          ls -alh dist
+          ls -alh dist/bin
+          make load
+          sudo cp -f dist/bin/* /usr/local/bin
+          sudo ctr run --rm --runtime=io.containerd.${{ inputs.runtime }}.v1 ghcr.io/containerd/runwasi/wasi-demo-app:latest testwasm /wasi-demo-app.wasm echo 'hello'

.github/workflows/ci.yml

@@ -49,6 +49,19 @@ jobs:
       os: ${{ matrix.os }}
       runtime: ${{ matrix.runtime }}
 
+  smoke-tests:
+    name: ${{ matrix.runtime }}
+    needs: [build-ubuntu, test-image]
+    strategy:
+      matrix:
+        # 20.04 uses cgroupv1, 22.04 uses cgroupv2
+        os: ["ubuntu-20.04", "ubuntu-22.04"]
+        runtime:  ["wasmtime", "wasmedge"]
+    uses: ./.github/workflows/action-test-smoke.yml
+    with:
+      os: ${{ matrix.os }}
+      runtime: ${{ matrix.runtime }}
+
   e2e-wasmtime:
     name: ${{ matrix.runtime }}
     needs: [build-ubuntu, test-image]

.github/workflows/release.yml

@@ -61,16 +61,14 @@ jobs:
       - uses: Swatinem/rust-cache@v2
         with:
           key: release-${{ needs.generate.outputs.crate }}
-      - name: "check cgroup version"
-        run: "mount | grep cgroup"
       - uses: actions/checkout@v3
+      - name: Setup build env
+        run: ./scripts/setup-linux.sh
+        shell: bash
       - name: Install rust
         uses: actions-rust-lang/setup-rust-toolchain@v1
         with:
           cache: false
-      - name: Setup build env
-        run: ${GITHUB_WORKSPACE}/.github/scripts/build.sh
-        shell: bash
       - name: Build
         run: cargo build --verbose --package ${{ needs.generate.outputs.crate }}
       - name: Test

Cargo.toml

@@ -36,6 +36,7 @@ sha256 = "1.4.0"
 # TODO: once lincontainer releases 0.2, switch to released version. The current commit is the tip of the tree from `youki` and a release candidate.
 libcontainer = { git = "https://github.com/containers/youki", rev = "09e67372a892f22a89eeef62ff429c3cbcac6d41", default-features = false }
 windows-sys = { version = "0.48" }
+crossbeam = { version = "0.8.2", default-features = false }
 
 [profile.release]
 panic = "abort"

Makefile

@@ -57,7 +57,7 @@ install:
 	)
 
 dist:
-	$(MAKE) install PREFIX=$(PWD)/dist RUNTIMES=$(RUNTIMES) TARGET=$(TARGET)
+	$(MAKE) install PREFIX="$(PWD)/dist" RUNTIMES="$(RUNTIMES)" TARGET="$(TARGET)"
 
 .PHONY: test-image
 test-image: dist/img.tar

crates/containerd-shim-wasm/Cargo.toml

@@ -23,6 +23,7 @@ ttrpc = { workspace = true }
 chrono = { workspace = true }
 log = { workspace = true }
 libc = { workspace = true }
+crossbeam = { workspace = true }
 
 [target.'cfg(unix)'.dependencies]
 clone3 = "0.2"

crates/containerd-shim-wasm/src/libcontainer_instance/container_executor.rs

@@ -25,7 +25,7 @@ impl LinuxContainerExecutor {
 
 impl Executor for LinuxContainerExecutor {
     fn exec(&self, spec: &Spec) -> Result<(), ExecutorError> {
-        self.stdio.redirect().map_err(|err| {
+        self.stdio.take().redirect().map_err(|err| {
             log::error!("failed to redirect io: {}", err);
             ExecutorError::Other(format!("failed to redirect io: {}", err))
         })?;

crates/containerd-shim-wasm/src/sandbox/stdio.rs

@@ -2,8 +2,11 @@ use std::fs::File;
 use std::io::ErrorKind::NotFound;
 use std::io::{Error, Result};
 use std::path::Path;
-use std::sync::{Arc, Mutex};
+use std::sync::Arc;
 
+use crossbeam::atomic::AtomicCell;
+
+use super::InstanceConfig;
 use crate::sys::stdio::*;
 
 #[derive(Default, Clone)]
@@ -14,51 +17,70 @@ pub struct Stdio {
 }
 
 impl Stdio {
-    pub fn redirect(&self) -> Result<()> {
+    pub fn redirect(self) -> Result<()> {
         self.stdin.redirect()?;
         self.stdout.redirect()?;
         self.stderr.redirect()?;
         Ok(())
     }
+
+    pub fn take(&self) -> Self {
+        Self {
+            stdin: self.stdin.take(),
+            stdout: self.stdout.take(),
+            stderr: self.stderr.take(),
+        }
+    }
+
+    pub fn init_from_cfg(cfg: &InstanceConfig<impl Send + Sync + Clone>) -> Result<Self> {
+        Ok(Self {
+            stdin: cfg.get_stdin().try_into()?,
+            stdout: cfg.get_stdout().try_into()?,
+            stderr: cfg.get_stderr().try_into()?,
+        })
+    }
 }
 
-macro_rules! stdio_impl {
-    ( $stdio_type:ident, $fd:expr ) => {
-        #[derive(Default, Clone)]
-        pub struct $stdio_type(Arc<Mutex<Option<File>>>);
+#[derive(Clone, Default)]
+pub struct StdioStream<const FD: StdioRawFd>(Arc<AtomicCell<Option<File>>>);
 
-        impl<P: AsRef<Path>> TryFrom<Option<P>> for $stdio_type {
-            type Error = std::io::Error;
-            fn try_from(path: Option<P>) -> Result<Self> {
-                path.and_then(|path| match path.as_ref() {
-                    path if path.as_os_str().is_empty() => None,
-                    path => Some(path.to_owned()),
-                })
-                .map(|path| match open_file(path) {
-                    Err(err) if err.kind() == NotFound => Ok(None),
-                    Ok(f) => Ok(Some(f)),
-                    Err(err) => Err(err),
-                })
-                .transpose()
-                .map(|opt| Self(Arc::new(Mutex::new(opt.flatten()))))
+impl<const FD: StdioRawFd> StdioStream<FD> {
+    pub fn redirect(self) -> Result<()> {
+        if let Some(f) = self.0.take() {
+            let f = try_into_fd(f)?;
+            let _ = unsafe { libc::dup(FD) };
+            if unsafe { libc::dup2(f.as_raw_fd(), FD) } == -1 {
+                return Err(Error::last_os_error());
             }
         }
+        Ok(())
+    }
 
-        impl $stdio_type {
-            pub fn redirect(&self) -> Result<()> {
-                if let Some(f) = self.0.try_lock().ok().and_then(|mut f| f.take()) {
-                    let f = try_into_fd(f)?;
-                    let _ = unsafe { libc::dup($fd) };
-                    if unsafe { libc::dup2(f.as_raw_fd(), $fd) } == -1 {
-                        return Err(Error::last_os_error());
-                    }
-                }
-                Ok(())
-            }
-        }
-    };
+    pub fn take(&self) -> Self {
+        Self(Arc::new(AtomicCell::new(self.0.take())))
+    }
+}
+
+impl<P: AsRef<Path>, const FD: StdioRawFd> TryFrom<Option<P>> for StdioStream<FD> {
+    type Error = Error;
+    fn try_from(path: Option<P>) -> Result<Self> {
+        let file = path
+            .and_then(|path| match path.as_ref() {
+                path if path.as_os_str().is_empty() => None,
+                path => Some(path.to_owned()),
+            })
+            .map(|path| match open_file(path) {
+                Err(err) if err.kind() == NotFound => Ok(None),
+                Ok(f) => Ok(Some(f)),
+                Err(err) => Err(err),
+            })
+            .transpose()?
+            .flatten();
+
+        Ok(Self(Arc::new(AtomicCell::new(file))))
+    }
 }
 
-stdio_impl!(Stdin, STDIN_FILENO);
-stdio_impl!(Stdout, STDOUT_FILENO);
-stdio_impl!(Stderr, STDERR_FILENO);
+pub type Stdin = StdioStream<STDIN_FILENO>;
+pub type Stdout = StdioStream<STDOUT_FILENO>;
+pub type Stderr = StdioStream<STDERR_FILENO>;

crates/containerd-shim-wasm/src/sys/unix/stdio.rs

@@ -1,7 +1,7 @@
 use std::fs::{File, OpenOptions};
 use std::io::Result;
-pub use std::os::fd::AsRawFd as StdioAsRawFd;
 use std::os::fd::OwnedFd;
+pub use std::os::fd::{AsRawFd as StdioAsRawFd, RawFd as StdioRawFd};
 use std::path::Path;
 
 pub use libc::{STDERR_FILENO, STDIN_FILENO, STDOUT_FILENO};

crates/containerd-shim-wasm/src/sys/windows/stdio.rs

@@ -8,11 +8,11 @@ use std::path::Path;
 use libc::{c_int, close, intptr_t, open_osfhandle, O_APPEND};
 use windows_sys::Win32::Storage::FileSystem::FILE_FLAG_OVERLAPPED;
 
-type StdioRawFd = libc::c_int;
+pub type StdioRawFd = libc::c_int;
 
-pub static STDIN_FILENO: StdioRawFd = 0;
-pub static STDOUT_FILENO: StdioRawFd = 1;
-pub static STDERR_FILENO: StdioRawFd = 2;
+pub const STDIN_FILENO: StdioRawFd = 0;
+pub const STDOUT_FILENO: StdioRawFd = 1;
+pub const STDERR_FILENO: StdioRawFd = 2;
 
 struct StdioOwnedFd(c_int);
 

crates/containerd-shim-wasmedge/src/executor.rs

@@ -96,7 +96,7 @@ impl WasmEdgeExecutor {
             .register_module_from_file("main", module_name)
             .map_err(|err| ExecutorError::Execution(err))?;
 
-        self.stdio.redirect()?;
+        self.stdio.take().redirect()?;
 
         Ok(vm)
     }

crates/containerd-shim-wasmedge/src/instance/instance_linux.rs

@@ -39,11 +39,7 @@ impl LibcontainerInstance for Wasi {
             )
             .unwrap(),
             exit_code: Arc::new((Mutex::new(None), Condvar::new())),
-            stdio: Stdio {
-                stdin: cfg.get_stdin().try_into().unwrap(),
-                stdout: cfg.get_stdout().try_into().unwrap(),
-                stderr: cfg.get_stderr().try_into().unwrap(),
-            },
+            stdio: Stdio::init_from_cfg(cfg).expect("failed to open stdio"),
             bundle,
         }
     }
@@ -65,7 +61,7 @@ impl LibcontainerInstance for Wasi {
 
         let err_others = |err| Error::Others(format!("failed to create container: {}", err));
         let container = ContainerBuilder::new(self.id.clone(), SyscallType::Linux)
-            .with_executor(WasmEdgeExecutor::new(self.stdio.clone()))
+            .with_executor(WasmEdgeExecutor::new(self.stdio.take()))
             .with_root_path(self.rootdir.clone())
             .map_err(err_others)?
             .as_init(&self.bundle)