File: Enforce setting inheritable and blocking flags during pipe creation

Pagghiu · Pagghiu · commit 43bb44e547b9 · 2025-08-15T17:32:18.000+02:00
diff --git a/Libraries/Async/Internal/AsyncPosix.inl b/Libraries/Async/Internal/AsyncPosix.inl
@@ -105,9 +105,9 @@ struct SC::AsyncEventLoop::Internal::KernelQueuePosix
     Result createWakeup(AsyncEventLoop& eventLoop)
     {
         // Create
-        SC_TRY(wakeupPipe.createPipe(PipeDescriptor::ReadNonInheritable, PipeDescriptor::WriteNonInheritable));
-        SC_TRY(wakeupPipe.readPipe.setBlocking(false));
-        SC_TRY(wakeupPipe.writePipe.setBlocking(false));
+        PipeOptions options;
+        options.blocking = false;
+        SC_TRY(wakeupPipe.createPipe(options));
 
         // Register
         FileDescriptor::Handle wakeUpPipeDescriptor;
diff --git a/Libraries/File/File.cpp b/Libraries/File/File.cpp
@@ -96,32 +96,6 @@ struct SC::FileDescriptor::Internal
         return success == FALSE;
     }
 };
-SC::Result SC::FileDescriptor::setBlocking(bool blocking)
-{
-    // TODO: IMPLEMENT
-    SC_COMPILER_UNUSED(blocking);
-    return Result(false);
-}
-
-SC::Result SC::FileDescriptor::setInheritable(bool inheritable)
-{
-    if (::SetHandleInformation(handle, HANDLE_FLAG_INHERIT, inheritable ? TRUE : FALSE) == FALSE)
-    {
-        return Result::Error("FileDescriptor::setInheritable - ::SetHandleInformation failed");
-    }
-    return Result(true);
-}
-
-SC::Result SC::FileDescriptor::isInheritable(bool& hasValue) const
-{
-    DWORD dwFlags = 0;
-    if (::GetHandleInformation(handle, &dwFlags) == FALSE)
-    {
-        return Result::Error("FileDescriptor::getInheritable = ::GetHandleInformation failed");
-    }
-    hasValue = (dwFlags & HANDLE_FLAG_INHERIT) != 0;
-    return Result(true);
-}
 
 SC::Result SC::FileDescriptor::seek(SeekMode seekMode, uint64_t offset)
 {
@@ -378,16 +352,6 @@ struct SC::FileDescriptor::Internal
         return Result(true);
     }
 
-    template <int flag>
-    static Result hasFileDescriptorFlags(int fileDescriptor, bool& hasFlag)
-    {
-        static_assert(flag == FD_CLOEXEC, "hasFileDescriptorFlags invalid value");
-        int flags = 0;
-        SC_TRY(getFileFlags(F_GETFD, fileDescriptor, flags));
-        hasFlag = (flags & flag) != 0;
-        return Result(true);
-    }
-
     template <int flag>
     static Result hasFileStatusFlags(int fileDescriptor, bool& hasFlag)
     {
@@ -466,28 +430,11 @@ SC::Result SC::FileDescriptor::open(StringSpan filePath, FileOpen mode)
     SC_TRY(assign(fileDescriptor));
     if (not mode.blocking)
     {
-        SC_TRY(setBlocking(false));
+        SC_TRY(Internal::setFileStatusFlags<O_NONBLOCK>(handle, true));
     }
     return Result(true);
 }
 
-SC::Result SC::FileDescriptor::setBlocking(bool blocking)
-{
-    return Internal::setFileStatusFlags<O_NONBLOCK>(handle, not blocking);
-}
-
-SC::Result SC::FileDescriptor::setInheritable(bool inheritable)
-{
-    return Internal::setFileDescriptorFlags<FD_CLOEXEC>(handle, not inheritable);
-}
-
-SC::Result SC::FileDescriptor::isInheritable(bool& hasValue) const
-{
-    auto res = Internal::hasFileDescriptorFlags<FD_CLOEXEC>(handle, hasValue);
-    hasValue = not hasValue;
-    return res;
-}
-
 SC::Result SC::FileDescriptor::seek(SeekMode seekMode, uint64_t offset)
 {
     int flags = 0;
@@ -625,42 +572,82 @@ SC::Result SC::FileDescriptor::readUntilEOF(IGrowableBuffer&& adapter)
 // PipeDescriptor
 //-------------------------------------------------------------------------------------------------------
 #if SC_PLATFORM_WINDOWS
-SC::Result SC::PipeDescriptor::createPipe(InheritableReadFlag readFlag, InheritableWriteFlag writeFlag)
+#include <stdio.h>
+SC::Result SC::PipeDescriptor::createPipe(PipeOptions options)
 {
     // On Windows to inherit flags they must be flagged as inheritable
     // https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873
     SECURITY_ATTRIBUTES security;
-    memset(&security, 0, sizeof(security));
+    ::memset(&security, 0, sizeof(security));
     security.nLength              = sizeof(security);
-    security.bInheritHandle       = readFlag == ReadInheritable or writeFlag == WriteInheritable ? TRUE : FALSE;
+    security.bInheritHandle       = options.readInheritable or options.writeInheritable ? TRUE : FALSE;
     security.lpSecurityDescriptor = nullptr;
 
     HANDLE pipeRead  = INVALID_HANDLE_VALUE;
     HANDLE pipeWrite = INVALID_HANDLE_VALUE;
 
-    if (CreatePipe(&pipeRead, &pipeWrite, &security, 0) == FALSE)
+    if (options.blocking == false)
     {
-        return Result::Error("PipeDescriptor::createPipe - ::CreatePipe failed");
+        char pipeName[64];
+        snprintf(pipeName, sizeof(pipeName), "\\\\.\\pipe\\SC-%lu-%llu", ::GetCurrentProcessId(), (intptr_t)this);
+
+        DWORD pipeFlags = PIPE_ACCESS_INBOUND | FILE_FLAG_FIRST_PIPE_INSTANCE | FILE_FLAG_OVERLAPPED;
+        DWORD pipeMode  = PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT;
+
+        pipeRead = ::CreateNamedPipeA(pipeName, pipeFlags, pipeMode, 1, 65536, 65536, 0, &security);
+        if (pipeRead == INVALID_HANDLE_VALUE)
+        {
+            return Result::Error("PipeDescriptor::createPipe - CreateNamedPipeW failed");
+        }
+        pipeWrite = ::CreateFileA(pipeName, GENERIC_WRITE | FILE_READ_ATTRIBUTES, 0, &security, OPEN_EXISTING,
+                                  FILE_FLAG_OVERLAPPED, nullptr);
+        if (pipeWrite == INVALID_HANDLE_VALUE)
+        {
+            ::CloseHandle(pipeRead);
+            return Result::Error("PipeDescriptor::createPipe - CreateFileW failed");
+        }
+        if (::ConnectNamedPipe(pipeRead, nullptr) == FALSE) // Connect the pipe immediately
+        {
+            if (GetLastError() != ERROR_PIPE_CONNECTED)
+            {
+                ::CloseHandle(pipeRead);
+                ::CloseHandle(pipeWrite);
+                return Result::Error("PipeDescriptor::createPipe - ConnectNamedPipe failed");
+            }
+        }
+    }
+    else
+    {
+        if (::CreatePipe(&pipeRead, &pipeWrite, &security, 0) == FALSE)
+        {
+            return Result::Error("PipeDescriptor::createPipe - ::CreatePipe failed");
+        }
     }
     SC_TRY(readPipe.assign(pipeRead));
     SC_TRY(writePipe.assign(pipeWrite));
 
     if (security.bInheritHandle)
     {
-        if (readFlag == ReadNonInheritable)
+        if (not options.readInheritable)
         {
-            SC_TRY_MSG(readPipe.setInheritable(false), "Cannot set read pipe inheritable");
+            if (::SetHandleInformation(pipeRead, HANDLE_FLAG_INHERIT, FALSE) == FALSE)
+            {
+                return Result::Error("Cannot set read pipe inheritable");
+            }
         }
-        if (writeFlag == WriteNonInheritable)
+        if (not options.writeInheritable)
         {
-            SC_TRY_MSG(writePipe.setInheritable(false), "Cannot set write pipe inheritable");
+            if (::SetHandleInformation(pipeWrite, HANDLE_FLAG_INHERIT, FALSE) == FALSE)
+            {
+                return Result::Error("Cannot set write pipe inheritable");
+            }
         }
     }
     return Result(true);
 }
 
 #else
-SC::Result SC::PipeDescriptor::createPipe(InheritableReadFlag readFlag, InheritableWriteFlag writeFlag)
+SC::Result SC::PipeDescriptor::createPipe(PipeOptions options)
 {
     int pipes[2];
     // TODO: Use pipe2 to set cloexec flags immediately
@@ -678,13 +665,22 @@ SC::Result SC::PipeDescriptor::createPipe(InheritableReadFlag readFlag, Inherita
     SC_TRY_MSG(writePipe.assign(pipes[1]), "Cannot assign write pipe");
     // On Posix by default descriptors are inheritable
     // https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873
-    if (readFlag == ReadNonInheritable)
+    if (options.readInheritable == false)
+    {
+        const Result pipeRes1 = FileDescriptor::Internal::setFileDescriptorFlags<FD_CLOEXEC>(pipes[0], true);
+        SC_TRY_MSG(pipeRes1, "Cannot set close on exec on read pipe");
+    }
+    if (options.writeInheritable == false)
     {
-        SC_TRY_MSG(readPipe.setInheritable(false), "Cannot set close on exec on read pipe");
+        const Result pipeRes2 = FileDescriptor::Internal::setFileDescriptorFlags<FD_CLOEXEC>(pipes[1], true);
+        SC_TRY_MSG(pipeRes2, "Cannot set close on exec on write pipe");
     }
-    if (writeFlag == WriteNonInheritable)
+    if (options.blocking == false)
     {
-        SC_TRY_MSG(writePipe.setInheritable(false), "Cannot set close on exec on write pipe");
+        const Result pipeRes1 = FileDescriptor::Internal::setFileStatusFlags<O_NONBLOCK>(pipes[0], true);
+        SC_TRY_MSG(pipeRes1, "Cannot set non-blocking flag on read");
+        const Result pipeRes2 = FileDescriptor::Internal::setFileStatusFlags<O_NONBLOCK>(pipes[1], true);
+        SC_TRY_MSG(pipeRes2, "Cannot set non-blocking flag on read");
     }
     return Result(true);
 }
diff --git a/Libraries/File/File.h b/Libraries/File/File.h
@@ -91,21 +91,6 @@ struct SC_COMPILER_EXPORT FileDescriptor : public UniqueHandle<detail::FileDescr
     /// @return Valid Result if file is opened successfully
     Result open(StringSpan path, FileOpen mode);
 
-    /// @brief Set blocking mode (read / write waiting for I/O). Can be set also during open with OpenOptions.
-    /// @param blocking `true` to set file to blocking mode
-    /// @return `true` if blocking mode has been changed successfully
-    Result setBlocking(bool blocking);
-
-    /// @brief Set inheritable flag (visibility to child processes). Can be set also during open with OpenOptions.
-    /// @param inheritable `true` to set file to blocking mode
-    /// @return `true` if blocking mode has been changed successfully
-    Result setInheritable(bool inheritable);
-
-    /// @brief Queries the inheritable state of this descriptor
-    /// @param hasValue will be set to true if the file descriptor has inheritable file set
-    /// @return Valid Result if the inheritable flag has been queried successfully
-    Result isInheritable(bool& hasValue) const;
-
     /// @brief Reads bytes at offset into user supplied span
     /// @param data Span of bytes where data should be written to
     /// @param actuallyRead A sub-span of data of the actually read bytes. A zero sized span means EOF.
@@ -197,34 +182,26 @@ struct SC_COMPILER_EXPORT FileDescriptor : public UniqueHandle<detail::FileDescr
 
   private:
     friend struct File;
+    friend struct PipeDescriptor;
     struct Internal;
 };
 
+struct SC_COMPILER_EXPORT PipeOptions
+{
+    bool readInheritable  = false; ///< Set to true to make the read side of the pipe visible to child processes
+    bool writeInheritable = false; ///< Set to true to make the write side of the pipe visible to child processes
+    bool blocking         = true;  ///< Set to false if pipe will be used for Async I/O (see @ref library_async)
+};
+
 /// @brief Read / Write pipe (Process stdin/stdout and IPC communication)
 struct SC_COMPILER_EXPORT PipeDescriptor
 {
-    /// @brief Specifies a flag for read side of the pipe
-    enum InheritableReadFlag
-    {
-        ReadInheritable,   ///< Requests read side of the pipe to be inheritable from child processes
-        ReadNonInheritable ///< Requests read side of the pipe not to be inheritable from child processes
-    };
-
-    /// @brief Specifies a flag for write side of the pipe
-    enum InheritableWriteFlag
-    {
-        WriteInheritable,   ///< Requests write side of the pipe to be inheritable from child processes
-        WriteNonInheritable ///< Requests write side of the pipe to be inheritable from child processes
-    };
     FileDescriptor readPipe;  ///< The read side of the pipe
     FileDescriptor writePipe; ///< The write side of the pipe
 
-    /// @brief Creates a Pipe. File descriptors are created with blocking mode enabled by default.
-    /// @param readFlag Specifies how the read side should be created
-    /// @param writeFlag Specifies how the write side should be created
+    /// @brief Creates a Pipe. File descriptors are created as blocking and non-inheritable
     /// @return Valid Result if pipe creation succeeded
-    Result createPipe(InheritableReadFlag  readFlag  = ReadNonInheritable,
-                      InheritableWriteFlag writeFlag = WriteNonInheritable);
+    Result createPipe(PipeOptions options = {});
 
     /// @brief Closes the pipe
     /// @return Valid Result if pipe destruction succeeded
diff --git a/Libraries/Process/Internal/ProcessPosix.inl b/Libraries/Process/Internal/ProcessPosix.inl
@@ -142,7 +142,7 @@ SC::Result SC::Process::launchImplementation()
 
     // Create a CLOSE_ON_EXEC pipe (non-inheritable) to communicate execvp failure
     PipeDescriptor pipe;
-    SC_TRY(pipe.createPipe(PipeDescriptor::ReadNonInheritable, PipeDescriptor::WriteNonInheritable));
+    SC_TRY(pipe.createPipe());
 
     // Fork child from parent here
     processID.pid = ::fork();
@@ -412,8 +412,8 @@ SC::Result SC::ProcessFork::resumeChildFork()
 SC::Result SC::ProcessFork::fork(State state)
 {
     // Create a CLOSE_ON_EXEC pipe (non-inheritable) to communicate with forked child
-    SC_TRY(parentToFork.createPipe(PipeDescriptor::ReadNonInheritable, PipeDescriptor::WriteNonInheritable));
-    SC_TRY(forkToParent.createPipe(PipeDescriptor::ReadNonInheritable, PipeDescriptor::WriteNonInheritable));
+    SC_TRY(parentToFork.createPipe());
+    SC_TRY(forkToParent.createPipe());
     int pid = ::fork();
     if (pid < 0)
     {
diff --git a/Libraries/Process/Internal/ProcessWindows.inl b/Libraries/Process/Internal/ProcessWindows.inl
@@ -396,8 +396,11 @@ SC::Result SC::ProcessFork::resumeChildFork()
 SC::Result SC::ProcessFork::fork(State state)
 {
     // We want this to be inheritable
-    SC_TRY(parentToFork.createPipe(PipeDescriptor::ReadInheritable, PipeDescriptor::WriteInheritable));
-    SC_TRY(forkToParent.createPipe(PipeDescriptor::ReadInheritable, PipeDescriptor::WriteInheritable));
+    PipeOptions options;
+    options.readInheritable  = true;
+    options.writeInheritable = true;
+    SC_TRY(parentToFork.createPipe(options));
+    SC_TRY(forkToParent.createPipe(options));
 
     RTL_USER_PROCESS_INFORMATION processInfo;
     // RTL_CLONE_PROCESS_FLAGS_CREATE_SUSPENDED could be used instead of parentToFork.readPipe.read
diff --git a/Libraries/Process/Process.cpp b/Libraries/Process/Process.cpp
@@ -61,7 +61,10 @@ SC::Result SC::ProcessChain::pipe(Process& process, const Span<const StringSpan>
     if (not processes.isEmpty())
     {
         PipeDescriptor chainPipe;
-        SC_TRY(chainPipe.createPipe(PipeDescriptor::ReadInheritable, PipeDescriptor::WriteInheritable));
+        PipeOptions    chainOptions;
+        chainOptions.writeInheritable = true;
+        chainOptions.readInheritable  = true;
+        SC_TRY(chainPipe.createPipe(chainOptions));
         SC_TRY(processes.back->stdOutFd.assign(move(chainPipe.writePipe)));
         SC_TRY(process.stdInFd.assign(move(chainPipe.readPipe)));
     }
@@ -135,7 +138,10 @@ SC::Result SC::Process::launch(const StdOut& stdOutput, const StdIn& stdInput, c
         case StdStream::Operation::ExternalPipe:
         case StdStream::Operation::GrowableBuffer:
         case StdStream::Operation::ReadableSpan: {
-            SC_TRY(pipe.createPipe(PipeDescriptor::ReadInheritable, PipeDescriptor::WriteNonInheritable));
+            PipeOptions pipeReadOptions;
+            pipeReadOptions.readInheritable  = true;
+            pipeReadOptions.writeInheritable = false;
+            SC_TRY(pipe.createPipe(pipeReadOptions));
             SC_TRY(fileDescriptor.assign(move(pipe.readPipe)));
         }
         break;
@@ -163,7 +169,10 @@ SC::Result SC::Process::launch(const StdOut& stdOutput, const StdIn& stdInput, c
         case StdStream::Operation::ExternalPipe:
         case StdStream::Operation::GrowableBuffer:
         case StdStream::Operation::WritableSpan: {
-            SC_TRY(pipe.createPipe(PipeDescriptor::ReadNonInheritable, PipeDescriptor::WriteInheritable));
+            PipeOptions pipeWriteOptions;
+            pipeWriteOptions.readInheritable  = false;
+            pipeWriteOptions.writeInheritable = true;
+            SC_TRY(pipe.createPipe(pipeWriteOptions));
             SC_TRY(fileDescriptor.assign(move(pipe.writePipe)));
         }
         break;
