Repository: SerenityOS/serenity
File: Kernel/Syscalls/posix_spawn.cpp (C++)

Code before:
    auto copy_user_strings = [](auto const& list, auto& output) -> ErrorOr<void> {
        if (!list.length)
            return {};
        Checked<size_t> size = sizeof(*list.strings);
        size *= list.length;
        if (size.has_overflow())
            return EOVERFLOW;
        Vector<Syscall::StringArgument, 32> strings;
        TRY(strings.try_resize(list.length));
        TRY(copy_from_user(strings.data(), list.strings, size.value()));
        for (size_t i = 0; i < list.length; ++i) {
            auto string = TRY(try_copy_kstring_from_user(strings[i]));
            TRY(output.try_append(move(string)));
        }
        return {};
    };

    Vector<NonnullOwnPtr<KString>> arguments;
    TRY(copy_user_strings(params.arguments, arguments));

    Vector<NonnullOwnPtr<KString>> environment;
    TRY(copy_user_strings(params.environment, environment));

    auto credentials = this->credentials();
    auto executable = TRY(VirtualFileSystem::resolve_path(vfs_root_context(), credentials, path->view(), current_directory()));
    auto [child, child_first_thread] = TRY(Process::create({}, credentials->uid(), credentials->gid(), pid(), m_is_kernel_process, vfs_root_context(), hostname_context(), current_directory(), executable, tty(), nullptr));

    ArmedScopeGuard thread_finalizer_guard = [&child_first_thread]() {
        SpinlockLocker lock(g_scheduler_lock);
        child_first_thread->detach();
        child_first_thread->set_state(Thread::State::Dying);
    };

    // "It is implementation-defined whether the fork handlers are run when posix_spawn() or posix_spawnp() is called."
    // We don't run them, as they are currently implemented in LibC.

    TRY(child->m_fds.with_exclusive([&](auto& child_fds) {
        return m_fds.with_exclusive([&](auto const& parent_fds) {
            return child_fds.try_clone(parent_fds);
        });

        // FD_CLOEXEC is handled by do_exec().
        // FIXME: Support FD_CLOFORK.
    }));

    // 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."

    // Copy protected data which isn't set by do_exec().
    child->with_mutable_protected_data([&](auto& child_protected_data) {
        with_protected_data([&](auto const& parent_protected_data) {

Review comment (performance):
Are you sure this sets the process's name? The name should be the first `Process::create()` argument, which is still an empty `StringView`. I think the profiler JSON just uses the executable path and falls back to the process name wrapped in `<>`: https://github.com/LucasChollet/serenity/blob/b24c7d39ccc4877afeed9c303d11513f5498f03c/Kernel/Tasks/PerformanceEventBuffer.cpp#L392
That should also be why the executable name was `"<>"`

You now seem to set the executable of the new process to the one we will `exec()` to. I don't think this is a nice solution, since the process doesn't actually have that executable image yet. Does Profiler expect the executable path to be valid for PERF_EVENT_PROCESS_CREATE? If no, can't we keep it empty?

Oh and `Process::create_user_process()` currently doesn't set an executable either.

Code after:
    auto copy_user_strings = [](auto const& list, auto& output) -> ErrorOr<void> {
        if (!list.length)
            return {};
        Checked<size_t> size = sizeof(*list.strings);
        size *= list.length;
        if (size.has_overflow())
            return EOVERFLOW;
        Vector<Syscall::StringArgument, 32> strings;
        TRY(strings.try_resize(list.length));
        TRY(copy_from_user(strings.data(), list.strings, size.value()));
        for (size_t i = 0; i < list.length; ++i) {
            auto string = TRY(try_copy_kstring_from_user(strings[i]));
            TRY(output.try_append(move(string)));
        }
        return {};
    };

    Vector<NonnullOwnPtr<KString>> arguments;
    TRY(copy_user_strings(params.arguments, arguments));

    Vector<NonnullOwnPtr<KString>> environment;
    TRY(copy_user_strings(params.environment, environment));

    auto const& credentials = this->credentials();
    VERIFY(!m_is_kernel_process);
    auto [child, child_first_thread] = TRY(Process::create_spawned(credentials->uid(), credentials->gid(), pid(), vfs_root_context(), hostname_context(), current_directory(), tty()));

    ArmedScopeGuard thread_finalizer_guard = [&child_first_thread]() {
        SpinlockLocker lock(g_scheduler_lock);
        child_first_thread->detach();
        child_first_thread->set_state(Thread::State::Dying);
    };

    // "It is implementation-defined whether the fork handlers are run when posix_spawn() or posix_spawnp() is called."
    // We don't run them, as they are currently implemented in LibC.

    TRY(child->m_fds.with_exclusive([&](auto& child_fds) {
        return m_fds.with_exclusive([&](auto const& parent_fds) {
            return child_fds.try_clone(parent_fds);
        });

        // FD_CLOEXEC is handled by do_exec().
        // FIXME: Support FD_CLOFORK.
    }));

    // 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."

    // Copy protected data which isn't set by do_exec().
    child->with_mutable_protected_data([&](auto& child_protected_data) {
        with_protected_data([&](auto const& parent_protected_data) {