From 35787be29db574c22ceb7fd9f78500f80f0c1dfb Mon Sep 17 00:00:00 2001
From: "Tate, Hongliang Tian" <tate.thl@antfin.com>
Date: Thu, 2 Jul 2020 09:59:33 +0000
Subject: [PATCH] Fix two bugs on process exit

The first bug is a race condition when acquiring the lock of a process's
parent. An example code with race condition looks like below:

```rust
let process : ProessRef = current!().process();
let parent : ProcessRef = process.parent();
let parent_guard : SgxMutexGuard<ProesssInner> = parent.inner();
// This assertion may fail because the process's parent may change to another
// process before the lock is acquired
assert!(parent.pid() == process.parent().pid());
```

The second bug is that when a process exits, its children processes are
not transfered to the idle process correctly.
---
 src/libos/src/process/do_exit.rs     | 46 ++++++++++++++++++++--------
 src/libos/src/process/process/mod.rs | 16 ++++++++--
 2 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/src/libos/src/process/do_exit.rs b/src/libos/src/process/do_exit.rs
index f56da2cd..2bebbc38 100644
--- a/src/libos/src/process/do_exit.rs
+++ b/src/libos/src/process/do_exit.rs
@@ -61,29 +61,49 @@ fn exit_thread(term_status: TermStatus) {
 fn exit_process(thread: &ThreadRef, term_status: TermStatus) {
     let process = thread.process();
 
-    // If the parent process is the idle process, we can release the process directly.
-    if process.parent().pid() == 0 {
-        // Deadlock note: Always lock parent then child.
-        let mut parent_inner = super::IDLE.process().inner();
-        let mut process_inner = process.inner();
+    // Deadlock note: always lock parent first, then child.
+
+    // Lock the idle process since it may adopt new children.
+    let idle_ref = super::IDLE.process().clone();
+    let mut idle_inner = idle_ref.inner();
+    // Lock the parent process as we want to prevent race conditions between
+    // current's exit() and parent's wait5().
+    let mut parent;
+    let mut parent_inner = loop {
+        parent = process.parent();
+        if parent.pid() == 0 {
+            // If the parent is the idle process, don't need to lock again
+            break None;
+        }
+
+        let parent_inner = parent.inner();
+        // To prevent the race condition that parent is changed after `parent()`,
+        // but before `parent().innner()`, we need to check again here.
+        if parent.pid() != process.parent().pid() {
+            continue;
+        }
+        break Some(parent_inner);
+    };
+    // Lock the current process
+    let mut process_inner = process.inner();
+
+    // The parent is the idle process
+    if parent_inner.is_none() {
+        debug_assert!(parent.pid() == 0);
 
         let pid = process.pid();
         let main_tid = pid;
         table::del_thread(main_tid).expect("tid must be in the table");
         table::del_process(pid).expect("pid must be in the table");
 
-        process_inner.exit(term_status);
-        parent_inner.remove_zombie_child(process.pid());
+        process_inner.exit(term_status, &idle_ref, &mut idle_inner);
+        idle_inner.remove_zombie_child(pid);
         return;
     }
     // Otherwise, we need to notify the parent process
+    let mut parent_inner = parent_inner.unwrap();
 
-    // Lock the parent process to ensure that parent's wait4 cannot miss the current
-    // process's exit.
-    // Deadlock note: Always lock parent then child.
-    let parent = process.parent();
-    let mut parent_inner = parent.inner();
-    process.inner().exit(term_status);
+    process_inner.exit(term_status, &idle_ref, &mut idle_inner);
 
     //Send SIGCHLD to parent
     send_sigchld_to(&parent);
diff --git a/src/libos/src/process/process/mod.rs b/src/libos/src/process/process/mod.rs
index b410c9b8..64ffab2d 100644
--- a/src/libos/src/process/process/mod.rs
+++ b/src/libos/src/process/process/mod.rs
@@ -233,15 +233,27 @@ impl ProcessInner {
         children.swap_remove(zombie_i)
     }
 
-    pub fn exit(&mut self, term_status: TermStatus) {
+    /// Exit means two things: 1) transfer all children to a new parent; 2) update the status.
+    ///
+    /// A lock guard for the new parent process is passed so that the transfer can be done
+    /// atomically.
+    pub fn exit(
+        &mut self,
+        term_status: TermStatus,
+        new_parent_ref: &ProcessRef,
+        new_parent_inner: &mut SgxMutexGuard<ProcessInner>,
+    ) {
         // Check preconditions
         debug_assert!(self.status() == ProcessStatus::Running);
         debug_assert!(self.num_threads() == 0);
 
         // When this process exits, its children are adopted by the init process
         for child in self.children().unwrap() {
+            let child_inner = child.inner();
             let mut parent = child.parent.as_ref().unwrap().write().unwrap();
-            *parent = IDLE.process().clone();
+            *parent = new_parent_ref.clone();
+
+            new_parent_inner.children_mut().unwrap().push(child.clone());
         }
 
         *self = Self::Zombie { term_status };