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 end to end tests on k3s
+
+on:
+  workflow_call:
+    inputs:
+      os:
+        required: true
+        type: string
+      runtime:
+        required: true
+        type: string
+
+jobs:
+  e2e-k3s:
+    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

@@ -35,6 +35,7 @@ oci-spec = { version = "0.6.1", features = ["runtime"] }
 sha256 = "1.4.0"
 libcontainer = { version = "0.1", 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/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)]
@@ -20,45 +23,77 @@ impl Stdio {
         self.stderr.redirect()?;
         Ok(())
     }
+
+    pub fn take(&self) -> Self {
+        Self {
+            stdin: self.stdin.take(),
+            stdout: self.stdout.take(),
+            stderr: self.stderr.take(),
+        }
+    }
+}
+
+impl<Engine: Send + Sync + Clone> TryFrom<&InstanceConfig<Engine>> for Stdio {
+    type Error = Error;
+    fn try_from(cfg: &InstanceConfig<Engine>) -> std::result::Result<Self, Self::Error> {
+        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(Default)]
+pub struct StdioStream<const FD: StdioRawFd>(Arc<AtomicCell<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> Clone for StdioStream<FD> {
+    fn clone(&self) -> Self {
+        let f = self.0.take();
+        self.0.store(f.clone());
+        Self(Arc::new(AtomicCell::new(f)))
+    }
+}
 
-        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(())
+impl<const FD: StdioRawFd> StdioStream<FD> {
+    pub fn redirect(&self) -> Result<()> {
+        if let Some(f) = self.0.take().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 = std::io::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(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/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: cfg.try_into().expect("failed to open stdio"),
             bundle,
         }
     }
@@ -66,7 +62,7 @@ impl LibcontainerInstance for Wasi {
         let syscall = create_syscall();
         let err_others = |err| Error::Others(format!("failed to create container: {}", err));
         let default_executor = Box::new(LinuxContainerExecutor::new(self.stdio.clone()));
-        let wasmedge_executor = Box::new(WasmEdgeExecutor::new(self.stdio.clone()));
+        let wasmedge_executor = Box::new(WasmEdgeExecutor::new(self.stdio.take()));
 
         let container = ContainerBuilder::new(self.id.clone(), syscall.as_ref())
             .with_executor(vec![default_executor, wasmedge_executor])

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

@@ -1,7 +1,7 @@
 use std::fs::OpenOptions;
 use std::path::PathBuf;
 
-use anyhow::{anyhow, Result};
+use anyhow::{anyhow, Context, Result};
 use containerd_shim_wasm::sandbox::{oci, Stdio};
 use libcontainer::workload::{Executor, ExecutorError};
 use oci_spec::runtime::Spec;
@@ -93,16 +93,10 @@ impl WasmtimeExecutor {
 
         log::info!("wasi context ready");
         let (module_name, method) = oci::get_module(spec);
-        let module_name = match module_name {
-            Some(m) => m,
-            None => {
-                return Err(anyhow::format_err!(
-                    "no module provided, cannot load module from file within container"
-                ))
-            }
-        };
+        let module_name = module_name
+            .context("no module provided, cannot load module from file within container")?;
 
-        log::info!("loading module from file {} ", module_name);
+        log::info!("loading module from file {}", module_name);
         let module = Module::from_file(&self.engine, module_name)?;
         let mut linker = Linker::new(&self.engine);