Kernel: Fix wait syscall to report status of exited children

kernel/include/kernel/Process.h

@@ -219,8 +219,6 @@ namespace Kernel
 		// Load elf from a file
 		static BAN::ErrorOr<BAN::UniqPtr<LibELF::LoadableELF>> load_elf_for_exec(const Credentials&, BAN::StringView file_path, const BAN::String& cwd, Kernel::PageTable&);
 
-		BAN::ErrorOr<int> block_until_exit(pid_t pid);
-
 		BAN::ErrorOr<void> validate_string_access(const char*);
 		BAN::ErrorOr<void> validate_pointer_access_check(const void*, size_t);
 		BAN::ErrorOr<void> validate_pointer_access(const void*, size_t);
@@ -255,12 +253,12 @@ namespace Kernel
 		}
 
 	private:
-		struct ExitStatus
+		struct ChildExitStatus
 		{
-			ThreadBlocker		thread_blocker;
+			pid_t    pid       { 0 };
-			int					exit_code	{ 0 };
+			pid_t    pgrp      { 0 };
-			BAN::Atomic<bool>	exited		{ false };
+			int      exit_code { 0 };
-			BAN::Atomic<int>	waiting		{ 0 };
+			bool     exited    { false };
 		};
 
 		Credentials m_credentials;
@@ -292,7 +290,10 @@ namespace Kernel
 
 		bool m_is_userspace { false };
 		userspace_info_t m_userspace_info;
-		ExitStatus m_exit_status;
+
+		SpinLock m_child_exit_lock;
+		BAN::Vector<ChildExitStatus> m_child_exit_statuses;
+		ThreadBlocker m_child_exit_blocker;
 
 		bool m_has_called_exec { false };
 

kernel/include/kernel/Thread.h

@@ -49,6 +49,8 @@ namespace Kernel
 		bool add_signal(int signal);
 
 		// blocks current thread and returns either on unblock, eintr, spuriously or after timeout
+		BAN::ErrorOr<void> sleep_or_eintr_ms(uint64_t ms) { return sleep_or_eintr_ns(ms * 1'000'000); }
+		BAN::ErrorOr<void> sleep_or_eintr_ns(uint64_t ns);
 		BAN::ErrorOr<void> block_or_eintr_indefinite(ThreadBlocker& thread_blocker);
 		BAN::ErrorOr<void> block_or_eintr_or_timeout_ms(ThreadBlocker& thread_blocker, uint64_t timeout_ms, bool etimedout) { return block_or_eintr_or_timeout_ns(thread_blocker, timeout_ms * 1'000'000, etimedout); }
 		BAN::ErrorOr<void> block_or_eintr_or_waketime_ms(ThreadBlocker& thread_blocker, uint64_t wake_time_ms, bool etimedout) { return block_or_eintr_or_waketime_ns(thread_blocker, wake_time_ms * 1'000'000, etimedout); }

kernel/kernel/Process.cpp

@@ -185,7 +185,6 @@ namespace Kernel
 		ASSERT(m_threads.empty());
 		ASSERT(m_mapped_regions.empty());
 		ASSERT(!m_loadable_elf);
-		ASSERT(m_exit_status.waiting == 0);
 		ASSERT(&PageTable::current() != m_page_table.ptr());
 	}
 
@@ -201,21 +200,15 @@ namespace Kernel
 			SpinLockGuard _(s_process_lock);
 			for (size_t i = 0; i < s_processes.size(); i++)
 			{
-				if (m_parent && s_processes[i]->pid() == m_parent)
+				if (s_processes[i] != this)
-					s_processes[i]->add_pending_signal(SIGCHLD);
+					continue;
-				if (s_processes[i] == this)
+				s_processes.remove(i);
-					s_processes.remove(i);
+				break;
 			}
 		}
 
 		ProcFileSystem::get().on_process_delete(*this);
 
-		m_exit_status.exited = true;
-		m_exit_status.thread_blocker.unblock();
-
-		while (m_exit_status.waiting > 0)
-			Processor::yield();
-
 		m_process_lock.lock();
 
 		m_open_file_descriptors.close_all();
@@ -252,7 +245,35 @@ namespace Kernel
 
 	void Process::exit(int status, int signal)
 	{
-		m_exit_status.exit_code = __WGENEXITCODE(status, signal);
+		if (m_parent)
+		{
+			for_each_process(
+				[&](Process& parent) -> BAN::Iteration
+				{
+					if (parent.pid() != m_parent)
+						return BAN::Iteration::Continue;
+
+					for (auto& child : parent.m_child_exit_statuses)
+					{
+						if (child.pid != pid())
+							continue;
+
+						child.exit_code = __WGENEXITCODE(status, signal);
+						child.exited = true;
+
+						parent.add_pending_signal(SIGCHLD);
+						Processor::scheduler().unblock_thread(parent.m_threads.front());
+
+						parent.m_child_exit_blocker.unblock();
+
+						break;
+					}
+
+					return BAN::Iteration::Break;
+				}
+			);
+		}
+
 		while (!m_threads.empty())
 			m_threads.front()->on_exit();
 	}
@@ -397,6 +418,20 @@ namespace Kernel
 
 		LockGuard _(m_process_lock);
 
+		ChildExitStatus* child_exit_status = nullptr;
+		for (auto& child : m_child_exit_statuses)
+		{
+			if (child.pid != 0)
+				continue;
+			child_exit_status = &child;
+			break;
+		}
+		if (child_exit_status == nullptr)
+		{
+			TRY(m_child_exit_statuses.emplace_back());
+			child_exit_status = &m_child_exit_statuses.back();
+		}
+
 		BAN::String working_directory;
 		TRY(working_directory.append(m_working_directory));
 
@@ -422,6 +457,10 @@ namespace Kernel
 		forked->m_has_called_exec = false;
 		memcpy(forked->m_signal_handlers, m_signal_handlers, sizeof(m_signal_handlers));
 
+		*child_exit_status = {};
+		child_exit_status->pid = forked->pid();
+		child_exit_status->pgrp = forked->pgrp();
+
 		ASSERT(this == &Process::current());
 		// FIXME: this should be able to fail
 		Thread* thread = MUST(Thread::current().clone(forked, sp, ip));
@@ -541,58 +580,72 @@ namespace Kernel
 		ASSERT_NOT_REACHED();
 	}
 
-	BAN::ErrorOr<int> Process::block_until_exit(pid_t pid)
-	{
-		ASSERT(this->pid() != pid);
-
-		Process* target = nullptr;
-		for_each_process(
-			[pid, &target](Process& process)
-			{
-				if (process.pid() == pid)
-				{
-					process.m_exit_status.waiting++;
-					target = &process;
-					return BAN::Iteration::Break;
-				}
-				return BAN::Iteration::Continue;
-			}
-		);
-
-		if (target == nullptr)
-			return BAN::Error::from_errno(ECHILD);
-
-		while (!target->m_exit_status.exited)
-		{
-			if (auto ret = Thread::current().block_or_eintr_indefinite(target->m_exit_status.thread_blocker); ret.is_error())
-			{
-				target->m_exit_status.waiting--;
-				return ret.release_error();
-			}
-		}
-
-		int exit_status = target->m_exit_status.exit_code;
-		target->m_exit_status.waiting--;
-
-		return exit_status;
-	}
-
 	BAN::ErrorOr<long> Process::sys_wait(pid_t pid, int* stat_loc, int options)
 	{
-		{
+		if (options & ~(WCONTINUED | WNOHANG | WUNTRACED))
-			LockGuard _(m_process_lock);
-			TRY(validate_pointer_access(stat_loc, sizeof(int)));
-		}
-
-		// FIXME: support options
-		if (options)
 			return BAN::Error::from_errno(EINVAL);
 
-		int stat = TRY(block_until_exit(pid));
+		// FIXME: support options stopped processes
-		if (stat_loc)
+		if (options & ~(WCONTINUED | WUNTRACED))
-			*stat_loc = stat;
+			return BAN::Error::from_errno(ENOTSUP);
 
-		return pid;
+		const auto pid_matches =
+			[&](const ChildExitStatus& child)
+			{
+				if (pid == -1)
+					return true;
+				if (pid == 0)
+					return child.pgrp == pgrp();
+				if (pid < 0)
+					return child.pgrp == -pid;
+				return child.pid == pid;
+			};
+
+		for (;;)
+		{
+			pid_t exited_pid = 0;
+			int exit_code = 0;
+			{
+				SpinLockGuard _(m_child_exit_lock);
+
+				bool found = false;
+				for (auto& child : m_child_exit_statuses)
+				{
+					if (!pid_matches(child))
+						continue;
+					found = true;
+					if (!child.exited)
+						continue;
+					exited_pid = child.pid;
+					exit_code = child.exit_code;
+					child = {};
+					break;
+				}
+
+				if (!found)
+					return BAN::Error::from_errno(ECHILD);
+			}
+
+			if (exited_pid != 0)
+			{
+				if (stat_loc)
+				{
+					LockGuard _(m_process_lock);
+					TRY(validate_pointer_access(stat_loc, sizeof(stat_loc)));
+					*stat_loc = exit_code;
+				}
+				remove_pending_signal(SIGCHLD);
+				return exited_pid;
+			}
+
+			if (Thread::current().is_interrupted_by_signal())
+				return BAN::Error::from_errno(EINTR);
+
+			if (options & WNOHANG)
+				return 0;
+
+			m_child_exit_blocker.block_indefinite();
+		}
 	}
 
 	BAN::ErrorOr<long> Process::sys_sleep(int seconds)
@@ -1289,7 +1342,7 @@ namespace Kernel
 				break;
 
 			LockFreeGuard free(m_process_lock);
-			SystemTimer::get().sleep_ms(1);
+			TRY(Thread::current().sleep_or_eintr_ms(1));
 		}
 
 		if (arguments->readfds)

kernel/kernel/Thread.cpp

@@ -423,6 +423,16 @@ namespace Kernel
 		return false;
 	}
 
+	BAN::ErrorOr<void> Thread::sleep_or_eintr_ns(uint64_t ns)
+	{
+		if (is_interrupted_by_signal())
+			return BAN::Error::from_errno(EINTR);
+		SystemTimer::get().sleep_ns(ns);
+		if (is_interrupted_by_signal())
+			return BAN::Error::from_errno(EINTR);
+		return {};
+	}
+
 	BAN::ErrorOr<void> Thread::block_or_eintr_indefinite(ThreadBlocker& thread_blocker)
 	{
 		if (is_interrupted_by_signal())

userspace/programs/Shell/main.cpp

@@ -147,7 +147,7 @@ BAN::Optional<BAN::String> parse_dollar(BAN::StringView command, size_t& i)
 		close(fds[0]);
 
 		int status;
-		if (waitpid(pid, &status, 0) == -1 && errno != ECHILD)
+		if (waitpid(pid, &status, 0) == -1)
 			ERROR_RETURN("waitpid", {});
 
 		while (!output.empty() && output.back() == '\n')