notsan nomultifork

doc/glibc-cond-var-desync.txt

@@ -0,0 +1,141 @@
+glibc pthread_cond_t desync under checkpoint/restart, and how it was fixed
+=============================================================================
+
+This is the condition-variable counterpart to doc/glibc-sem-desync.txt (read
+that first for the general "glibc desync" mechanism -- packed nwaiters/G1/G2
+waiter-group bookkeeping going out of sync with the kernel's real futex wait
+state). This file covers the parts specific to pthread_cond_wait/signal/
+broadcast, including a point that isn't obvious by analogy with semaphores:
+application condition variables are normally single-process (pshared=0), so
+the "why does this need pshared=1" argument in glibc-sem-desync.txt does not
+directly apply here. The real mechanism is different, and narrower than a
+simple "lost wakeup."
+
+Why a normally single-process (pshared=0) CV can still be affected
+----------------------------------------------------------------------
+A target program's own pthread_cond_t is typically initialized with the
+default attribute -- PTHREAD_PROCESS_PRIVATE, pshared=0 -- since it is only
+ever used among that program's own threads. That is unlike child_side_sem,
+which had to be pshared=1 because it is shared between two genuinely
+different, concurrently-running OS processes (the verifier and the target).
+
+The relevant boundary for CVs is not "two concurrently-running processes";
+it is "two different incarnations of the same checkpointed memory". McMini's
+own architecture recreates the entire target process repeatedly: every
+explored DPOR branch is a brand-new OS process, produced by
+multithreaded_fork()/__clone(), all derived from the same original DMTCP
+checkpoint. A clone()-recreated thread in one of these new branch processes
+resumes exactly where it left off at checkpoint time -- including,
+potentially, being genuinely blocked at the kernel level inside
+pthread_cond_wait()'s real futex wait. If anything in that same new branch
+process were to touch the CV's real glibc state independently of that
+resurrected thread's still-live kernel wait, the same class of desync as
+the semaphore case could occur -- without pshared=1 ever being involved,
+because the "other party" here is not a second process but McMini's own
+restart-handling code running in that same, newly-created process.
+
+What tracing McMini's actual CV code found
+---------------------------------------------
+Unlike the child_side_sem case (confirmed via a direct FUTEX_WAKE(INT_MAX)
+probe), this vulnerability was found by reading mc_pthread_cond_wait(),
+mc_pthread_cond_signal(), mc_pthread_cond_broadcast(), mc_pthread_cond_init(),
+and mc_pthread_cond_destroy() in src/lib/wrappers.c end to end, across every
+libmcmini_mode. Two things stood out:
+
+1. mc_pthread_cond_wait() NEVER calls the real libpthread_cond_wait()/
+   libpthread_cond_timedwait() in any post-restart mode (DMTCP_RESTART_INTO_
+   BRANCH, DMTCP_RESTART_INTO_TEMPLATE, TARGET_BRANCH, TARGET_BRANCH_AFTER_
+   RESTART). In those modes, the wait is entirely simulated: the thread
+   posts a COND_ENQUEUE_TYPE and then a COND_WAIT_TYPE message via the
+   mailbox (thread_wake_scheduler_and_wait()/thread_handle_after_dmtcp_
+   restart(), which run on model_side_sem/child_side_sem -- the same
+   mailbox already hardened in doc/glibc-sem-desync.txt), unlocks/relocks the
+   real mutex around that handshake, and returns 0. This is true even in
+   classic (non-DMTCP) mode, not just under DMTCP -- it has nothing to do
+   with checkpoint/restart per se; it is simply how McMini simulates CV
+   waits for model checking in general.
+
+2. mc_pthread_cond_signal()/mc_pthread_cond_broadcast()/mc_pthread_cond_init()/
+   mc_pthread_cond_destroy() DID still call the real libpthread_cond_signal()/
+   broadcast()/init()/destroy() in those exact same post-restart modes, after
+   doing the same mailbox handshake. This was an asymmetry: the wait side had
+   already stopped depending on the real object's state, but the signal/
+   broadcast/init/destroy side kept touching it anyway.
+
+Why that asymmetry is dangerous
+-----------------------------------
+Since wait never blocks on the real CV once restart-related, unlike
+child_side_sem's straightforward lost-wakeup story, the risk here is not
+"the real signal quietly does nothing." It's the opposite and arguably
+worse: a clone()-resurrected thread can still be genuinely, kernel-level
+blocked inside a pre-restart real pthread_cond_timedwait() call (from
+RECORD mode, if the checkpoint happened to land mid-call). If a real
+signal/broadcast in a later branch reaches that resurrected thread, it
+wakes it for real -- letting it resume running real application code
+without ever going through the model checker's own scheduling. That breaks
+DPOR's core invariant that the verifier controls exactly one thread's next
+step at a time. A real, uncontrolled wakeup escaping the model checker is
+a more severe failure mode than a lost wakeup: instead of a hang, it is a
+thread running unsupervised.
+
+Note this is *not* about McMini reinitializing the application's own CV the
+way mc_runner_mailbox_init() reinitializes child_side_sem every branch:
+mc_pthread_cond_init() only touches the real object if the *application
+itself* calls pthread_cond_init() again post-restart (uncommon). The danger
+here is purely the wait/signal asymmetry above.
+
+The fix
+--------
+Removed the real libpthread_cond_signal()/libpthread_cond_broadcast()/
+libpthread_cond_init()/libpthread_cond_destroy() calls from all four
+post-restart cases (DMTCP_RESTART_INTO_BRANCH, DMTCP_RESTART_INTO_TEMPLATE,
+TARGET_BRANCH, TARGET_BRANCH_AFTER_RESTART) in src/lib/wrappers.c, replacing
+each with an unconditional `return 0;` after the existing mailbox handshake
+-- mirroring exactly what mc_pthread_cond_wait() already did in those same
+modes. Once wait never consults the real object's state, there is no
+correctness benefit to signal/broadcast/init/destroy touching it either;
+only risk.
+
+Functions changed (src/lib/wrappers.c), each in its
+DMTCP_RESTART_INTO_BRANCH/DMTCP_RESTART_INTO_TEMPLATE and
+TARGET_BRANCH/TARGET_BRANCH_AFTER_RESTART cases:
+
+  - mc_pthread_cond_init()      -- no longer calls libpthread_cond_init().
+  - mc_pthread_cond_signal()    -- no longer calls libpthread_cond_signal().
+  - mc_pthread_cond_broadcast() -- no longer calls libpthread_cond_broadcast().
+  - mc_pthread_cond_destroy()   -- no longer calls libpthread_cond_destroy().
+
+RECORD/PRE_CHECKPOINT mode is untouched: those calls still execute for
+real. That is safe and necessary, because RECORD mode is one continuous
+execution before any checkpoint has happened -- nothing external
+reinitializes the CV's memory mid-flight there, unlike the restart-time
+scenario above.
+
+Verification
+-------------
+- Rebuilt and confirmed classic (non-DMTCP) mode's cv-test example produces
+  byte-for-byte-equivalent output before and after the fix (modulo harmless
+  debug-log pid/interleaving differences) -- expected, since TARGET_BRANCH
+  mode's real signal/broadcast/init/destroy calls were already functionally
+  irrelevant there, exactly as wait's real call already was.
+- cv-hello-world's run time (and its eventual completion behavior within a
+  20s window) was unchanged before/after the fix, ruling out a new hang.
+- No DMTCP+TSan condition-variable target exists yet in this repo's own
+  CMake build to reproduce the resurrected-thread-woken-for-real scenario
+  end to end (unlike producer-consumer-tsan for the semaphore fix); this fix
+  is based on a full trace of the wrapper code's control flow across every
+  libmcmini_mode, not on reproducing the failure live.
+
+Related, separate observation (not part of this fix)
+----------------------------------------------------------
+mc_pthread_cond_wait()'s RECORD-mode retry loop declares
+`struct timespec wait_time = {.tv_sec = 2, .tv_nsec = 0};` and passes it
+directly to libpthread_cond_timedwait() as an absolute deadline. pthread_
+cond_timedwait()'s abstime is interpreted as an absolute point on the CV's
+associated clock (CLOCK_REALTIME by default) -- so this reads as "2 seconds
+past the epoch," not "2 seconds from now," meaning the call would return
+ETIMEDOUT almost immediately rather than genuinely blocking for up to 2
+seconds. This looks like a pre-existing issue independent of the desync
+fix above (mc_pthread_join_impl's analogous RECORD-mode retry loop uses the
+same `{2, 0}` pattern for pthread_timedjoin_np). Worth checking separately;
+out of scope here.

doc/glibc-sem-desync.txt

@@ -0,0 +1,147 @@
+glibc sem_t "nwaiters" desync, and why child_side_sem is a raw futex instead
+=============================================================================
+
+What "glibc desync" means
+--------------------------
+glibc's NPTL sem_t packs two things into one 64-bit atomic word: the actual
+semaphore count, and `nwaiters` -- a count of how many threads are currently
+blocked inside sem_wait(). sem_post() uses `nwaiters` purely as an
+optimization: it increments the count, then checks `nwaiters`, and only
+makes the FUTEX_WAKE syscall if `nwaiters > 0`. If `nwaiters == 0`, it
+assumes nobody's waiting and skips the syscall entirely.
+
+"Desync" means `nwaiters` (glibc's userspace belief about who's waiting) no
+longer matches the kernel's actual futex wait queue for that address. When
+that happens, sem_post() can wrongly conclude "nobody is waiting" and
+silently skip the wake -- even though a real thread is genuinely parked in
+the kernel on that exact word. That is a lost wakeup, and the thread hangs
+forever.
+
+Does this happen only across checkpoint-restart, or anywhere?
+---------------------------------------------------------------
+Only in scenarios like McMini's, not in ordinary programs.
+
+In normal execution, `nwaiters` can never drift from reality, because the
+only code that ever touches it is sem_wait()/sem_post() themselves, via
+atomic read-modify-write on that same packed word -- it is a closed,
+self-consistent system. The only way to break it is for something *outside*
+the semaphore's own API to reset that memory (sem_destroy() + sem_init(), or
+an equivalent memset) while a thread is still validly blocked on it -- which
+POSIX explicitly documents as undefined behavior ("it is safe to destroy a
+semaphore only once no thread is blocked on it").
+
+Ordinary programs don't hit this because they only ever destroy a semaphore
+once they've confirmed nothing is using it. McMini's situation is different:
+DMTCP's checkpoint/restart transparently preserves a thread's real
+kernel-level "I'm blocked in this exact FUTEX_WAIT" state across the
+restart (that's just how checkpointing a blocked syscall works), while
+McMini's own code (mc_runner_mailbox_init()/mc_runner_mailbox_destroy())
+separately, explicitly reinitializes that same shared-memory semaphore for
+the new branch -- invisible to, and inconsistent with, a thread that (from
+the kernel's point of view) never actually left its old wait.
+
+CRIU's own documentation names this exact rule: everything sharing a
+futex/shared-memory region must be checkpointed and restored together as
+one atomic unit. McMini's architecture -- a permanent, never-checkpointed
+verifier sharing memory with a repeatedly-restarted target -- inherently
+breaks that rule.
+
+So: this is not a general glibc footgun waiting anywhere in a normal
+program; it is specific to externally reinitializing a semaphore's memory
+while a checkpoint/restart mechanism has silently kept a thread genuinely
+blocked on it underneath. Take away either half (no checkpoint/restart, or
+no external reinitialization) and it cannot occur.
+
+The fix
+--------
+Commit 1a4b3d9 ("Replace child_side_sem's glibc sem_t with a plain futex
+word") replaces `child_side_sem` -- the mailbox semaphore a DMTCP-restored
+target thread waits on, posted by the verifier -- with a bare futex word
+that has no separate userspace bookkeeping to desync in the first place.
+
+Files and functions:
+
+  include/mcmini/real_world/mailbox/runner_mailbox.h
+    - `child_side_sem` changed from `sem_t` to `uint32_t`.
+
+  src/common/runner_mailbox.c
+    - `mc_futex()`               -- thin wrapper around syscall(SYS_futex, ...).
+    - `mc_raw_sem_wait()`        -- replaces sem_wait() on child_side_sem: spins
+                                    on an atomic compare-and-swap against the
+                                    counter, blocking via FUTEX_WAIT only when
+                                    the counter is 0.
+    - `mc_raw_sem_post()`        -- replaces sem_post() on child_side_sem:
+                                    atomically increments the counter, then
+                                    calls FUTEX_WAKE *unconditionally* -- no
+                                    "is anyone really waiting" check, so there
+                                    is nothing to desync.
+    - `mc_wait_for_scheduler()`  -- now calls mc_raw_sem_wait().
+    - `mc_wake_thread()`         -- now calls mc_raw_sem_post().
+    - `mc_runner_mailbox_init()`/`mc_runner_mailbox_destroy()` -- initialize/
+      no-op-destroy the futex word directly instead of calling sem_init()/
+      sem_destroy() on it.
+
+`model_side_sem` (the verifier waits, the target posts) is untouched and
+remains a real sem_t: the verifier process is never checkpointed, so its
+side of the bookkeeping can never desync.
+
+Not yet fixed
+---------------
+Condition variables (pthread_cond_wait/pthread_cond_signal) have the same
+class of vulnerability via glibc's G1/G2 waiter-group bookkeeping, for the
+same reason (checkpoint/restart + externally-managed reinitialization). An
+analogous fix has not yet been applied there.
+
+Does this require pshared=1, or can it happen with pshared=0 too?
+---------------------------------------------------------------------
+`pshared` is the second parameter of sem_init() (`int sem_init(sem_t *sem,
+int pshared, unsigned int value)`): pshared=0 means the semaphore may only
+be used among threads of the single process that created it; pshared=1
+means it may be shared across process boundaries (typically by placing it
+in memory obtained via shm_open()/mmap(), as McMini does here).
+
+This bug requires pshared=1. It cannot happen with pshared=0, and the
+reason follows directly from the mechanism above: the desync needs an
+entity *outside the checkpointed unit* to reinitialize the semaphore's
+memory while a thread inside that unit is still genuinely blocked on it. In
+McMini's architecture that outside entity is the verifier (mcmini) -- a
+separate, permanent, never-checkpointed process -- while the target (with
+the blocked thread) gets checkpoint/restarted. DMTCP transparently
+preserves the target's kernel-level block across the restart, but the
+verifier's reinitialization of that shared memory has no way to know about,
+or wait for, that survival, because it is not part of the same checkpoint
+image at all.
+
+If pshared=0, the semaphore is only valid for use among threads of a
+single process. DMTCP checkpoints and restores that whole process --
+every thread, and all of the semaphore's own memory (nwaiters and value
+together) -- as one atomic, consistent snapshot. There is no outsider who
+could reinitialize the memory independently of the blocked thread's
+restore, because everything that touches it moves through the
+checkpoint/restart together.
+
+Two more reasons this is specifically a pshared=1 problem:
+
+  1. Using a pshared=0 semaphore across two processes is undefined
+     behavior under POSIX regardless of checkpointing -- Linux's NPTL
+     implementation uses FUTEX_PRIVATE_FLAG for private (pshared=0) futex
+     operations, keyed off the process's own memory descriptor, which
+     specifically assumes same-process access. "verifier process + target
+     process share a pshared=0 semaphore" is not a valid configuration to
+     begin with.
+  2. model_side_sem (still a real sem_t, pshared=1, in this same mailbox)
+     is the control case that proves the point: it is just as
+     cross-process-shared as child_side_sem was, but it is fine, because
+     the verifier's side of it is never checkpointed -- only the target
+     side (posting) is. The bug needs the *waiting* side to be the one
+     that gets checkpoint/restarted while the memory gets reinitialized
+     out from under it; that combination cannot arise for a purely
+     intra-process, pshared=0 semaphore.
+
+Separately: an ordinary program could still hit undefined behavior by
+calling sem_destroy() on any semaphore -- pshared=0 or 1 -- while a thread
+is genuinely blocked on it. That is always illegal per POSIX. But that is
+a plain application bug, not "the checkpoint/restart desync": the
+McMini-specific failure mode is that the reinitialization happens from a
+vantage point that literally cannot see the blocked thread at all, which
+is only possible across the process boundary that pshared=1 implies.