DeepDebug: McMini, DMTCP and TSAN

CMakeLists.txt

@@ -14,6 +14,7 @@ set(CMAKE_CXX_STANDARD_REQUIRED TRUE)
 # Project configuration
 option(BUILD_TESTS OFF)
 option(VERBOSE_TESTING OFF)
+option(MCMINI_LIBMCMINI_TSAN "Build libmcmini.so itself with -fsanitize=thread" OFF)
 set(MCMINI_DIR "${CMAKE_SOURCE_DIR}")
 set(MCMINI_INCLUDE_DIR "${CMAKE_SOURCE_DIR}/include")
 set(MCMINI_CMAKE_MODULE_DIR "${CMAKE_SOURCE_DIR}/cmake")
@@ -74,6 +75,7 @@ set(LIBMCMINI_C_SRC
   src/lib/sem-wrappers.c
   src/lib/template/loop.c
   src/lib/template/sig.c
+  src/lib/tsan_support.c
   src/lib/wrappers.c
   src/mcmini/Thread_queue.c
 )
@@ -91,6 +93,13 @@ set(LIBMCMINI_EXTRA_COMPILER_DEFINITIONS MC_SHARED_LIBRARY=1 DMTCP=1 LOGGING_ROO
 # -ldl -> dlsym etc.
 set(LIBMCMINI_EXTRA_LINK_FLAGS -lrt -pthread -lm -ldl)
 
+# libmcmini.so is normally uninstrumented even when the target links libtsan
+# (see PLAN.txt); MCMINI_LIBMCMINI_TSAN is an experiment to instrument it too.
+if(MCMINI_LIBMCMINI_TSAN)
+  list(APPEND LIBMCMINI_EXTRA_COMPILER_FLAGS -fsanitize=thread)
+  list(APPEND LIBMCMINI_EXTRA_LINK_FLAGS -fsanitize=thread)
+endif()
+
 # libmcmini.so -> the dylib which is loaded
 add_library(libmcmini SHARED "${LIBMCMINI_C_SRC}")
 set_target_properties(libmcmini PROPERTIES OUTPUT_NAME "mcmini")

doc/classic-mode-thread-registration-segv.txt

@@ -0,0 +1,108 @@
+Classic-mode SEGV: new application threads never registered with TSan
+=============================================================================
+
+Symptom
+--------
+Running any TSan-instrumented target (producer-consumer-tsan,
+cv-producer-consumer-tsan) under classic (non-DMTCP) model checking --
+`mcmini ./target`, no `-i`/`--from-checkpoint` involved at all -- crashed
+100% of the time, deterministically, the moment the target's first
+non-main thread reached its first `libpthread_*`-wrapped call:
+
+  ThreadSanitizer:DEADLYSIGNAL
+  ==PID==ERROR: ThreadSanitizer: SEGV on unknown address 0x000000000000
+  ==PID==The signal is caused by a READ memory access.
+  ==PID==Hint: address points to the zero page.
+  ThreadSanitizer: nested bug in the same thread, aborting.
+
+TSan's own crash handler could not even produce a symbolicated backtrace
+(a second fault while handling the first -- itself a symptom of the same
+root cause: the crashing thread has no valid per-thread TSan state, and
+neither does TSan's own signal-handling code when it tries to use that
+same state to report the crash).
+
+Root cause
+-----------
+Caught live via `gdb --args ./mcmini ./producer-consumer-tsan` with
+`set follow-fork-mode child`, `set detach-on-fork off`, and
+`set schedule-multiple on` (needed, so the parent `mcmini` process keeps
+running -- classic mode requires it to actively coordinate with the
+branch process, so freezing it while continuing only the child deadlocks
+instead of crashing). `handle SIGSEGV stop nopass` lets gdb catch the
+fault before TSan's own handler consumes it. Full backtrace of the
+crashing thread:
+
+  #0  __tsan::SlotLock (thr=0x...) at tsan_rtl.cpp:366
+  #1  __tsan::SlotLocker::SlotLocker (...) at tsan_rtl.h:641
+  #2  __tsan::Acquire (thr=..., pc=..., addr=...) at tsan_rtl_mutex.cpp:448
+  #3  guard_acquire (thr=..., g=<libmcini_init>, ...) at tsan_interceptors_posix.cpp:891
+  #4  ___interceptor_pthread_once (o=<libmcini_init>, f=mc_load_intercepted_pthread_functions)
+  #5  libmcmini_init () from ./libmcmini.so
+  #6  libpthread_mutex_lock () from ./libmcmini.so
+  #7  mc_register_this_thread () from ./libmcmini.so
+  #8  mc_thread_routine_wrapper () from ./libmcmini.so
+  #9  start_thread
+  #10 clone3
+
+The chain: `mc_thread_routine_wrapper()` (the new thread's actual OS-level
+entry point) calls `mc_register_this_thread()` as its first statement,
+which calls `libpthread_mutex_lock()`, which calls `libmcmini_init()`,
+which lazily resolves all the `libpthread_*` bypass handles exactly once
+via `pthread_once()`. `pthread_once` is a public, dynamically-resolved
+symbol that TSan intercepts -- and TSan's interceptor needs this thread's
+own `ThreadState` to do its own bookkeeping. But this thread was never
+registered with TSan in the first place: `mc_pthread_create()`'s
+TARGET_BRANCH/TARGET_BRANCH_AFTER_RESTART case (src/lib/wrappers.c)
+created it via `libpthread_pthread_create()` -- a handle resolved via
+`dlopen("libpthread")` + `dlsym()`, which always finds real, raw glibc,
+bypassing *any* interceptor (TSan's or libmcmini's own) unconditionally,
+regardless of load order. TSan's own `pthread_create` interceptor -- the
+thing that would normally set up this thread's `ThreadState` -- never ran
+for this thread at all.
+
+The comment above the original call ("Calling libpthread_pthread_create
+simplifies all this") suggests this was chosen for convenience, not as a
+deliberate TSan-avoidance measure -- unlike the `__clone`-based recreation
+work for DMTCP-restarted threads (doc files elsewhere in this directory),
+which deliberately bypasses TSan's interceptor for well-documented,
+load-order-specific reasons. This one had no such reason and was simply a
+bug: classic mode has no DMTCP/checkpoint-restart concerns at all, so
+there was never a reason to avoid TSan's own registration for these
+threads.
+
+The fix
+--------
+Added `tsan_or_real_pthread_create()` (src/lib/interception.c), resolved
+via `dlsym(RTLD_NEXT, "pthread_create")` instead of the libpthread-handle
+dlsym used by `libpthread_pthread_create_ptr`. `RTLD_NEXT` finds whatever
+comes *after* libmcmini.so in this process's actual, specific load order:
+
+  - Classic mode: libmcmini loads ahead of libtsan (see
+    TSAN-McMini-DMTCP.txt's own explanation of this load order), so
+    "next after libmcmini" correctly lands on TSan's own pthread_create
+    interceptor -- the fix.
+  - DMTCP mode: libtsan loads ahead of libmcmini, so "next after
+    libmcmini" still resolves past both and lands on real glibc, exactly
+    like libpthread_pthread_create's behavior today -- no change, no
+    regression risk for the already-extensively-verified DMTCP+TSan
+    restart flow (which handles TSan registration for its own
+    clone()-recreated threads via an entirely separate mechanism: R3/R4's
+    __clone + fresh-fiber approach, not pthread_create at all).
+
+`mc_pthread_create()`'s TARGET_BRANCH/TARGET_BRANCH_AFTER_RESTART case
+(src/lib/wrappers.c) now calls `tsan_or_real_pthread_create()` instead of
+`libpthread_pthread_create()`.
+
+Verification
+-------------
+- producer-consumer-tsan and cv-producer-consumer-tsan, classic mode: 100%
+  reproducible SEGV before the fix, zero crashes across multiple runs
+  after (cv-producer-consumer-tsan takes longer to fully explore -- more
+  interleavings from the mutex+cond-based synchronization -- but completes
+  cleanly given enough time).
+- Classic-mode regression check: cv-test, deadly-embrace,
+  producer-consumer, and cv-producer-consumer (non-TSan) all give
+  unchanged results.
+- DMTCP+TSan --multithreaded-fork restart flow (producer-consumer-tsan):
+  batch-verified unaffected, consistent with the RTLD_NEXT resolution
+  falling through to real glibc in that load order exactly as before.

doc/cond-wait-tsan-interceptor-bypass.txt

@@ -0,0 +1,72 @@
+pthread_cond_wait bypasses libmcmini under DMTCP+TSan
+=======================================================
+
+Symptom
+-------
+A DMTCP-restart-from-checkpoint of a TSan-instrumented target hangs
+forever as soon as any thread is genuinely blocked in pthread_cond_wait()
+at checkpoint time. mcmini's own trace output never even reaches
+"INITIAL STATE" for that thread; it just waits on a mailbox message that
+never arrives.
+
+Root cause
+----------
+Under DMTCP, libtsan.so loads ahead of libmcmini.so (the reverse of
+classic mode), so the target's own calls to the public pthread_cond_wait()
+symbol resolve to TSan's interceptor first, via the PLT/GOT -- confirmed
+empirically by reading the resolved GOT entry out of /proc/pid/mem.
+
+This is the same load-order problem pthread_join already had (see
+pthread_join_wrap.c), but pthread_cond_wait can't be fixed the same way
+sem_wait/pthread_mutex_lock/pthread_cond_signal/broadcast are handled:
+those TSan interceptors do their own bookkeeping and then delegate the
+actual operation to whatever's next in the interposition chain (landing
+on libmcmini's mc_* wrapper, since it's next after libtsan under DMTCP).
+pthread_cond_wait must atomically release the mutex and suspend the
+thread, then atomically reacquire the mutex on wakeup; TSan implements
+that whole sequence itself, inside its own runtime, rather than risk a
+race window from splitting the atomicity across an interposed call. So
+the target's pthread_cond_wait() calls never reach mc_pthread_cond_wait()
+at all -- the thread genuinely, permanently blocks inside TSan's own real,
+untimed wait, with nothing left to wake it (see glibc-cond-var-desync.txt
+for why the earlier CV desync fix removed the only thing that could have).
+
+Fix
+---
+-Wl,--wrap=pthread_cond_wait, applied when linking the TARGET (not
+libmcmini.so), rewrites the target's own pthread_cond_wait() calls to
+__wrap_pthread_cond_wait() at link time -- before the dynamic linker (and
+hence TSan's interceptor) is ever involved. See pthread_cond_wait_wrap.c.
+Unlike pthread_join, there's no "sometimes a real wait is still needed"
+case: mc_pthread_cond_wait() is already fully self-contained in every
+libmcmini_mode, so the wrapper forwards unconditionally with no
+__real_pthread_cond_wait() fallback.
+
+Verification
+------------
+Confirmed via GOT inspection that pthread_mutex_lock, pthread_mutex_unlock,
+sem_post, sem_wait, and pthread_cond_wait all resolve to libtsan.so
+directly with no exception -- the asymmetry is in TSan's internal
+delegation behavior per function, not in symbol resolution.
+
+Confirmed live: before the fix, restarting cv-producer-consumer-tsan from
+a checkpoint taken mid-consumer-cond_wait hung indefinitely (trace log
+truncated right after "waiting for them to get into a consistent state").
+After the fix, the same restart completes promptly, and the consumer's
+cond_wait shows up as a genuine, modeled NEXT THREAD OPERATIONS entry
+instead of an invisible black-box call.
+
+Follow-on issue (separate from this fix)
+-----------------------------------------
+That same verification run surfaced a second, previously-unreachable bug:
+once the consumer's cond_wait is actually visible to the model, the
+restart reports an immediate DEADLOCK. The reconstructed state shows the
+mutex still "locked" by the consumer and the condition_variable still at
+cv_initialized (never advanced to cv_waiting), so neither the producer's
+pthread_mutex_lock nor the consumer's own resume-from-wait transition
+(condition_variables_wait.hpp's modify()) can ever become enabled. This
+looks like a bug in how the checkpoint/restart path reconstructs CV
+waiter-queue state (the CV_PREWAITING/CV_WAITING bookkeeping described in
+Cond_Var_Readme.md) rather than anything to do with the TSan-bypass fix
+above, since this code path was never reachable before this fix landed.
+Tracked as a separate, follow-on investigation.

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.