Refactor and test get{pgid,sid}

kernel/src/process/process/mod.rs

@@ -350,10 +350,21 @@ impl Process {
     }
 
     /// Returns the process group ID of the process.
+    //
+    // FIXME: If we call this method without holding the process table lock, it may return zero if
+    // the process is reaped at the same time.
     pub fn pgid(&self) -> Pgid {
         self.process_group().map_or(0, |group| group.pgid())
     }
 
+    /// Returns the session ID of the process.
+    //
+    // FIXME: If we call this method without holding the process table lock, it may return zero if
+    // the process is reaped at the same time.
+    pub fn sid(&self) -> Sid {
+        self.session().map_or(0, |session| session.sid())
+    }
+
     /// Returns the session to which the process belongs.
     pub fn session(&self) -> Option<Arc<Session>> {
         self.process_group()?.session()

kernel/src/syscall/getpgid.rs

@@ -8,25 +8,24 @@ use crate::{
 
 pub fn sys_getpgid(pid: Pid, ctx: &Context) -> Result<SyscallReturn> {
     debug!("pid = {}", pid);
-    // type Pid = u32, pid would never less than 0.
-    // if pid < 0 {
-    //     return_errno_with_message!(Errno::EINVAL, "pid cannot be negative");
-    // }
 
-    // if pid is 0, should return the pgid of current process
+    // The documentation quoted below is from
+    // <https://www.man7.org/linux/man-pages/man2/getpgid.2.html>.
+
+    // "If `pid` is equal to 0, getpgid() shall return the process group ID of the calling
+    // process."
     if pid == 0 {
         return Ok(SyscallReturn::Return(ctx.process.pgid() as _));
     }
 
-    let process = process_table::get_process(pid)
+    let process = process_table::get_process(pid).ok_or(Error::with_message(
-        .ok_or(Error::with_message(Errno::ESRCH, "process does not exist"))?;
+        Errno::ESRCH,
+        "the process to get the PGID does not exist",
+    ))?;
 
-    if !Arc::ptr_eq(&ctx.process.session().unwrap(), &process.session().unwrap()) {
+    // The man pages allow the implementation to return `EPERM` if `process` is in a different
-        return_errno_with_message!(
+    // session than the current process. Linux does not perform this check by default, but some
-            Errno::EPERM,
+    // strict security policies (e.g. SELinux) may do so.
-            "the process and current process does not belong to the same session"
-        );
-    }
 
     Ok(SyscallReturn::Return(process.pgid() as _))
 }

kernel/src/syscall/getsid.rs

@@ -9,23 +9,22 @@ use crate::{
 pub fn sys_getsid(pid: Pid, ctx: &Context) -> Result<SyscallReturn> {
     debug!("pid = {}", pid);
 
-    let session = ctx.process.session().unwrap();
+    // The documentation quoted below is from
-    let sid = session.sid();
+    // <https://www.man7.org/linux/man-pages/man2/getsid.2.html>.
 
+    // "If `pid` is 0, getsid() returns the session ID of the calling process."
     if pid == 0 {
-        return Ok(SyscallReturn::Return(sid as _));
+        return Ok(SyscallReturn::Return(ctx.process.sid() as _));
     }
 
-    let Some(process) = process_table::get_process(pid) else {
+    let process = process_table::get_process(pid).ok_or(Error::with_message(
-        return_errno_with_message!(Errno::ESRCH, "the process does not exist")
+        Errno::ESRCH,
-    };
+        "the process to get the SID does not exist",
+    ))?;
 
-    if !Arc::ptr_eq(&session, &process.session().unwrap()) {
+    // The man pages allow the implementation to return `EPERM` if `process` is in a different
-        return_errno_with_message!(
+    // session than the current process. Linux does not perform this check by default, but some
-            Errno::EPERM,
+    // strict security policies (e.g. SELinux) may do so.
-            "the process and current process does not belong to the same session"
-        );
-    }
 
-    Ok(SyscallReturn::Return(sid as _))
+    Ok(SyscallReturn::Return(process.sid() as _))
 }

test/apps/process/group_session.c

@@ -48,7 +48,7 @@ FN_TEST(setpgid_invalid)
 }
 END_TEST()
 
-FN_TEST(setpgid)
+FN_TEST(setpgid_getpgid)
 {
 	//                   PGID       members
 	//                     |           |
@@ -63,17 +63,37 @@ FN_TEST(setpgid)
 	TEST_ERRNO(setpgid(child1, child2), EPERM);
 	TEST_ERRNO(setpgid(child2, child1), EPERM);
 
+	TEST_RES(getpgid(current), _ret == current);
+	TEST_RES(getpgid(child1), _ret == current);
+	TEST_RES(getpgid(child2), _ret == current);
+
 	// Process groups: [current] = { current, child2 }, [child1] = { child1 }
 	TEST_SUCC(setpgid(child1, 0));
 
+	TEST_RES(getpgid(current), _ret == current);
+	TEST_RES(getpgid(child1), _ret == child1);
+	TEST_RES(getpgid(child2), _ret == current);
+
 	// Process groups: [current] = { current }, [child1] = { child1, child2 }
 	TEST_SUCC(setpgid(child2, child1));
 
+	TEST_RES(getpgid(current), _ret == current);
+	TEST_RES(getpgid(child1), _ret == child1);
+	TEST_RES(getpgid(child2), _ret == child1);
+
 	// Process groups: [current] = { current, child1 }, [child1] = { child2 }
 	TEST_SUCC(setpgid(child1, current));
 
+	TEST_RES(getpgid(current), _ret == current);
+	TEST_RES(getpgid(child1), _ret == current);
+	TEST_RES(getpgid(child2), _ret == child1);
+
 	// Process groups: [current] = { current }, [child1] = { child1, child2 }
 	TEST_SUCC(setpgid(child1, child1));
+
+	TEST_RES(getpgid(current), _ret == current);
+	TEST_RES(getpgid(child1), _ret == child1);
+	TEST_RES(getpgid(child2), _ret == child1);
 }
 END_TEST()
 
@@ -87,6 +107,10 @@ FN_TEST(setsid_group_leader)
 	TEST_SUCC(setpgid(child1, current));
 	TEST_SUCC(setpgid(current, child1));
 
+	TEST_RES(getpgid(current), _ret == child1);
+	TEST_RES(getpgid(child1), _ret == current);
+	TEST_RES(getpgid(child2), _ret == child1);
+
 	TEST_ERRNO(setsid(), EPERM);
 }
 END_TEST()
@@ -96,6 +120,10 @@ FN_TEST(setsid)
 	// Process groups: [child1] = { current, child1, child2 }
 	TEST_SUCC(setpgid(child1, child1));
 
+	TEST_RES(getpgid(current), _ret == child1);
+	TEST_RES(getpgid(child1), _ret == child1);
+	TEST_RES(getpgid(child2), _ret == child1);
+
 	// Process groups (old session): [child1] = { child1, child2 }
 	// Process groups (new session): [current] = { current }
 	TEST_SUCC(setsid());
@@ -127,6 +155,39 @@ FN_TEST(setpgid_two_sessions)
 }
 END_TEST()
 
+FN_TEST(getpgid_two_sessions)
+{
+	TEST_RES(getpgid(current), _ret == current);
+	TEST_RES(getpgid(child1), _ret == child1);
+	TEST_RES(getpgid(child2), _ret == child1);
+}
+END_TEST()
+
+FN_TEST(getsid_two_sessions)
+{
+	int old_sid;
+
+	TEST_RES(getsid(current), _ret == current);
+
+	old_sid = TEST_SUCC(getsid(child1));
+	TEST_RES(getsid(child2), _ret == old_sid);
+}
+END_TEST()
+
+FN_TEST(getpgid_invalid)
+{
+	// Non-present processes
+	TEST_ERRNO(getpgid(0x3c3c3c3c), ESRCH);
+}
+END_TEST()
+
+FN_TEST(getsid_invalid)
+{
+	// Non-present processes
+	TEST_ERRNO(getsid(0x3c3c3c3c), ESRCH);
+}
+END_TEST()
+
 FN_SETUP(kill_child)
 {
 	CHECK(kill(child1, SIGKILL));