runc-shim: fix leaking thread and fd

The stdin fifo fd should be closed when containerd
calls close io, otherwise, runc-shim would hold
this fd and the copy console thread forever.

Signed-off-by: Zhang Tianyang <burning9699@gmail.com>

crates/runc-shim/src/asynchronous/runc.rs

@@ -22,7 +22,7 @@ use std::{
     },
     path::{Path, PathBuf},
     process::ExitStatus,
-    sync::Arc,
+    sync::{Arc, Mutex},
 };
 
 use async_trait::async_trait;
@@ -241,6 +241,7 @@ impl ProcessFactory<ExecProcess> for RuncExecFactory {
                 spec: p,
                 exit_signal: Default::default(),
             }),
+            stdin: Arc::new(Mutex::new(None)),
         })
     }
 }
@@ -402,6 +403,24 @@ impl ProcessLifecycle<ExecProcess> for RuncExecLifecycle {
             }
             return Err(runtime_error(&bundle, e, "OCI runtime exec failed").await);
         }
+
+        if !p.stdio.stdin.is_empty() {
+            let stdin_clone = p.stdio.stdin.clone();
+            let stdin_w = p.stdin.clone();
+            // Open the write side in advance to make sure read side will not block,
+            // open it in another thread otherwise it will block too.
+            tokio::spawn(async move {
+                if let Ok(stdin_w_file) = OpenOptions::new()
+                    .write(true)
+                    .open(stdin_clone.as_str())
+                    .await
+                {
+                    let mut lock_guard = stdin_w.lock().unwrap();
+                    *lock_guard = Some(stdin_w_file);
+                }
+            });
+        }
+
         copy_io_or_console(p, socket, pio, p.lifecycle.exit_signal.clone()).await?;
         let pid = read_file_to_str(pid_path).await?.parse::<i32>()?;
         p.pid = pid;
@@ -466,28 +485,12 @@ async fn copy_console(
             .try_clone()
             .await
             .map_err(io_error!(e, "failed to clone console file"))?;
-        let stdin_fut = async {
+        let stdin = OpenOptions::new()
-            OpenOptions::new()
+            .read(true)
-                .read(true)
+            .open(stdio.stdin.as_str())
-                .open(stdio.stdin.as_str())
+            .await
-                .await
+            .map_err(io_error!(e, "failed to open stdin"))?;
-        };
+        spawn_copy(stdin, console_stdin, exit_signal.clone(), None::<fn()>);
-        let stdin_w_fut = async {
-            OpenOptions::new()
-                .write(true)
-                .open(stdio.stdin.as_str())
-                .await
-        };
-        let (stdin, stdin_w) =
-            tokio::try_join!(stdin_fut, stdin_w_fut).map_err(io_error!(e, "open stdin"))?;
-        spawn_copy(
-            stdin,
-            console_stdin,
-            exit_signal.clone(),
-            Some(move || {
-                drop(stdin_w);
-            }),
-        );
     }
 
     if !stdio.stdout.is_empty() {

crates/runc-shim/src/synchronous/container.rs

@@ -20,7 +20,10 @@ use std::{
     fs::{File, OpenOptions},
     os::unix::io::{AsRawFd, FromRawFd},
     path::Path,
-    sync::mpsc::{sync_channel, Receiver, SyncSender},
+    sync::{
+        mpsc::{sync_channel, Receiver, SyncSender},
+        Arc, Mutex,
+    },
 };
 
 use containerd_shim as shim;
@@ -65,6 +68,7 @@ pub trait Process {
     fn copy_io(&self) -> Result<()>;
     fn set_pid_from_file(&mut self, pid_path: &Path) -> Result<()>;
     fn resize_pty(&mut self, height: u32, width: u32) -> Result<()>;
+    fn close_io(&self) -> Result<()>;
 }
 
 pub trait Container {
@@ -81,6 +85,7 @@ pub trait Container {
     fn update(&mut self, resources: &LinuxResources) -> Result<()>;
     fn pids(&self) -> Result<PidsResponse>;
     fn id(&self) -> String;
+    fn close_io(&mut self, exec_id: Option<&str>) -> Result<()>;
 }
 
 pub struct CommonContainer<T, E> {
@@ -178,6 +183,7 @@ pub struct CommonProcess {
     pub exited_at: Option<OffsetDateTime>,
     pub wait_chan_tx: Vec<SyncSender<i8>>,
     pub console: Option<Console>,
+    pub stdin: Arc<Mutex<Option<File>>>,
 }
 
 impl Process for CommonProcess {
@@ -260,7 +266,6 @@ impl Process for CommonProcess {
             let f = unsafe { File::from_raw_fd(fd) };
             let stdin = OpenOptions::new()
                 .read(true)
-                .write(true)
                 .open(self.stdio.stdin.as_str())
                 .map_err(io_error!(e, "open stdin"))?;
             spawn_copy_for_tty(stdin, f, None, None);
@@ -323,4 +328,12 @@ impl Process for CommonProcess {
             None => Err(other!("there is no console")),
         }
     }
+
+    fn close_io(&self) -> Result<()> {
+        let mut lock_guard = self.stdin.lock().unwrap();
+        if let Some(stdin_w) = lock_guard.take() {
+            drop(stdin_w);
+        }
+        Ok(())
+    }
 }

crates/runc-shim/src/synchronous/runc.rs

@@ -17,14 +17,14 @@
 
 use std::{
     convert::TryFrom,
-    fs::{remove_file, File},
+    fs::{remove_file, File, OpenOptions},
     io::{BufRead, BufReader, Read},
     os::unix::prelude::ExitStatusExt,
     path::{Path, PathBuf},
     process::ExitStatus,
     sync::{
         mpsc::{Receiver, SyncSender},
-        Arc,
+        Arc, Mutex,
     },
 };
 
@@ -203,7 +203,23 @@ impl Container for RuncContainer {
                     .init
                     .runtime
                     .exec(&id, &process.spec, Some(&exec_opts))
-                    .map_err(|e| runtime_error(&bundle, e, "OCI runtime exec failed"))?;
+                    .map_err(other_error!(e, "failed exec"))?;
+
+                if !process.common.stdio.stdin.is_empty() {
+                    let stdin_clone = process.common.stdio.stdin.clone();
+                    let stdin_w = process.common.stdin.clone();
+                    // Open the write side in advance to make sure read side will not block,
+                    // open it in another thread otherwise it will block too.
+                    std::thread::spawn(move || {
+                        if let Ok(stdin_w_file) =
+                            OpenOptions::new().write(true).open(stdin_clone.as_str())
+                        {
+                            let mut lock_guard = stdin_w.lock().unwrap();
+                            *lock_guard = Some(stdin_w_file);
+                        }
+                    });
+                }
+
                 if process.common.stdio.terminal {
                     let console_socket =
                         socket.ok_or_else(|| other!("failed to get console socket"))?;
@@ -368,6 +384,11 @@ impl Container for RuncContainer {
     fn id(&self) -> String {
         self.common.id.to_string()
     }
+
+    fn close_io(&mut self, exec_id: Option<&str>) -> Result<()> {
+        let p = self.common.get_mut_process(exec_id)?;
+        p.close_io()
+    }
 }
 
 impl RuncContainer {
@@ -424,6 +445,7 @@ impl InitProcess {
                 exited_at: None,
                 wait_chan_tx: vec![],
                 console: None,
+                stdin: Arc::new(Mutex::new(None)),
             },
             bundle: bundle.to_string(),
             runtime,
@@ -548,6 +570,10 @@ impl Process for InitProcess {
     fn resize_pty(&mut self, height: u32, width: u32) -> Result<()> {
         self.common.resize_pty(height, width)
     }
+
+    fn close_io(&self) -> Result<()> {
+        self.common.close_io()
+    }
 }
 
 pub(crate) struct ExecProcess {
@@ -623,6 +649,10 @@ impl Process for ExecProcess {
     fn resize_pty(&mut self, height: u32, width: u32) -> Result<()> {
         self.common.resize_pty(height, width)
     }
+
+    fn close_io(&self) -> Result<()> {
+        self.common.close_io()
+    }
 }
 
 impl TryFrom<ExecProcessRequest> for ExecProcess {
@@ -645,6 +675,7 @@ impl TryFrom<ExecProcessRequest> for ExecProcess {
                 exited_at: None,
                 wait_chan_tx: vec![],
                 console: None,
+                stdin: Arc::new(Mutex::new(None)),
             },
             spec: p,
         };

crates/runc-shim/src/synchronous/task.rs

@@ -262,8 +262,13 @@ where
         Ok(Empty::new())
     }
 
-    fn close_io(&self, _ctx: &TtrpcContext, _req: CloseIORequest) -> TtrpcResult<Empty> {
+    fn close_io(&self, _ctx: &TtrpcContext, req: CloseIORequest) -> TtrpcResult<Empty> {
-        // unnecessary close io here since fd was closed automatically after object was destroyed.
+        let mut containers = self.containers.lock().unwrap();
+        let container = containers
+            .get_mut(&req.id)
+            .ok_or_else(|| Error::Other(format!("can not find container by id {}", &req.id)))?;
+        let exec_id_opt = req.exec_id().none_if(|x| x.is_empty());
+        container.close_io(exec_id_opt)?;
         Ok(Empty::new())
     }
 

crates/shim/src/asynchronous/container.rs

@@ -49,6 +49,7 @@ pub trait Container {
     async fn update(&mut self, resources: &LinuxResources) -> Result<()>;
     async fn stats(&self) -> Result<Metrics>;
     async fn all_processes(&self) -> Result<Vec<ProcessInfo>>;
+    async fn close_io(&mut self, exec_id: Option<&str>) -> Result<()>;
 }
 
 #[async_trait]
@@ -182,6 +183,11 @@ where
     async fn all_processes(&self) -> Result<Vec<ProcessInfo>> {
         self.init.ps().await
     }
+
+    async fn close_io(&mut self, exec_id: Option<&str>) -> Result<()> {
+        let process = self.get_mut_process(exec_id)?;
+        process.close_io().await
+    }
 }
 
 impl<T, E, P> ContainerTemplate<T, E, P>

crates/shim/src/asynchronous/processes.rs

@@ -14,7 +14,10 @@
    limitations under the License.
 */
 
-use std::{os::unix::io::AsRawFd, sync::Arc};
+use std::{
+    os::unix::io::AsRawFd,
+    sync::{Arc, Mutex},
+};
 
 use async_trait::async_trait;
 use containerd_shim_protos::{
@@ -24,7 +27,10 @@ use containerd_shim_protos::{
 };
 use oci_spec::runtime::LinuxResources;
 use time::OffsetDateTime;
-use tokio::sync::oneshot::{channel, Receiver, Sender};
+use tokio::{
+    fs::File,
+    sync::oneshot::{channel, Receiver, Sender},
+};
 
 use crate::{io::Stdio, ioctl_set_winsz, util::asyncify, Console};
 
@@ -43,6 +49,7 @@ pub trait Process {
     async fn update(&mut self, resources: &LinuxResources) -> crate::Result<()>;
     async fn stats(&self) -> crate::Result<Metrics>;
     async fn ps(&self) -> crate::Result<Vec<ProcessInfo>>;
+    async fn close_io(&mut self) -> crate::Result<()>;
 }
 
 #[async_trait]
@@ -65,6 +72,7 @@ pub struct ProcessTemplate<S> {
     pub wait_chan_tx: Vec<Sender<()>>,
     pub console: Option<Console>,
     pub lifecycle: Arc<S>,
+    pub stdin: Arc<Mutex<Option<File>>>,
 }
 
 impl<S> ProcessTemplate<S> {
@@ -79,6 +87,7 @@ impl<S> ProcessTemplate<S> {
             wait_chan_tx: vec![],
             console: None,
             lifecycle: Arc::new(lifecycle),
+            stdin: Arc::new(Mutex::new(None)),
         }
     }
 }
@@ -176,4 +185,12 @@ where
     async fn ps(&self) -> crate::Result<Vec<ProcessInfo>> {
         self.lifecycle.ps(self).await
     }
+
+    async fn close_io(&mut self) -> crate::Result<()> {
+        let mut lock_guard = self.stdin.lock().unwrap();
+        if let Some(stdin_w_file) = lock_guard.take() {
+            drop(stdin_w_file);
+        }
+        Ok(())
+    }
 }

crates/shim/src/asynchronous/task.rs

@@ -269,8 +269,9 @@ where
         Ok(Empty::new())
     }
 
-    async fn close_io(&self, _ctx: &TtrpcContext, _req: CloseIORequest) -> TtrpcResult<Empty> {
+    async fn close_io(&self, _ctx: &TtrpcContext, req: CloseIORequest) -> TtrpcResult<Empty> {
-        // TODO call close_io of container
+        let mut container = self.get_container(req.id()).await?;
+        container.close_io(req.exec_id().as_option()).await?;
         Ok(Empty::new())
     }