From a993264265533bd9bb5a481d8611c64554ef61d7 Mon Sep 17 00:00:00 2001 From: Ruihan Li Date: Fri, 2 May 2025 22:43:16 +0800 Subject: [PATCH] Refactor and test `get{pgid,sid}` --- kernel/src/process/process/mod.rs | 11 ++++++ kernel/src/syscall/getpgid.rs | 25 ++++++------ kernel/src/syscall/getsid.rs | 25 ++++++------ test/apps/process/group_session.c | 63 ++++++++++++++++++++++++++++++- 4 files changed, 97 insertions(+), 27 deletions(-) diff --git a/kernel/src/process/process/mod.rs b/kernel/src/process/process/mod.rs index 8bb3ad41..313c8b6f 100644 --- a/kernel/src/process/process/mod.rs +++ b/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> { self.process_group()?.session() diff --git a/kernel/src/syscall/getpgid.rs b/kernel/src/syscall/getpgid.rs index baa581e2..7ba07cbe 100644 --- a/kernel/src/syscall/getpgid.rs +++ b/kernel/src/syscall/getpgid.rs @@ -8,25 +8,24 @@ use crate::{ pub fn sys_getpgid(pid: Pid, ctx: &Context) -> Result { 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 + // . + + // "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) - .ok_or(Error::with_message(Errno::ESRCH, "process does not exist"))?; + let process = process_table::get_process(pid).ok_or(Error::with_message( + Errno::ESRCH, + "the process to get the PGID does not exist", + ))?; - if !Arc::ptr_eq(&ctx.process.session().unwrap(), &process.session().unwrap()) { - return_errno_with_message!( - Errno::EPERM, - "the process and current process does not belong to the same session" - ); - } + // The man pages allow the implementation to return `EPERM` if `process` is in a different + // session than the current process. Linux does not perform this check by default, but some + // strict security policies (e.g. SELinux) may do so. Ok(SyscallReturn::Return(process.pgid() as _)) } diff --git a/kernel/src/syscall/getsid.rs b/kernel/src/syscall/getsid.rs index cb84525a..5faf1e5a 100644 --- a/kernel/src/syscall/getsid.rs +++ b/kernel/src/syscall/getsid.rs @@ -9,23 +9,22 @@ use crate::{ pub fn sys_getsid(pid: Pid, ctx: &Context) -> Result { debug!("pid = {}", pid); - let session = ctx.process.session().unwrap(); - let sid = session.sid(); + // The documentation quoted below is from + // . + // "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 { - return_errno_with_message!(Errno::ESRCH, "the process does not exist") - }; + let process = process_table::get_process(pid).ok_or(Error::with_message( + Errno::ESRCH, + "the process to get the SID does not exist", + ))?; - if !Arc::ptr_eq(&session, &process.session().unwrap()) { - return_errno_with_message!( - Errno::EPERM, - "the process and current process does not belong to the same session" - ); - } + // The man pages allow the implementation to return `EPERM` if `process` is in a different + // session than the current process. Linux does not perform this check by default, but some + // strict security policies (e.g. SELinux) may do so. - Ok(SyscallReturn::Return(sid as _)) + Ok(SyscallReturn::Return(process.sid() as _)) } diff --git a/test/apps/process/group_session.c b/test/apps/process/group_session.c index 875ea332..ec09027f 100644 --- a/test/apps/process/group_session.c +++ b/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));