Kernel: Track allocated FileDescriptionAndFlag elements in each Process

The way the Process::FileDescriptions::allocate() API works today means
that two callers who allocate back to back without associating a
FileDescription with the allocated FD, will receive the same FD and thus
one will stomp over the other.

Naively tracking which FileDescriptions are allocated and moving onto
the next would introduce other bugs however, as now if you "allocate"
a fd and then return early further down the control flow of the syscall
you would leak that fd.

This change modifies this behavior by tracking which descriptions are
allocated and then having an RAII type to "deallocate" the fd if the
association is not setup the end of it's scope.

Kernel/Process.cpp

@@ -169,9 +169,14 @@ RefPtr<Process> Process::create_user_process(RefPtr<Thread>& first_thread, const
     }
     auto& device_to_use_as_tty = tty ? (CharacterDevice&)*tty : NullDevice::the();
     auto description = device_to_use_as_tty.open(O_RDWR).value();
-    process->m_fds[0].set(*description);
+
-    process->m_fds[1].set(*description);
+    auto setup_description = [&process, &description](int fd) {
-    process->m_fds[2].set(*description);
+        process->m_fds.m_fds_metadatas[fd].allocate();
+        process->m_fds[fd].set(*description);
+    };
+    setup_description(0);
+    setup_description(1);
+    setup_description(2);
 
     error = process->exec(path, move(arguments), move(environment));
     if (error != 0) {
@@ -411,11 +416,13 @@ RefPtr<Process> Process::from_pid(ProcessID pid)
 const Process::FileDescriptionAndFlags& Process::FileDescriptions::at(size_t i) const
 {
     ScopedSpinLock lock(m_fds_lock);
+    VERIFY(m_fds_metadatas[i].is_allocated());
     return m_fds_metadatas[i];
 }
 Process::FileDescriptionAndFlags& Process::FileDescriptions::at(size_t i)
 {
     ScopedSpinLock lock(m_fds_lock);
+    VERIFY(m_fds_metadatas[i].is_allocated());
     return m_fds_metadatas[i];
 }
 
@@ -465,14 +472,16 @@ size_t Process::FileDescriptions::open_count() const
     return count;
 }
 
-KResultOr<int> Process::FileDescriptions::allocate(int first_candidate_fd)
+KResultOr<Process::ScopedDescriptionAllocation> Process::FileDescriptions::allocate(int first_candidate_fd)
 {
     ScopedSpinLock lock(m_fds_lock);
     for (size_t i = first_candidate_fd; i < max_open(); ++i) {
-        if (!m_fds_metadatas[i])
+        if (!m_fds_metadatas[i].is_allocated()) {
-            return i;
+            m_fds_metadatas[i].allocate();
+            return Process::ScopedDescriptionAllocation { static_cast<int>(i), &m_fds_metadatas[i] };
+        }
     }
-    return KResult(EMFILE);
+    return EMFILE;
 }
 
 Time kgettimeofday()

Kernel/Process.h

@@ -597,6 +597,19 @@ public:
         operator bool() const { return !!m_description; }
 
         bool is_valid() const { return !m_description.is_null(); }
+        bool is_allocated() const { return m_is_allocated; }
+        void allocate()
+        {
+            VERIFY(!m_is_allocated);
+            VERIFY(!is_valid());
+            m_is_allocated = true;
+        }
+        void deallocate()
+        {
+            VERIFY(m_is_allocated);
+            VERIFY(!is_valid());
+            m_is_allocated = false;
+        }
 
         FileDescription* description() { return m_description; }
         const FileDescription* description() const { return m_description; }
@@ -610,6 +623,7 @@ public:
 
     private:
         RefPtr<FileDescription> m_description;
+        bool m_is_allocated { false };
         u32 m_flags { 0 };
 
         // Note: This is needed so when we generate inodes for ProcFS, we know that
@@ -617,6 +631,7 @@ public:
         InodeIndex m_global_procfs_inode_index;
     };
 
+    class ScopedDescriptionAllocation;
     class FileDescriptions {
         friend class Process;
 
@@ -641,7 +656,7 @@ public:
         void enumerate(Function<void(const FileDescriptionAndFlags&)>) const;
         void change_each(Function<void(FileDescriptionAndFlags&)>);
 
-        KResultOr<int> allocate(int first_candidate_fd = 0);
+        KResultOr<ScopedDescriptionAllocation> allocate(int first_candidate_fd = 0);
         size_t open_count() const;
 
         bool try_resize(size_t size) { return m_fds_metadatas.try_resize(size); }
@@ -668,6 +683,37 @@ public:
         Vector<FileDescriptionAndFlags> m_fds_metadatas;
     };
 
+    class ScopedDescriptionAllocation {
+        AK_MAKE_NONCOPYABLE(ScopedDescriptionAllocation);
+
+    public:
+        ScopedDescriptionAllocation() = default;
+        ScopedDescriptionAllocation(int tracked_fd, FileDescriptionAndFlags* description)
+            : fd(tracked_fd)
+            , m_description(description)
+        {
+        }
+
+        ScopedDescriptionAllocation(ScopedDescriptionAllocation&& other)
+            : fd(other.fd)
+        {
+            // Take over the responsibility of tracking to deallocation.
+            swap(m_description, other.m_description);
+        }
+
+        ~ScopedDescriptionAllocation()
+        {
+            if (m_description && m_description->is_allocated() && !m_description->is_valid()) {
+                m_description->deallocate();
+            }
+        }
+
+        const int fd { -1 };
+
+    private:
+        FileDescriptionAndFlags* m_description { nullptr };
+    };
+
     FileDescriptions& fds() { return m_fds; }
     const FileDescriptions& fds() const { return m_fds; }
 

Kernel/Syscalls/anon_create.cpp

@@ -28,7 +28,7 @@ KResultOr<FlatPtr> Process::sys$anon_create(size_t size, int options)
     auto new_fd_or_error = m_fds.allocate();
     if (new_fd_or_error.is_error())
         return new_fd_or_error.error();
-    auto new_fd = new_fd_or_error.value();
+    auto new_fd = new_fd_or_error.release_value();
     auto vmobject = AnonymousVMObject::try_create_purgeable_with_size(size, AllocationStrategy::Reserve);
     if (!vmobject)
         return ENOMEM;
@@ -48,8 +48,8 @@ KResultOr<FlatPtr> Process::sys$anon_create(size_t size, int options)
     if (options & O_CLOEXEC)
         fd_flags |= FD_CLOEXEC;
 
-    m_fds[new_fd].set(move(description), fd_flags);
+    m_fds[new_fd.fd].set(move(description), fd_flags);
-    return new_fd;
+    return new_fd.fd;
 }
 
 }

Kernel/Syscalls/dup2.cpp

@@ -20,6 +20,8 @@ KResultOr<FlatPtr> Process::sys$dup2(int old_fd, int new_fd)
         return new_fd;
     if (new_fd < 0 || static_cast<size_t>(new_fd) >= fds().max_open())
         return EINVAL;
+    if (!m_fds.m_fds_metadatas[new_fd].is_allocated())
+        m_fds.m_fds_metadatas[new_fd].allocate();
     m_fds[new_fd].set(*description);
     return new_fd;
 }

Kernel/Syscalls/execve.cpp

@@ -591,12 +591,13 @@ KResult Process::do_exec(NonnullRefPtr<FileDescription> main_program_description
 
     int main_program_fd = -1;
     if (interpreter_description) {
-        main_program_fd = m_fds.allocate().value();
+        auto main_program_fd_wrapper = m_fds.allocate().release_value();
-        VERIFY(main_program_fd >= 0);
+        VERIFY(main_program_fd_wrapper.fd >= 0);
         auto seek_result = main_program_description->seek(0, SEEK_SET);
         VERIFY(!seek_result.is_error());
         main_program_description->set_readable(true);
-        m_fds[main_program_fd].set(move(main_program_description), FD_CLOEXEC);
+        m_fds[main_program_fd_wrapper.fd].set(move(main_program_description), FD_CLOEXEC);
+        main_program_fd = main_program_fd_wrapper.fd;
     }
 
     new_main_thread = nullptr;

Kernel/Syscalls/fcntl.cpp

@@ -28,9 +28,9 @@ KResultOr<FlatPtr> Process::sys$fcntl(int fd, int cmd, u32 arg)
         auto new_fd_or_error = fds().allocate(arg_fd);
         if (new_fd_or_error.is_error())
             return new_fd_or_error.error();
-        auto new_fd = new_fd_or_error.value();
+        auto new_fd = new_fd_or_error.release_value();
-        m_fds[new_fd].set(*description);
+        m_fds[new_fd.fd].set(*description);
-        return new_fd;
+        return new_fd.fd;
     }
     case F_GETFD:
         return m_fds[fd].flags();

Kernel/Syscalls/inode_watcher.cpp

@@ -21,7 +21,7 @@ KResultOr<FlatPtr> Process::sys$create_inode_watcher(u32 flags)
     auto fd_or_error = m_fds.allocate();
     if (fd_or_error.is_error())
         return fd_or_error.error();
-    auto fd = fd_or_error.value();
+    auto inode_watcher_fd = fd_or_error.release_value();
 
     auto watcher_or_error = InodeWatcher::create();
     if (watcher_or_error.is_error())
@@ -31,15 +31,15 @@ KResultOr<FlatPtr> Process::sys$create_inode_watcher(u32 flags)
     if (description_or_error.is_error())
         return description_or_error.error();
 
-    m_fds[fd].set(description_or_error.release_value());
+    m_fds[inode_watcher_fd.fd].set(description_or_error.release_value());
-    m_fds[fd].description()->set_readable(true);
+    m_fds[inode_watcher_fd.fd].description()->set_readable(true);
 
     if (flags & static_cast<unsigned>(InodeWatcherFlags::Nonblock))
-        m_fds[fd].description()->set_blocking(false);
+        m_fds[inode_watcher_fd.fd].description()->set_blocking(false);
     if (flags & static_cast<unsigned>(InodeWatcherFlags::CloseOnExec))
-        m_fds[fd].set_flags(m_fds[fd].flags() | FD_CLOEXEC);
+        m_fds[inode_watcher_fd.fd].set_flags(m_fds[inode_watcher_fd.fd].flags() | FD_CLOEXEC);
 
-    return fd;
+    return inode_watcher_fd.fd;
 }
 
 KResultOr<FlatPtr> Process::sys$inode_watcher_add_watch(Userspace<const Syscall::SC_inode_watcher_add_watch_params*> user_params)

Kernel/Syscalls/open.cpp

@@ -49,7 +49,7 @@ KResultOr<FlatPtr> Process::sys$open(Userspace<const Syscall::SC_open_params*> u
     auto fd_or_error = m_fds.allocate();
     if (fd_or_error.is_error())
         return fd_or_error.error();
-    auto fd = fd_or_error.value();
+    auto new_fd = fd_or_error.release_value();
     RefPtr<Custody> base;
     if (dirfd == AT_FDCWD) {
         base = current_directory();
@@ -73,8 +73,8 @@ KResultOr<FlatPtr> Process::sys$open(Userspace<const Syscall::SC_open_params*> u
         return ENXIO;
 
     u32 fd_flags = (options & O_CLOEXEC) ? FD_CLOEXEC : 0;
-    m_fds[fd].set(move(description), fd_flags);
+    m_fds[new_fd.fd].set(move(description), fd_flags);
-    return fd;
+    return new_fd.fd;
 }
 
 KResultOr<FlatPtr> Process::sys$close(int fd)

Kernel/Syscalls/pipe.cpp

@@ -33,19 +33,20 @@ KResultOr<FlatPtr> Process::sys$pipe(int pipefd[2], int flags)
     auto reader_fd_or_error = m_fds.allocate();
     if (reader_fd_or_error.is_error())
         return reader_fd_or_error.error();
-    auto reader_fd = reader_fd_or_error.value();
+    auto reader_fd = reader_fd_or_error.release_value();
-    m_fds[reader_fd].set(open_reader_result.release_value(), fd_flags);
+    m_fds[reader_fd.fd].set(open_reader_result.release_value(), fd_flags);
-    m_fds[reader_fd].description()->set_readable(true);
+    m_fds[reader_fd.fd].description()->set_readable(true);
-    if (!copy_to_user(&pipefd[0], &reader_fd))
+    if (!copy_to_user(&pipefd[0], &reader_fd.fd))
         return EFAULT;
 
     auto writer_fd_or_error = m_fds.allocate();
     if (writer_fd_or_error.is_error())
         return writer_fd_or_error.error();
-    auto writer_fd = writer_fd_or_error.value();
+    auto writer_fd = writer_fd_or_error.release_value();
-    m_fds[writer_fd].set(open_writer_result.release_value(), fd_flags);
+    m_fds[writer_fd.fd].set(open_writer_result.release_value(), fd_flags);
-    m_fds[writer_fd].description()->set_writable(true);
+    m_fds[writer_fd.fd].description()->set_writable(true);
-    if (!copy_to_user(&pipefd[1], &writer_fd))
+
+    if (!copy_to_user(&pipefd[1], &writer_fd.fd))
         return EFAULT;
 
     return 0;

Kernel/Syscalls/sendfd.cpp

@@ -49,7 +49,7 @@ KResultOr<FlatPtr> Process::sys$recvfd(int sockfd, int options)
     auto new_fd_or_error = m_fds.allocate();
     if (new_fd_or_error.is_error())
         return new_fd_or_error.error();
-    auto new_fd = new_fd_or_error.value();
+    auto new_fd = new_fd_or_error.release_value();
 
     auto& local_socket = static_cast<LocalSocket&>(socket);
     auto received_descriptor_or_error = local_socket.recvfd(*socket_description);
@@ -61,8 +61,8 @@ KResultOr<FlatPtr> Process::sys$recvfd(int sockfd, int options)
     if (options & O_CLOEXEC)
         fd_flags |= FD_CLOEXEC;
 
-    m_fds[new_fd].set(*received_descriptor_or_error.value(), fd_flags);
+    m_fds[new_fd.fd].set(*received_descriptor_or_error.value(), fd_flags);
-    return new_fd;
+    return new_fd.fd;
 }
 
 }

Kernel/Syscalls/socket.cpp

@@ -42,15 +42,15 @@ KResultOr<FlatPtr> Process::sys$socket(int domain, int type, int protocol)
     auto fd_or_error = m_fds.allocate();
     if (fd_or_error.is_error())
         return fd_or_error.error();
-    auto fd = fd_or_error.value();
+    auto socket_fd = fd_or_error.release_value();
     auto result = Socket::create(domain, type, protocol);
     if (result.is_error())
         return result.error();
     auto description_result = FileDescription::create(*result.value());
     if (description_result.is_error())
         return description_result.error();
-    setup_socket_fd(fd, description_result.value(), type);
+    setup_socket_fd(socket_fd.fd, description_result.value(), type);
-    return fd;
+    return socket_fd.fd;
 }
 
 KResultOr<FlatPtr> Process::sys$bind(int sockfd, Userspace<const sockaddr*> address, socklen_t address_length)
@@ -104,7 +104,7 @@ KResultOr<FlatPtr> Process::sys$accept4(Userspace<const Syscall::SC_accept4_para
     auto accepted_socket_fd_or_error = m_fds.allocate();
     if (accepted_socket_fd_or_error.is_error())
         return accepted_socket_fd_or_error.error();
-    auto accepted_socket_fd = accepted_socket_fd_or_error.value();
+    auto accepted_socket_fd = accepted_socket_fd_or_error.release_value();
     auto accepting_socket_description = fds().file_description(accepting_socket_fd);
     if (!accepting_socket_description)
         return EBADF;
@@ -145,11 +145,11 @@ KResultOr<FlatPtr> Process::sys$accept4(Userspace<const Syscall::SC_accept4_para
     int fd_flags = 0;
     if (flags & SOCK_CLOEXEC)
         fd_flags |= FD_CLOEXEC;
-    m_fds[accepted_socket_fd].set(accepted_socket_description_result.release_value(), fd_flags);
+    m_fds[accepted_socket_fd.fd].set(accepted_socket_description_result.release_value(), fd_flags);
 
     // NOTE: Moving this state to Completed is what causes connect() to unblock on the client side.
     accepted_socket->set_setup_state(Socket::SetupState::Completed);
-    return accepted_socket_fd;
+    return accepted_socket_fd.fd;
 }
 
 KResultOr<FlatPtr> Process::sys$connect(int sockfd, Userspace<const sockaddr*> user_address, socklen_t user_address_size)
@@ -416,13 +416,13 @@ KResultOr<FlatPtr> Process::sys$socketpair(Userspace<const Syscall::SC_socketpai
     auto fd1_or_error = m_fds.allocate();
     if (fd1_or_error.is_error())
         return fd1_or_error.error();
-    fds[0] = fd1_or_error.value();
+    fds[0] = fd1_or_error.value().fd;
     setup_socket_fd(fds[0], pair.description1, params.type);
 
     auto fd2_or_error = m_fds.allocate();
     if (fd2_or_error.is_error())
         return fd2_or_error.error();
-    fds[1] = fd2_or_error.value();
+    fds[1] = fd2_or_error.value().fd;
     setup_socket_fd(fds[1], pair.description2, params.type);
 
     if (!copy_to_user(params.sv, fds, sizeof(fds))) {