notsan nomultifork nocondvar

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.

include/dmtcp.h

@@ -16,6 +16,8 @@
 #define DMTCP_H
 
 #include <netinet/ip.h>
+#include <assert.h>
+#include <pthread.h>
 #include <stddef.h>
 #include <stdio.h>
 #include <sys/socket.h>
@@ -43,8 +45,8 @@
 # define EXTERNC
 #endif // ifdef __cplusplus
 
-/* Define to the version of this package. */
-#define DMTCP_PLUGIN_API_VERSION "3"
+/* Bump when DmtcpPluginDescriptor_t changes ABI. */
+#define DMTCP_PLUGIN_API_VERSION "4"
 
 #ifdef __cplusplus
 namespace dmtcp {
@@ -145,6 +147,11 @@ typedef union _DmtcpEventData_t {
   struct {
     char *path;
   } realToVirtualPath, virtualToRealPath;
+
+  struct {
+    pthread_t pthread;
+    pid_t tid;
+  } pthreadInfo;
 } DmtcpEventData_t;
 
 typedef void (*HookFunctionPtr_t)(DmtcpEvent_t, DmtcpEventData_t *);
@@ -238,12 +245,78 @@ void dmtcp_initialize_plugin(void) __attribute((weak));
 typedef struct DmtcpUniqueProcessId {
   uint64_t _hostid;  // gethostid()
   uint64_t _time; // time()
-  pid_t _pid; // getpid()
+
+  union {
+    pid_t _pid; // getpid()
+    int32_t _;
+  };
+
   uint32_t _computation_generation; // computationGeneration()
 } DmtcpUniqueProcessId;
 
 int dmtcp_unique_pids_equal(DmtcpUniqueProcessId a, DmtcpUniqueProcessId b);
 
+typedef struct DmtcpInfo {
+  int argc;
+  const char **argv;
+} DmtcpInfo;
+
+enum ElfType {
+  Elf_32,
+  Elf_64
+};
+
+typedef struct {
+  uint64_t startAddr;
+  uint64_t endAddr;
+} MemRegion;
+
+typedef void (*PostRestartFnPtr_t)(double, int);
+#define DMTCP_CKPT_SIGNATURE "DMTCP_CHECKPOINT_IMAGE_v4.0\n"
+typedef struct {
+  char ckptSignature[32];
+
+  DmtcpUniqueProcessId upid;
+  DmtcpUniqueProcessId uppid;
+  DmtcpUniqueProcessId compGroup;
+
+  pid_t pid;
+  pid_t ppid;
+  pid_t sid;
+  pid_t gid;
+  pid_t fgid;
+  uint32_t isRootOfProcessTree;
+
+  uint32_t numPeers;
+  uint32_t elfType;
+
+  uint64_t clock_gettime_offset;
+  uint64_t getcpu_offset;
+  uint64_t gettimeofday_offset;
+  uint64_t time_offset;
+
+  // Reserve 3 * 30MB for restore buffer.
+#define RESTORE_BUF_TOTAL_SIZE (90 * 1024 * 1024)
+  MemRegion restoreBuf;
+
+  MemRegion vdso;
+  MemRegion vvar;
+  MemRegion vvarVClock;
+
+  uint64_t savedBrk;
+  uint64_t endOfStack;
+
+  uint64_t postRestartAddr;
+  //void (*post_restart)(double, int);
+
+  char procname[1024];
+  char procSelfExe[1024];
+
+  char padding[1792];
+} DmtcpCkptHeader;
+
+static_assert(sizeof(DmtcpCkptHeader) == 4096, "DmtcpCkptHeader must be 4096 bytes");
+
 // FIXME:
 // If a plugin is not compiled with defined(__PIC__) and we can verify
 // that we're using DMTCP (environment variables), and dmtcp_is_enabled
@@ -329,8 +402,6 @@ const char *dmtcp_get_ckpt_files_subdir(void);
 int dmtcp_should_ckpt_open_files(void);
 int dmtcp_allow_overwrite_with_ckpted_files(void);
 int dmtcp_skip_truncate_file_at_restart(const char* path);
-void dmtcp_set_restore_buf_addr(void *new_addr, uint64_t len);
-uint64_t dmtcp_restore_buf_len();
 
 int dmtcp_get_ckpt_signal(void);
 const char *dmtcp_get_uniquepid_str(void) __attribute__((weak));
@@ -428,13 +499,32 @@ int dmtcp_protected_environ_fd(void);
  *  discovers a pid without going through a system call (e.g., through
  *  the proc filesystem), use this to virtualize the pid.
  */
-pid_t dmtcp_real_to_virtual_pid(pid_t realPid) __attribute((weak));
-pid_t dmtcp_virtual_to_real_pid(pid_t virtualPid) __attribute((weak));
-
+pid_t dmtcp_pid_real_to_virtual(pid_t realPid) __attribute((weak));
+pid_t dmtcp_pid_virtual_to_real(pid_t virtualPid) __attribute((weak));
+
+// Returns 1 if virtual_tid names TSAN's own background thread, else 0.
+int dmtcp_is_tsan_background_thread(int virtual_tid) __attribute((weak));
+#define dmtcp_is_tsan_background_thread(virtual_tid) \
+  (dmtcp_is_tsan_background_thread ? \
+   dmtcp_is_tsan_background_thread(virtual_tid) : 0)
+
+// Tells DMTCP that this restart is one-shot: the checkpoint thread should
+// not resume its usual sleep-checkpoint-resume loop (i.e., it should not
+// wait for a future checkpoint request), since no such request is ever
+// coming. Must be called on the checkpoint thread itself, from a
+// DMTCP_EVENT_RESTART hook (before that event's plugin dispatch returns).
+void dmtcp_skip_post_restart_checkpoint_loop(void) __attribute((weak));
+#define dmtcp_skip_post_restart_checkpoint_loop() \
+  (dmtcp_skip_post_restart_checkpoint_loop ? \
+   dmtcp_skip_post_restart_checkpoint_loop() : (void)0)
+
+// McMini helpers: translate pids only when running under DMTCP.
+// (DMTCP plugin API v4 renamed dmtcp_{real_to_virtual,virtual_to_real}_pid to
+// dmtcp_pid_{real_to_virtual,virtual_to_real}.)
 #define mcmini_virtual_pid(PID) \
-  (dmtcp_is_enabled() ? dmtcp_real_to_virtual_pid((PID)) : (PID))
+  (dmtcp_is_enabled() ? dmtcp_pid_real_to_virtual((PID)) : (PID))
 #define mcmini_real_pid(PID) \
-  (dmtcp_is_enabled() ? dmtcp_virtual_to_real_pid((PID)) : (PID))
+  (dmtcp_is_enabled() ? dmtcp_pid_virtual_to_real((PID)) : (PID))
 
 // bq_file -> "batch queue file"; used only by batch-queue plugin
 int dmtcp_is_bq_file(const char *path) __attribute((weak));
@@ -445,7 +535,7 @@ int dmtcp_bq_restore_file(const char *path,
                           int type) __attribute((weak));
 
 /*  These next two functions are defined in contrib/ckptfile/ckptfile.cpp
- *  But they are currently used only in src/plugin/ipc/file/fileconnection.cpp
+ *  But they are currently used only in src/plugin/file/fileconnection.cpp
  *    and in a trivial fashion.  These are intended for future extensions.
  */
 int dmtcp_must_ckpt_file(const char *path) __attribute((weak));

include/mcmini/model/objects/mutex.hpp

@@ -1,5 +1,6 @@
 #pragma once
 
+#include "mcmini/defines.h"
 #include "mcmini/misc/extensions/unique_ptr.hpp"
 #include "mcmini/model/visible_object_state.hpp"
 
@@ -20,9 +21,16 @@ struct mutex : public model::visible_object_state {
   mutex() = default;
   ~mutex() = default;
   mutex(const mutex &) = default;
-  mutex(state s) : current_state(s) {}
-  mutex(state s, pthread_mutex_t* loc) : current_state(s), location(loc) {}
-  mutex(state s, pthread_mutex_t* loc, runner_id_t tid): current_state(s), location(loc), owner(tid) {}
+  // `location` has no default member initializer (unlike, say,
+  // condition_variable's `policy`), so it must always be passed explicitly
+  // -- a mutex without its real address is meaningless, and every
+  // mutex_lock/unlock faithfully forwards whatever get_location() returns,
+  // so an uninitialized one corrupts every subsequent state derived from
+  // it (see doc history around commit 9bd9ecf). `tid` defaults to
+  // RID_INVALID: "unlocked, no owner" is exactly what callers that omit it
+  // (mutex_init, condition_variable_enqueue_thread) mean.
+  mutex(state s, pthread_mutex_t* loc, runner_id_t tid = RID_INVALID)
+      : current_state(s), location(loc), owner(tid) {}
 
   // ---- State Observation --- //
   bool operator==(const mutex &other) const {

include/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hpp

@@ -40,9 +40,15 @@ struct condition_variable_enqueue_thread : public model::transition{
 
     cv->get_policy()->add_waiter_with_state(executor, CV_WAITING);
     const int new_waiting_count = cv->get_policy()->return_wait_queue().size();
-    
+
     s.add_state_for_obj(cond_id, new condition_variable(condition_variable::cv_waiting, executor, m->get_location(), new_waiting_count));
-    s.add_state_for_obj(mutex_id, new mutex(mutex::unlocked));
+    // Preserve the mutex's location: mutex(state) (1-arg) has no default
+    // member initializer for `location` (unlike condition_variable's
+    // policy), so it's left completely uninitialized -- every later
+    // mutex_lock/unlock just faithfully forwards whatever garbage this
+    // leaves behind via ms->get_location(), corrupting the mutex's identity
+    // for the rest of the run.
+    s.add_state_for_obj(mutex_id, new mutex(mutex::unlocked, m->get_location()));
     return status::exists;
   }
   state::objid_t get_id() const { return this->cond_id; }

include/mcmini/model/transitions/mutex/mutex_init.hpp

@@ -17,7 +17,14 @@ struct mutex_init : public model::transition {
 
   status modify(model::mutable_state& s) const override {
     using namespace model::objects;
-    s.add_state_for_obj(mutex_id, new mutex(mutex::unlocked));
+    // Preserve the location set on the placeholder object created by
+    // mutex_init_callback (see mutex.cpp) -- the 1-arg mutex(state)
+    // constructor used below has no default member initializer for
+    // `location`, so it would otherwise be left uninitialized, corrupting
+    // every later mutex_lock/unlock that faithfully forwards whatever
+    // garbage ms->get_location() reads back afterward.
+    const mutex* ms = s.get_state_of_object<mutex>(mutex_id);
+    s.add_state_for_obj(mutex_id, new mutex(mutex::unlocked, ms->get_location()));
     return status::exists;
   }
   state::objid_t get_id() const { return this->mutex_id; }

include/mcmini/model/transitions/mutex/mutex_unlock.hpp

@@ -25,7 +25,7 @@ struct mutex_unlock : public model::transition {
       return status::disabled;
     }
 
-    s.add_state_for_obj(mutex_id, new mutex(mutex::unlocked, ms->get_location(), 0));
+    s.add_state_for_obj(mutex_id, new mutex(mutex::unlocked, ms->get_location()));
     return status::exists;
   }
   state::objid_t get_id() const { return this->mutex_id; }