Kernel+LibC: Implement posix_spawn file actions in kernel by firatkizilboga · Pull Request #26562 · SerenityOS/serenity

firatkizilboga · 2026-01-19T18:34:42Z

This commit implements the FIXME left by Tomás Simões to handle file actions directly in the kernel during posix_spawn, avoiding the expensive fork() path.

Changes include:

Define serialized file action structures in Kernel/API/POSIX/spawn.h
Serialize file actions to a ByteBuffer in LibC instead of lambdas
Add Process::execute_file_actions() to apply actions in the kernel
Update posix_spawn to pass serialized buffer to kernel syscall
Fall back to fork() only when spawnattr is present

Supported actions: addopen, addclose, adddup2, addchdir, addfchdir

Hendiadyoin1

Welcome to the Project, and thank you for looking into this

Here are a few nits on my side

Kernel/Syscalls/posix_spawn.cpp

Hendiadyoin1 · 2026-01-20T13:33:00Z

Tests/Kernel/TestPosixSpawn.cpp

In general, nice to have more test cases,
but I dont quite get the point of having self cleaning and non self cleaning variants of the tests

Could you clarify this comment? Are you referring to the two paths we are testing? If so, I wanted to test the 'old' path as well, since these changes affect it too.

Ah my bad, misread the branch and logic a bit
read it as a thing only getting destroyed in the slow path,
but its also only created in the slow path, so you can disregard the comment

It took me a while to realize this, but this is for testing the non-syscall version. The names use_slow_path and get_attr_for_path confused me a bit. Maybe inverting that condition and calling it use_syscall could be clearer? I'm also not sure why that function is called get_attr_for_path. There is no path parameter.

Also, we usually prefer to use enum classes over bools for parameters that are likely passed as constants (https://github.com/SerenityOS/serenity/blob/master/Documentation/CodingStyle.md#right-8). So I think having an enum

enum class UseSyscall { No, Yes, };

would be better here.

Additionally, I think there should be a FIXME comment somewhere noting that this non-syscall version should stopped being tested once we implement spawnattrs in the kernel (as setting spawnattrs would then not change anything).

Kernel/API/POSIX/spawn.h

firatkizilboga · 2026-01-20T19:21:04Z

Thanks for the warm welcome! This is my first significant contribution to a project of this size, so I really appreciate the detailed guidance. I hope I'm getting the workflow right!

I've just updated the PR with the changes you requested.

spholz · 2026-01-31T12:56:41Z

Please use git amend to modify your commits instead of making follow-up commits to address review comments or fix your PR in other ways.

To combine those two commits retroactively, you can do git rebase -i master.
That will cause an editor window to pop up. Go to the second line with the commit "Kernel+LibC: Fix posix_spawn file action alignment and fd handling" and change the "pick" into a "f" for "fixup". Then save the file and close the editor. After that, use git push --force-with-lease to push your changes. If you need help, feel free to ask on discord.

spholz · 2026-01-31T12:58:08Z

Also, it would be nice if you could split the PR up into smaller atomic commits that only do one change. So one or more for adding the basic infrastructure and then individual commits for each of the spawn actions.

Each commit should fully work though, so you likely need to make LibC continue to use the fork+exec impl when a specific file action isn't implemented yet.
If that's too much work, then sure I can still review it like this. We just usually prefer small atomic steps over big changes, which makes both reviewing and bisecting easier.

spholz

Still not a full review, it would really be helpful if you could split the changes into smaller steps.

Kernel/API/POSIX/spawn.h

Tests/Kernel/TestPosixSpawn.cpp

Add comprehensive tests for posix_spawn file actions including: - addopen, addclose, adddup2, addchdir, addfchdir - Both fast path (kernel) and slow path (fork) variants - Error propagation tests for various failure cases - Action ordering verification

Add serialization structures for posix_spawn file actions: - Kernel/API/spawn.h: SpawnFileAction* structs and alignment - LibC spawn.cpp: convert from lambdas to serialize/deserialize - Kernel returns ENOTSUP, triggering LibC fallback to fork() Runtime behavior unchanged - all actions still use fork() path.

First kernel-side file action implementation. The dup2 action duplicates a file descriptor, with explicit closure of any existing fd at the target slot. Other action types still return ENOTSUP, triggering LibC fallback.

Implement the close file action for posix_spawn. Validates the fd exists and closes it properly.

Opens a file and places it at the specified fd. Handles path parsing, flags, mode, umask, and explicit closure of any existing fd.

Changes the current working directory for the spawned process.

Changes current directory via fd. Validates the fd is a directory and checks for execute (search) permission.

All posix_spawn file actions are now implemented in the kernel: - dup2, close, open, chdir, fchdir The ENOTSUP fallback path in LibC is no longer needed. Errors from file actions are now returned directly from the syscall. The fork() path is still used when spawnattr is present (FIXME).

spholz · 2026-02-26T22:15:30Z

Sorry for the delay, I've almost finished my full review. I hope I can submit it by tomorrow.

spholz

Thank you for working on this!

(I hope I'm not overwhelming you with my following review comments.)

spholz · 2026-02-25T18:26:41Z

Kernel/API/spawn.h

nit: I'd rename this file to "Spawn.h" (uppercase "S").

spholz · 2026-02-25T18:33:29Z

Kernel/API/spawn.h

+    i32 fd;
+};
+
+inline constexpr size_t SPAWN_FILE_ACTION_ALIGNMENT = max(


Instead of doing this, I'd either put all spawn file action structs in a (tagged) union, or at least somehow get rid of this max() cascade.

Alternatively, you could maybe use Array::max() by doing something like to_array<size_t>({ alignof(...), ... }).max().

Also, constexpr variables are always implicitly inline, so this inline is unnecessary here.

spholz · 2026-02-25T18:34:31Z

Kernel/API/spawn.h

+
+struct SpawnFileActionDup2 {
+    SpawnFileActionHeader header;
+    i32 old_fd;


File descriptors use int as their type. Any reason why you don't want to use that here? (Same for all other spawn file action struct file descriptor members)

spholz · 2026-02-25T18:48:12Z

Userland/Libraries/LibC/spawn.cpp

-            if (action() < 0) {
-                perror("posix_spawn file action");
+    if (file_actions && !file_actions->state->buffer.is_empty()) {
+        using namespace Kernel;


I'm not really happy about the using namespace Kernels in this file. I'd prefer to qualify the complete name instead.

spholz · 2026-02-25T18:56:51Z

Userland/Libraries/LibC/spawn.cpp

-                return IterationDecision::Continue;
-            return IterationDecision::Break;
-        });
+    // Use posix_spawn which handles the syscall path


I'm confused by this comment here. I assume you mean that posix_spawn will use the posix_spawn syscall?

spholz · 2026-02-26T20:06:57Z

Tests/Kernel/TestPosixSpawn.cpp

+    spawn_and_wait(&actions, attr_ptr, "/bin/pwd"sv, argv, 0);
+
+    auto content = read_file_content(out_path);
+    EXPECT(content.trim_whitespace() == "/tmp" || content.trim_whitespace() == "/private/tmp");


Why do you allow /private/tmp here? Same for fchdir.

spholz · 2026-02-26T20:31:15Z

Kernel/Syscalls/posix_spawn.cpp

glibc and freebsd (https://man.freebsd.org/cgi/man.cgi?query=posix_spawn_file_actions_addchdir_np) only seem to provide _np (non portable) versions of addchdir and addfchdir, not sure why. Maybe there is some reason they didn't like some POSIX behavior?

Maybe something worth investigating, but that's not really related to your PR.

spholz · 2026-02-27T19:05:30Z

Kernel/Syscalls/posix_spawn.cpp


        switch (header->type) {
+        case SpawnFileActionType::Dup2: {
+            if (header->record_length < sizeof(SpawnFileActionDup2))


For non-variable length structs I would be extra cautious and check that they have the exact same size as expected.

spholz · 2026-02-27T19:17:16Z

Kernel/Syscalls/posix_spawn.cpp

+                return EINVAL;
+
+            auto const* action = reinterpret_cast<SpawnFileActionDup2 const*>(header);
+            TRY(m_fds.with_exclusive([&](auto& fds) -> ErrorOr<void> {


Rather than duplicating syscall implementations here, please reuse the existing syscall implementations (I think that should be possible). In some cases that won't be directly possible, so you probably need to add new _impl functions for some of the syscalls.

(note to self: I haven't reviewed at any file action implementation other than dup2.)

spholz · 2026-02-27T19:20:29Z

Kernel/Syscalls/posix_spawn.cpp


                auto description = TRY(fds.open_file_description(action->old_fd));
                if (action->old_fd != action->new_fd) {
+


It seems like you accidentally added two empty lines in the last commit in this file. Or was that on purpose and you applied the fix to the wrong commit? In that case, please also add empty lines between the other cases.

firatkizilboga · 2026-03-11T18:06:07Z

Thank you for the review @spholz. I did some initial work to address your points. I am a bit busy these days, but I have not abandoned this and I will definitely come back to this over the next weeks. About AK::Stream and the discussion around that, I thought we can remove the path from the structs and just put it directly after the related struct in the buffer. I think this could allow use of a stream, please let me know what you think.

spholz · 2026-03-11T19:03:26Z

Thank you for the review @spholz. I did some initial work to address your points. I am a bit busy these days, but I have not abandoned this and I will definitely come back to this over the next weeks.

No pressure, take your time!

About AK::Stream and the discussion around that, I thought we can remove the path from the structs and just put it directly after the related struct in the buffer. I think this could allow use of a stream, please let me know what you think.

Yes, I think that should work. Note that you need to do something like this for every struct type to make read_value<T>()/write_value<T>() work, assuming that your types are trivally serializable, which they should be:

template<>
class AK::Traits<YourStructNameHere> : public DefaultTraits<YourStructNameHere> {
public:
    static constexpr bool is_trivially_serializable() { return true; }
};

Hendiadyoin1 reviewed Jan 20, 2026

View reviewed changes

spholz reviewed Jan 20, 2026

View reviewed changes

Kernel/API/POSIX/spawn.h Outdated Show resolved Hide resolved

firatkizilboga force-pushed the spwan-file-attrs branch from 22b6a15 to a8e5617 Compare January 20, 2026 20:59

firatkizilboga marked this pull request as ready for review January 21, 2026 17:02

github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Jan 21, 2026

spholz reviewed Jan 31, 2026

View reviewed changes

Kernel/API/POSIX/spawn.h Outdated Show resolved Hide resolved

Kernel/API/POSIX/spawn.h Outdated Show resolved Hide resolved

Kernel/API/POSIX/spawn.h Outdated Show resolved Hide resolved

spholz reviewed Jan 31, 2026

View reviewed changes

Tests/Kernel/TestPosixSpawn.cpp Show resolved Hide resolved

firatkizilboga added 8 commits February 3, 2026 21:07

Kernel: Implement adddup2 spawn file action

3252698

First kernel-side file action implementation. The dup2 action duplicates a file descriptor, with explicit closure of any existing fd at the target slot. Other action types still return ENOTSUP, triggering LibC fallback.

Kernel: Implement addclose spawn file action

aa9ce98

Implement the close file action for posix_spawn. Validates the fd exists and closes it properly.

Kernel: Implement addopen spawn file action

dc5cf72

Opens a file and places it at the specified fd. Handles path parsing, flags, mode, umask, and explicit closure of any existing fd.

Kernel: Implement addchdir spawn file action

b331393

Changes the current working directory for the spawned process.

Kernel: Implement addfchdir spawn file action

7fb91e0

Changes current directory via fd. Validates the fd is a directory and checks for execute (search) permission.

firatkizilboga force-pushed the spwan-file-attrs branch from a8e5617 to 6d63f2a Compare February 7, 2026 15:29

firatkizilboga requested a review from spholz February 17, 2026 16:00

spholz reviewed Feb 27, 2026

View reviewed changes

spholz added ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels Mar 5, 2026

spholz mentioned this pull request Mar 24, 2026

LibSSH+SSHServer: Enough to run commands over the network #26684

Merged


		auto description = TRY(fds.open_file_description(action->old_fd));
		if (action->old_fd != action->new_fd) {

Conversation

firatkizilboga commented Jan 19, 2026

Uh oh!

Hendiadyoin1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firatkizilboga Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

firatkizilboga commented Jan 20, 2026

Uh oh!

spholz commented Jan 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spholz commented Jan 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spholz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spholz commented Feb 26, 2026

Uh oh!

spholz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firatkizilboga commented Mar 11, 2026

Uh oh!

spholz commented Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

firatkizilboga Jan 20, 2026 •

edited

Loading

spholz commented Jan 31, 2026 •

edited

Loading

spholz commented Jan 31, 2026 •

edited

Loading