Make read() and write() signal handling atomic

You would think this is an important bug fix, but unfortunately all UNIX
implementations I've evaluated have a bug in read that causes signals to
not be handled atomically. The only exception is the latest iteration of
Cosmopolitan's read/write polyfill on Windows, which is somewhat ironic.

libc/calls/checkcancel.c

@@ -21,12 +21,16 @@
 #include "libc/intrin/weaken.h"
 #include "libc/thread/posixthread.internal.h"
 
+textwindows bool _is_canceled(void) {
+  struct PosixThread *pt;
+  return _weaken(_pthread_cancel_ack) && (pt = _pthread_self()) &&
+         atomic_load_explicit(&pt->pt_canceled, memory_order_acquire) &&
+         !(pt->pt_flags & PT_NOCANCEL);
+}
+
 textwindows int _check_cancel(void) {
-  if (_weaken(_pthread_cancel_ack) &&  //
-      _pthread_self() && !(_pthread_self()->pt_flags & PT_NOCANCEL) &&
-      atomic_load_explicit(&_pthread_self()->pt_canceled,
-                           memory_order_acquire)) {
+  if (_is_canceled())
+    // once acknowledged _is_canceled() will return false
     return _weaken(_pthread_cancel_ack)();
-  }
   return 0;
 }

libc/calls/internal.h

@@ -43,6 +43,7 @@ forceinline bool __isfdkind(int fd, int kind) {
 
 int _check_signal(bool);
 int _check_cancel(void);
+bool _is_canceled(void);
 int sys_close_nt(int, int);
 int _park_norestart(uint32_t, uint64_t);
 int _park_restartable(uint32_t, uint64_t);

libc/calls/islinuxmodern.c

@@ -0,0 +1,27 @@
+/*-*- mode:c;indent-tabs-mode:nil;c-basic-offset:2;tab-width:8;coding:utf-8 -*-│
+│ vi: set et ft=c ts=2 sts=2 sw=2 fenc=utf-8                               :vi │
+╞══════════════════════════════════════════════════════════════════════════════╡
+│ Copyright 2024 Justine Alexandra Roberts Tunney                              │
+│                                                                              │
+│ Permission to use, copy, modify, and/or distribute this software for         │
+│ any purpose with or without fee is hereby granted, provided that the         │
+│ above copyright notice and this permission notice appear in all copies.      │
+│                                                                              │
+│ THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL                │
+│ WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED                │
+│ WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE             │
+│ AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL         │
+│ DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR        │
+│ PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER               │
+│ TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR             │
+│ PERFORMANCE OF THIS SOFTWARE.                                                │
+╚─────────────────────────────────────────────────────────────────────────────*/
+#include "libc/calls/calls.h"
+#include "libc/calls/syscall-sysv.internal.h"
+#include "libc/calls/syscall_support-sysv.internal.h"
+#include "libc/dce.h"
+#include "libc/errno.h"
+
+bool IsLinuxModern(void) {
+  return IsLinux() && sys_close_range(-1, -2, 0) == -1 && errno == EINVAL;
+}

libc/calls/readwrite-nt.c

@@ -16,21 +16,25 @@
 │ TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR             │
 │ PERFORMANCE OF THIS SOFTWARE.                                                │
 ╚─────────────────────────────────────────────────────────────────────────────*/
+#include "libc/assert.h"
 #include "libc/calls/createfileflags.internal.h"
 #include "libc/calls/internal.h"
 #include "libc/calls/sig.internal.h"
 #include "libc/calls/struct/sigset.h"
 #include "libc/calls/syscall_support-nt.internal.h"
+#include "libc/errno.h"
 #include "libc/intrin/fds.h"
 #include "libc/intrin/weaken.h"
 #include "libc/nt/enum/filetype.h"
 #include "libc/nt/errors.h"
 #include "libc/nt/events.h"
 #include "libc/nt/files.h"
+#include "libc/nt/process.h"
 #include "libc/nt/runtime.h"
 #include "libc/nt/struct/overlapped.h"
 #include "libc/nt/synchronization.h"
 #include "libc/nt/thread.h"
+#include "libc/sock/internal.h"
 #include "libc/stdio/sysparam.h"
 #include "libc/sysv/consts/sicode.h"
 #include "libc/sysv/errfuns.h"
@@ -47,8 +51,6 @@ sys_readwrite_nt(int fd, void *data, size_t size, ssize_t offset,
                  int64_t handle, sigset_t waitmask,
                  bool32 ReadOrWriteFile(int64_t, void *, uint32_t, uint32_t *,
                                         struct NtOverlapped *)) {
-  int sig;
-  uint32_t exchanged;
   struct Fd *f = g_fds.p + fd;
 
   // pread() and pwrite() perform an implicit lseek() operation, so
@@ -60,105 +62,153 @@ sys_readwrite_nt(int fd, void *data, size_t size, ssize_t offset,
   if (pwriting && !seekable)
     return espipe();
 
-  // determine if we need to lock a file descriptor across processes
+  // determine if we need to lock file descriptor
   bool locked = isdisk && !pwriting && f->cursor;
-  if (locked)
-    __cursor_lock(f->cursor);
 
-RestartOperation:
-  // when a file is opened in overlapped mode win32 requires that we
-  // take over full responsibility for managing our own file pointer
-  // which is fine, because the one win32 has was never very good in
-  // the sense that it behaves so differently from linux, that using
-  // win32 i/o required more compatibilty toil than doing it by hand
-  if (!pwriting) {
-    if (seekable && f->cursor) {
-      offset = f->cursor->shared->pointer;
-    } else {
-      offset = 0;
-    }
-  }
+  for (;;) {
+    int got_sig = 0;
+    bool got_eagain = false;
+    uint32_t other_error = 0;
 
-  bool eagained = false;
-  // check for signals and cancelation
-  if (_check_cancel() == -1) {
+    // create event handle for overlapped i/o
+    intptr_t event;
+    if (!(event = CreateEvent(0, 1, 0, 0)))
+      return __winerr();
+
+    // ensure iops are ordered across threads and processes if seeking
     if (locked)
-      __cursor_unlock(f->cursor);
-    return -1;  // ECANCELED
-  }
-  if (_weaken(__sig_get) && (sig = _weaken(__sig_get)(waitmask)))
-    goto HandleInterrupt;
+      __cursor_lock(f->cursor);
 
-  // signals have already been fully blocked by caller
-  // perform i/o operation with atomic signal/cancel checking
-  struct NtOverlapped overlap = {.hEvent = CreateEvent(0, 1, 0, 0),
-                                 .Pointer = offset};
-  bool32 ok = ReadOrWriteFile(handle, data, size, 0, &overlap);
-  if (!ok && GetLastError() == kNtErrorIoPending) {
-    // win32 says this i/o operation needs to block
-    if (f->flags & _O_NONBLOCK) {
-      // abort the i/o operation if file descriptor is in non-blocking mode
-      CancelIoEx(handle, &overlap);
-      eagained = true;
-    } else {
-      // wait until i/o either completes or is canceled by another thread
-      // we avoid a race condition by having a second mask for unblocking
-      struct PosixThread *pt;
-      pt = _pthread_self();
-      pt->pt_blkmask = waitmask;
-      pt->pt_iohandle = handle;
-      pt->pt_ioverlap = &overlap;
-      atomic_store_explicit(&pt->pt_blocker, PT_BLOCKER_IO,
-                            memory_order_release);
-      WaitForSingleObject(overlap.hEvent, -1u);
-      atomic_store_explicit(&pt->pt_blocker, 0, memory_order_release);
+    // when a file is opened in overlapped mode win32 requires that we
+    // take over full responsibility for managing our own file pointer
+    // which is fine, because the one win32 has was never very good in
+    // the sense that it behaves so differently from linux, that using
+    // win32 i/o required more compatibilty toil than doing it by hand
+    if (!pwriting) {
+      if (seekable && f->cursor) {
+        offset = f->cursor->shared->pointer;
+      } else {
+        offset = 0;
+      }
     }
-    ok = true;
-  }
-  if (ok)
-    ok = GetOverlappedResult(handle, &overlap, &exchanged, true);
-  CloseHandle(overlap.hEvent);
 
-  // if i/o succeeded then return its result
-  if (ok) {
-    if (!pwriting && seekable && f->cursor)
-      f->cursor->shared->pointer = offset + exchanged;
-    if (locked)
-      __cursor_unlock(f->cursor);
-    return exchanged;
-  }
+    // initiate asynchronous i/o operation with win32
+    struct NtOverlapped overlap = {.hEvent = event, .Pointer = offset};
+    bool32 ok = ReadOrWriteFile(handle, data, size, 0, &overlap);
+    if (!ok && GetLastError() == kNtErrorIoPending) {
+      if (f->flags & _O_NONBLOCK) {
+        // immediately back out of blocking i/o if non-blocking
+        CancelIoEx(handle, &overlap);
+        got_eagain = true;
+      } else {
+        // atomic block on i/o completion, signal, or cancel
+        // it's not safe to acknowledge cancelation from here
+        // it's not safe to call any signal handlers from here
+        intptr_t sem;
+        if ((sem = CreateSemaphore(0, 0, 1, 0))) {
+          // installing semaphore before sig get makes wait atomic
+          struct PosixThread *pt = _pthread_self();
+          pt->pt_semaphore = sem;
+          pt->pt_blkmask = waitmask;
+          atomic_store_explicit(&pt->pt_blocker, PT_BLOCKER_SEM,
+                                memory_order_release);
+          if (_is_canceled()) {
+            CancelIoEx(handle, &overlap);
+          } else if (_weaken(__sig_get) &&
+                     (got_sig = _weaken(__sig_get)(waitmask))) {
+            CancelIoEx(handle, &overlap);
+          } else {
+            intptr_t hands[] = {event, sem};
+            uint32_t wi = WaitForMultipleObjects(2, hands, 0, -1u);
+            if (wi == 1) {  // semaphore was signaled by signal enqueue
+              CancelIoEx(handle, &overlap);
+              if (_weaken(__sig_get))
+                got_sig = _weaken(__sig_get)(waitmask);
+            } else if (wi == -1u) {
+              other_error = GetLastError();
+              CancelIoEx(handle, &overlap);
+            }
+          }
+          atomic_store_explicit(&pt->pt_blocker, 0, memory_order_release);
+          CloseHandle(sem);
+        } else {
+          other_error = GetLastError();
+          CancelIoEx(handle, &overlap);
+        }
+      }
+      ok = true;
+    }
+    uint32_t exchanged = 0;
+    if (ok)
+      ok = GetOverlappedResult(handle, &overlap, &exchanged, true);
+    uint32_t io_error = GetLastError();
+    CloseHandle(event);
 
-  // only raise EINTR or EAGAIN if I/O got canceled
-  if (GetLastError() == kNtErrorOperationAborted) {
-    // raise EAGAIN if it's due to O_NONBLOCK mmode
-    if (eagained) {
+    // check if i/o completed
+    // this could forseeably happen even if CancelIoEx was called
+    if (ok) {
+      if (!pwriting && seekable && f->cursor)
+        f->cursor->shared->pointer = offset + exchanged;
       if (locked)
         __cursor_unlock(f->cursor);
+      if (got_sig)  // swallow dequeued signal
+        _weaken(__sig_relay)(got_sig, SI_KERNEL, waitmask);
+      return exchanged;
+    }
+
+    // it's now safe to unlock cursor
+    if (locked)
+      __cursor_unlock(f->cursor);
+
+    // check if i/o failed
+    if (io_error != kNtErrorOperationAborted) {
+      if (got_sig)  // swallow dequeued signal
+        _weaken(__sig_relay)(got_sig, SI_KERNEL, waitmask);
+      // read() and write() have different error paths
+      SetLastError(io_error);
+      return -2;
+    }
+
+    // the i/o operation was successfully canceled
+    if (got_eagain) {
+      unassert(!got_sig);
       return eagain();
     }
-    // otherwise it must be due to a kill() via __sig_cancel()
-    if (_weaken(__sig_relay) && (sig = _weaken(__sig_get)(waitmask))) {
-    HandleInterrupt:
-      if (locked)
-        __cursor_unlock(f->cursor);
-      int handler_was_called = _weaken(__sig_relay)(sig, SI_KERNEL, waitmask);
-      if (_check_cancel() == -1)
-        return -1;  // possible if we SIGTHR'd
-      if (locked)
-        __cursor_lock(f->cursor);
-      // read() is @restartable unless non-SA_RESTART hands were called
-      if (!(handler_was_called & SIG_HANDLED_NO_RESTART))
-        goto RestartOperation;
-    }
-    if (locked)
-      __cursor_unlock(f->cursor);
-    return eintr();
-  }
 
-  // read() and write() have generally different error-handling paths
-  if (locked)
-    __cursor_unlock(f->cursor);
-  return -2;
+    // it's now reasonable to report semaphore creation error
+    if (other_error) {
+      unassert(!got_sig);
+      errno = __dos2errno(other_error);
+      return -1;
+    }
+
+    // check for thread cancelation and acknowledge
+    if (_check_cancel() == -1)
+      return -1;
+
+    // if signal module has been linked, then
+    if (_weaken(__sig_get)) {
+
+      // gobble up all unmasked pending signals
+      // it's now safe to recurse into signal handlers
+      int handler_was_called = 0;
+      do {
+        if (got_sig)
+          handler_was_called |=
+              _weaken(__sig_relay)(got_sig, SI_KERNEL, waitmask);
+      } while ((got_sig = _weaken(__sig_get)(waitmask)));
+
+      // check if SIGTHR handler was called
+      if (_check_cancel() == -1)
+        return -1;
+
+      // check if signal handler without SA_RESTART was called
+      if (handler_was_called & SIG_HANDLED_NO_RESTART)
+        return eintr();
+    }
+
+    // otherwise try the i/o operation again
+  }
 }
 
 #endif /* __x86_64__ */

libc/calls/sigtimedwait-nt.c

@@ -32,6 +32,7 @@
 #include "libc/sysv/consts/sig.h"
 #include "libc/sysv/errfuns.h"
 #include "libc/thread/posixthread.internal.h"
+#ifdef __x86_64__
 
 textwindows static int sys_sigtimedwait_nt_check(sigset_t syncsigs,
                                                  siginfo_t *opt_info,
@@ -111,3 +112,5 @@ textwindows int sys_sigtimedwait_nt(const sigset_t *set, siginfo_t *opt_info,
   ALLOW_SIGNALS;
   return rc;
 }
+
+#endif /* __x86_64__ */

libc/calls/write-nt.c

@@ -53,20 +53,17 @@ static textwindows ssize_t sys_write_nt_impl(int fd, void *data, size_t size,
   bool isconsole = f->kind == kFdConsole;
 
   // not implemented, XNU returns eperm();
-  if (f->kind == kFdDevRandom) {
+  if (f->kind == kFdDevRandom)
     return eperm();
-  }
 
   // determine win32 handle for writing
   int64_t handle = f->handle;
-  if (isconsole && _weaken(GetConsoleOutputHandle)) {
+  if (isconsole && _weaken(GetConsoleOutputHandle))
     handle = _weaken(GetConsoleOutputHandle)();
-  }
 
   // intercept ansi tty configuration sequences
-  if (isconsole && _weaken(GetConsoleOutputHandle)) {
+  if (isconsole && _weaken(GetConsoleOutputHandle))
     _weaken(InterceptTerminalCommands)(data, size);
-  }
 
   // perform heavy lifting
   ssize_t rc;