Skip to content

Kernel+LibC: Implement posix_spawn file actions in kernel #26562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
firatkizilboga wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Kernel+LibC: Implement posix_spawn file actions in kernel #26562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
94fbfd3
Tests/Kernel: Add posix_spawn file actions tests
firatkizilboga Feb 3, 2026
c08f8b7
Kernel+LibC: Add spawn file action serialization
firatkizilboga Feb 3, 2026
3252698
Kernel: Implement adddup2 spawn file action
firatkizilboga Feb 3, 2026
aa9ce98
Kernel: Implement addclose spawn file action
firatkizilboga Feb 3, 2026
dc5cf72
Kernel: Implement addopen spawn file action
firatkizilboga Feb 3, 2026
b331393
Kernel: Implement addchdir spawn file action
firatkizilboga Feb 3, 2026
7fb91e0
Kernel: Implement addfchdir spawn file action
firatkizilboga Feb 3, 2026
6d63f2a
Kernel+LibC: Remove ENOTSUP fallback for file actions
firatkizilboga Feb 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions Kernel/API/spawn.h
View file
Open in desktop
Copy link
Copy Markdown
Member

@spholz spholz Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright (c) 2026, Fırat Kızılboğa <firatkizilboga11@gmail.com>
*
* SPDX-License-Identifier: BSD-2-Clause
*/

#pragma once

#include <AK/StdLibExtras.h>
#include <AK/Types.h>
#include <Kernel/API/POSIX/sys/types.h>

namespace Kernel {

enum class SpawnFileActionType : u8 {
Dup2 = 0,
Open = 1,
Close = 2,
Chdir = 3,
Fchdir = 4,
};

struct SpawnFileActionHeader {
SpawnFileActionType type;
u16 record_length;
};

struct SpawnFileActionDup2 {
SpawnFileActionHeader header;
i32 old_fd;
Copy link
Copy Markdown
Member

@spholz spholz Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

i32 new_fd;
};

struct SpawnFileActionOpen {
SpawnFileActionHeader header;
i32 fd;
i32 flags;
mode_t mode;
u16 path_length;
char path[];
};

struct SpawnFileActionClose {
SpawnFileActionHeader header;
i32 fd;
};

struct SpawnFileActionChdir {
SpawnFileActionHeader header;
u16 path_length;
char path[];
};

struct SpawnFileActionFchdir {
SpawnFileActionHeader header;
i32 fd;
};

inline constexpr size_t SPAWN_FILE_ACTION_ALIGNMENT = max(
Copy link
Copy Markdown
Member

@spholz spholz Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

max(alignof(SpawnFileActionHeader), alignof(SpawnFileActionDup2)),
max(max(alignof(SpawnFileActionOpen), alignof(SpawnFileActionClose)),
max(alignof(SpawnFileActionChdir), alignof(SpawnFileActionFchdir))));

}
155 changes: 153 additions & 2 deletions Kernel/Syscalls/posix_spawn.cpp
View file
Open in desktop
Copy link
Copy Markdown
Member

@spholz spholz Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
@@ -1,23 +1,159 @@
/*
* Copyright (c) 2025, Tomás Simões <tomasprsimoes@tecnico.ulisboa.pt>
* Copyright (c) 2026, Fırat Kızılboğa <firatkizilboga11@gmail.com>
*
* SPDX-License-Identifier: BSD-2-Clause
*/

#include <Kernel/API/spawn.h>
#include <Kernel/Debug.h>
#include <Kernel/Devices/BaseDevices.h>
#include <Kernel/Devices/Device.h>
#include <Kernel/Devices/Generic/NullDevice.h>
#include <Kernel/Devices/TTY/TTY.h>
#include <Kernel/FileSystem/Custody.h>
#include <Kernel/FileSystem/VirtualFileSystem.h>
#include <Kernel/Memory/Region.h>
#include <Kernel/Net/LocalSocket.h>
#include <Kernel/Tasks/PerformanceManager.h>
#include <Kernel/Tasks/Process.h>
#include <Kernel/Tasks/Scheduler.h>
#include <Kernel/Tasks/ScopedProcessList.h>

namespace Kernel {

ErrorOr<void> Process::execute_file_actions(ReadonlyBytes file_actions_data)
{
size_t offset = 0;
while (offset < file_actions_data.size()) {
if (offset + sizeof(SpawnFileActionHeader) > file_actions_data.size())
return EINVAL;

auto const* header = reinterpret_cast<SpawnFileActionHeader const*>(file_actions_data.data() + offset);
Copy link
Copy Markdown
Member

@spholz spholz Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes that userspace aligned the spawn file action structs properly. It also assumes that the spawn actions buffer itself is properly aligned (so that the KBuffer is properly aligned, not sure if that is always the case).

I assume you intended all spawn file action structs to be aligned on a SPAWN_FILE_ACTION_ALIGNMENT-byte boundary, right? So you'd either need to ensure that all structs are properly aligned on the kernel side or not rely on them being properly aligned (whichever is easier to do).

Ideally this would just use an AK::Stream (which handles all of this including bounds checks), but since you are using flexible array members this isn't easily possible.

Copy link
Copy Markdown
Member

@spholz spholz Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you could use a stream if you read the variable length part separately from the fixed length part of the structs. The only other change I think you would need to do is to move the header struct out of the file action structs to be able to read it without knowing what kind of file action struct is next.

You should also be able to use streams for writing the file action list.

But if converting this code to use streams is too much effort, it might be fine to keep serializing/deserializing the file actions manually, provided alignment is handled properly.

Copy link
Copy Markdown
Member

@spholz spholz Feb 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though if you use an AK::Stream the advantage of having word-sized loads/stored by not using gnu::packed is somewhat nullified, since AK::Stream will use bit_cast, which is essentially a memcpy.

So I'm not sure what the best option is. But I do still think that being more on the safe side with using a AK::Stream is more important than any (likely negligible) performance impact.

Copy link
Copy Markdown
Member

@spholz spholz Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also be fine with going back to [[gnu::packed]] (and possibly static_asserting that the alignment of all structs is 1). That's probably the simplest option.

if (header->record_length < sizeof(SpawnFileActionHeader))
return EINVAL;
if (offset + header->record_length > file_actions_data.size())
return EINVAL;

switch (header->type) {
case SpawnFileActionType::Dup2: {
if (header->record_length < sizeof(SpawnFileActionDup2))
Copy link
Copy Markdown
Member

@spholz spholz Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return EINVAL;

auto const* action = reinterpret_cast<SpawnFileActionDup2 const*>(header);
TRY(m_fds.with_exclusive([&](auto& fds) -> ErrorOr<void> {
Copy link
Copy Markdown
Member

@spholz spholz Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

if (action->new_fd < 0 || static_cast<size_t>(action->new_fd) >= fds.max_open())
return EINVAL;

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

Copy link
Copy Markdown
Member

@spholz spholz Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (fds.m_fds_metadatas[action->new_fd].is_allocated()) {
if (auto* old_description = fds[action->new_fd].description())
(void)old_description->close();
fds[action->new_fd].clear();
} else {
fds.m_fds_metadatas[action->new_fd].allocate();
}

fds[action->new_fd].set(move(description));
}
return {};
}));
break;
}
case SpawnFileActionType::Close: {
if (header->record_length < sizeof(SpawnFileActionClose))
return EINVAL;

auto const* action = reinterpret_cast<SpawnFileActionClose const*>(header);
TRY(m_fds.with_exclusive([&](auto& fds) -> ErrorOr<void> {
auto description = TRY(fds.open_file_description(action->fd));
TRY(description->close());
fds[action->fd].clear();
return {};
}));
break;
}
case SpawnFileActionType::Open: {
if (header->record_length < sizeof(SpawnFileActionOpen))
return EINVAL;

auto const* action = reinterpret_cast<SpawnFileActionOpen const*>(header);
if (header->record_length < sizeof(SpawnFileActionOpen) + action->path_length)
return EINVAL;

auto path = TRY(KString::try_create(StringView { action->path, action->path_length }));
CustodyBase base(AT_FDCWD, path->view());
auto description = TRY(VirtualFileSystem::open(
vfs_root_context(), credentials(), path->view(),
action->flags, action->mode & ~umask(), base));

if (description->inode() && description->inode()->bound_socket())
return ENXIO;

TRY(m_fds.with_exclusive([&](auto& fds) -> ErrorOr<void> {
if (action->fd < 0 || static_cast<size_t>(action->fd) >= fds.max_open())
return EINVAL;

if (fds.m_fds_metadatas[action->fd].is_allocated()) {
if (auto* old_description = fds[action->fd].description())
(void)old_description->close();
fds[action->fd].clear();
} else {
fds.m_fds_metadatas[action->fd].allocate();
}

u32 fd_flags = (action->flags & O_CLOEXEC) ? FD_CLOEXEC : 0;
fds[action->fd].set(move(description), fd_flags);
return {};
}));
break;
}

case SpawnFileActionType::Chdir: {
if (header->record_length < sizeof(SpawnFileActionChdir))
return EINVAL;

auto const* action = reinterpret_cast<SpawnFileActionChdir const*>(header);
if (header->record_length < sizeof(SpawnFileActionChdir) + action->path_length)
return EINVAL;

auto path = TRY(KString::try_create(StringView { action->path, action->path_length }));
auto new_directory = TRY(VirtualFileSystem::open_directory(
vfs_root_context(), credentials(), path->view(), current_directory()));
m_current_directory.with([&](auto& current_directory) {
current_directory = move(new_directory);
});
break;
}
case SpawnFileActionType::Fchdir: {
if (header->record_length < sizeof(SpawnFileActionFchdir))
return EINVAL;

auto const* action = reinterpret_cast<SpawnFileActionFchdir const*>(header);
auto description = TRY(open_file_description(action->fd));
if (!description->is_directory())
return ENOTDIR;

// Check for search (+x) permission on the directory.
if (!description->metadata().may_execute(credentials()))
return EACCES;

m_current_directory.with([&](auto& current_directory) {
current_directory = description->custody();
});
break;
}
default:
return EINVAL;
}

offset += header->record_length;
}
return {};
}

// https://pubs.opengroup.org/onlinepubs/9799919799/functions/posix_spawn.html
ErrorOr<FlatPtr> Process::sys$posix_spawn(Userspace<Syscall::SC_posix_spawn_params const*> user_params)
{
Expand All @@ -32,11 +168,21 @@ ErrorOr<FlatPtr> Process::sys$posix_spawn(Userspace<Syscall::SC_posix_spawn_para
if (params.arguments.length == 0)
return EINVAL;

if (params.attr_data.ptr() != 0 || params.attr_data_size != 0 || params.serialized_file_actions_data.ptr() != 0 || params.serialized_file_actions_data_size != 0) {
// FIXME: Implement spawn attributes and spawn file actions handling.
if (params.attr_data.ptr() != 0 || params.attr_data_size != 0) {
// FIXME: Implement spawn attributes handling.
return ENOTSUP;
}

// Copy file actions buffer from userspace
OwnPtr<KBuffer> file_actions_buffer;
if (params.serialized_file_actions_data.ptr() != 0 && params.serialized_file_actions_data_size != 0) {

if (params.serialized_file_actions_data_size > 1 * MiB)
return E2BIG;
file_actions_buffer = TRY(KBuffer::try_create_with_size("posix_spawn file actions"sv, params.serialized_file_actions_data_size));
TRY(copy_from_user(file_actions_buffer->data(), Userspace<u8 const*> { params.serialized_file_actions_data.ptr() }, params.serialized_file_actions_data_size));
}

auto path = TRY(get_syscall_path_argument(params.path));

auto copy_user_strings = [](auto const& list, auto& output) -> ErrorOr<void> {
Expand Down Expand Up @@ -84,6 +230,11 @@ ErrorOr<FlatPtr> Process::sys$posix_spawn(Userspace<Syscall::SC_posix_spawn_para
// FIXME: Support FD_CLOFORK.
}));

// Execute file actions on the child's FD table
if (file_actions_buffer) {
TRY(child->execute_file_actions(file_actions_buffer->bytes()));
}

// FIXME: "If file descriptor 0, 1, or 2 would otherwise be closed in the new process image created by posix_spawn() or posix_spawnp(),
// implementations may open an unspecified file for the file descriptor in the new process image."

Expand Down
1 change: 1 addition & 0 deletions Kernel/Tasks/Process.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,7 @@ class Process final

ErrorOr<FlatPtr> open_impl(Userspace<Syscall::SC_open_params const*>);
ErrorOr<FlatPtr> close_impl(int fd);
ErrorOr<void> execute_file_actions(ReadonlyBytes file_actions_data);
ErrorOr<FlatPtr> read_impl(int fd, Userspace<u8*> buffer, size_t size);
ErrorOr<FlatPtr> pread_impl(int fd, Userspace<u8*>, size_t, off_t);
ErrorOr<FlatPtr> preadv_impl(int fd, Userspace<const struct iovec*> iov, int iov_count, off_t);
Expand Down
Loading
Loading