From d4b60ae69abb3336dec488a2d9657607d9342611 Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Tue, 29 Aug 2023 10:49:40 +0100 Subject: [PATCH] fix dangling fd with stdio Signed-off-by: Jorge Prendes --- .github/workflows/action-build.yml | 1 + .github/workflows/action-test-k3s.yml | 5 +- .github/workflows/action-test-kind.yml | 5 +- .github/workflows/action-test-smoke.yml | 44 +++++++++ .github/workflows/ci.yml | 13 +++ .github/workflows/release.yml | 8 +- Cargo.lock | 11 +++ Cargo.toml | 1 + Makefile | 2 +- crates/containerd-shim-wasm/Cargo.toml | 1 + .../container_executor.rs | 2 +- .../containerd-shim-wasm/src/sandbox/stdio.rs | 94 ++++++++++++------- .../src/sys/unix/stdio.rs | 2 +- .../src/sys/windows/stdio.rs | 8 +- .../containerd-shim-wasmedge/src/executor.rs | 2 +- .../src/instance/instance_linux.rs | 8 +- .../containerd-shim-wasmtime/src/executor.rs | 18 ++-- .../src/instance/instance_linux.rs | 9 +- 18 files changed, 152 insertions(+), 82 deletions(-) create mode 100644 .github/workflows/action-test-smoke.yml diff --git a/.github/workflows/action-build.yml b/.github/workflows/action-build.yml index 3e1b58269..72a4e711d 100644 --- a/.github/workflows/action-build.yml +++ b/.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 diff --git a/.github/workflows/action-test-k3s.yml b/.github/workflows/action-test-k3s.yml index 8eddf5369..7383b83ca 100644 --- a/.github/workflows/action-test-k3s.yml +++ b/.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() diff --git a/.github/workflows/action-test-kind.yml b/.github/workflows/action-test-kind.yml index d30a7cf2d..790709c62 100644 --- a/.github/workflows/action-test-kind.yml +++ b/.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 diff --git a/.github/workflows/action-test-smoke.yml b/.github/workflows/action-test-smoke.yml new file mode 100644 index 000000000..ec80af802 --- /dev/null +++ b/.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' diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3ea5bad88..bba063c51 100644 --- a/.github/workflows/ci.yml +++ b/.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] diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index ff5c51944..5dcb989be 100644 --- a/.github/workflows/release.yml +++ b/.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 diff --git a/Cargo.lock b/Cargo.lock index d035b9b0f..9a373f0af 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -528,6 +528,7 @@ dependencies = [ "clone3", "command-fds", "containerd-shim", + "crossbeam", "libc", "libcontainer", "log", @@ -734,6 +735,16 @@ dependencies = [ "cfg-if 1.0.0", ] +[[package]] +name = "crossbeam" +version = "0.8.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2801af0d36612ae591caa9568261fddce32ce6e08a7275ea334a06a4ad021a2c" +dependencies = [ + "cfg-if 1.0.0", + "crossbeam-utils", +] + [[package]] name = "crossbeam-channel" version = "0.5.8" diff --git a/Cargo.toml b/Cargo.toml index bf9adea04..3c8130b9d 100644 --- a/Cargo.toml +++ b/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" diff --git a/Makefile b/Makefile index 27754e565..e5ba0e580 100644 --- a/Makefile +++ b/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 diff --git a/crates/containerd-shim-wasm/Cargo.toml b/crates/containerd-shim-wasm/Cargo.toml index 216367660..2b5fcc757 100644 --- a/crates/containerd-shim-wasm/Cargo.toml +++ b/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" diff --git a/crates/containerd-shim-wasm/src/libcontainer_instance/container_executor.rs b/crates/containerd-shim-wasm/src/libcontainer_instance/container_executor.rs index 81c999aff..07d8a6642 100644 --- a/crates/containerd-shim-wasm/src/libcontainer_instance/container_executor.rs +++ b/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)) })?; diff --git a/crates/containerd-shim-wasm/src/sandbox/stdio.rs b/crates/containerd-shim-wasm/src/sandbox/stdio.rs index 434fdece3..9354f3375 100644 --- a/crates/containerd-shim-wasm/src/sandbox/stdio.rs +++ b/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) -> Result { + 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>>); +#[derive(Clone, Default)] +pub struct StdioStream(Arc>>); - impl> TryFrom> for $stdio_type { - type Error = std::io::Error; - fn try_from(path: Option

) -> Result { - 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 StdioStream { + 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, const FD: StdioRawFd> TryFrom> for StdioStream { + type Error = Error; + fn try_from(path: Option

) -> Result { + 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; +pub type Stdout = StdioStream; +pub type Stderr = StdioStream; diff --git a/crates/containerd-shim-wasm/src/sys/unix/stdio.rs b/crates/containerd-shim-wasm/src/sys/unix/stdio.rs index 2cdc507c6..3bd0ae0f9 100644 --- a/crates/containerd-shim-wasm/src/sys/unix/stdio.rs +++ b/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}; diff --git a/crates/containerd-shim-wasm/src/sys/windows/stdio.rs b/crates/containerd-shim-wasm/src/sys/windows/stdio.rs index f40a8d02a..1c92d1c71 100644 --- a/crates/containerd-shim-wasm/src/sys/windows/stdio.rs +++ b/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); diff --git a/crates/containerd-shim-wasmedge/src/executor.rs b/crates/containerd-shim-wasmedge/src/executor.rs index 25636d190..4d8f30a4a 100644 --- a/crates/containerd-shim-wasmedge/src/executor.rs +++ b/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) } diff --git a/crates/containerd-shim-wasmedge/src/instance/instance_linux.rs b/crates/containerd-shim-wasmedge/src/instance/instance_linux.rs index 60c928e78..53f9035c9 100644 --- a/crates/containerd-shim-wasmedge/src/instance/instance_linux.rs +++ b/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) diff --git a/crates/containerd-shim-wasmtime/src/executor.rs b/crates/containerd-shim-wasmtime/src/executor.rs index 4fe71b351..b2740aa16 100644 --- a/crates/containerd-shim-wasmtime/src/executor.rs +++ b/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::{Context, Result}; use containerd_shim_wasm::libcontainer_instance::LinuxContainerExecutor; use containerd_shim_wasm::sandbox::{oci, Stdio}; use libcontainer::workload::{Executor, ExecutorError, ExecutorValidationError}; @@ -83,23 +83,17 @@ impl WasmtimeExecutor { .inherit_stdio() .preopened_dir(path, "/")?; - self.stdio.redirect()?; + self.stdio.take().redirect()?; log::info!("building wasi context"); let wctx = wasi_builder.build(); 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); @@ -112,7 +106,7 @@ impl WasmtimeExecutor { log::info!("getting start function"); let start_func = instance .get_func(&mut store, &method) - .ok_or_else(|| anyhow!("module does not have a WASI start function".to_string()))?; + .context("module does not have a WASI start function")?; Ok((store, start_func)) } } diff --git a/crates/containerd-shim-wasmtime/src/instance/instance_linux.rs b/crates/containerd-shim-wasmtime/src/instance/instance_linux.rs index 01df4f6e8..0285eea6e 100644 --- a/crates/containerd-shim-wasmtime/src/instance/instance_linux.rs +++ b/crates/containerd-shim-wasmtime/src/instance/instance_linux.rs @@ -43,11 +43,7 @@ impl LibcontainerInstance for Wasi { id, exit_code: Arc::new((Mutex::new(None), Condvar::new())), engine: cfg.get_engine(), - 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, rootdir, } @@ -67,11 +63,10 @@ impl LibcontainerInstance for Wasi { fn build_container(&self) -> std::result::Result { let engine = self.engine.clone(); - self.stdio.redirect()?; let err_others = |err| Error::Others(format!("failed to create container: {}", err)); let container = ContainerBuilder::new(self.id.clone(), SyscallType::Linux) - .with_executor(WasmtimeExecutor::new(self.stdio.clone(), engine)) + .with_executor(WasmtimeExecutor::new(self.stdio.take(), engine)) .with_root_path(self.rootdir.clone()) .map_err(err_others)? .as_init(&self.bundle)