diff --git a/CMakeLists.txt b/CMakeLists.txt index c2fa5fce..88154cce 100644 --- a/CMakeLists.txt +++ b/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") diff --git a/doc/classic-mode-thread-registration-segv.txt b/doc/classic-mode-thread-registration-segv.txt new file mode 100644 index 00000000..309ef7ba --- /dev/null +++ b/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=, ...) at tsan_interceptors_posix.cpp:891 + #4 ___interceptor_pthread_once (o=, 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. diff --git a/doc/cond-wait-tsan-interceptor-bypass.txt b/doc/cond-wait-tsan-interceptor-bypass.txt new file mode 100644 index 00000000..954415c9 --- /dev/null +++ b/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. diff --git a/doc/glibc-cond-var-desync.txt b/doc/glibc-cond-var-desync.txt new file mode 100644 index 00000000..f3d1c5d5 --- /dev/null +++ b/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. diff --git a/doc/glibc-sem-desync.txt b/doc/glibc-sem-desync.txt new file mode 100644 index 00000000..1498c1a4 --- /dev/null +++ b/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. diff --git a/doc/log-mutex-fork-desync.txt b/doc/log-mutex-fork-desync.txt new file mode 100644 index 00000000..145af52a --- /dev/null +++ b/doc/log-mutex-fork-desync.txt @@ -0,0 +1,116 @@ +mcmini_log()'s own mutex frozen locked across multithreaded_fork() +============================================================================= + +This is a third, distinct instance of the general "external state gets +frozen inconsistent across checkpoint/restart or fork/clone" pattern +documented in doc/glibc-sem-desync.txt and doc/glibc-cond-var-desync.txt -- +but the mechanism here is plain, ordinary fork-safety, not a glibc-internal +bookkeeping desync. + +Symptom +-------- +Rare (roughly 1-in-13 to 1-in-25 runs observed), reproducible via repeated +`mcmini --from-first-checkpoint --multithreaded-fork ./producer-consumer-tsan` +cycles: every thread in an occasional DPOR branch hangs forever, very early, +before the branch does any real work. gdb-attaching live to a hung branch +(see doc/glibc-sem-desync.txt's verification-methodology note for the +technique) showed all threads blocked in the exact same place: + + mcmini_log() -> libpthread_mutex_lock(&log_mut) -> real glibc mutex lock, + blocked in a futex wait. + +Reading log_mut's raw memory directly (`x/4xw
` in gdb) confirmed +its lock word was 2 (glibc's "locked, with waiter(s)" encoding) -- a +genuinely, correctly-locked state from glibc's own point of view, just one +nobody alive in this process could ever release. + +Root cause +----------- +`multithreaded_fork()`'s process-duplication step is `_Fork()` (glibc's +internal, minimal fork primitive), not the public `fork()`. This is +deliberate -- the surrounding comment says "We want to skip the normal fork +cleanup of child threads" -- and one of the things `_Fork()` explicitly, +intentionally skips is running any handler registered via +`pthread_atfork()`. That is precisely the mechanism a normal `fork()` would +use to let any lock protecting shared state reset itself safely in the +child. + +`mcmini_log()` serializes its whole body (including a glibc `localtime_r()` +call that is not safe to race) with a single, ordinary, static +`pthread_mutex_t log_mut`, locked and unlocked by whichever thread is +logging at the time -- including, most plausibly, the template thread +itself, which logs extremely frequently (every "Restarting into template", +"The template process created child with pid ...", etc. debug line seen +throughout this project's own logs). If _Fork() happens to snapshot memory +for a new branch at the exact instant some thread holds log_mut, that +thread's matching unlock() is now stranded in a different process (or a +different, non-recreated context) that this specific branch has no +relationship to. Every thread in the new branch that later calls +mcmini_log() -- which is essentially every thread, given how pervasively +this codebase logs -- deadlocks on it forever. + +The fix +-------- +`mcmini_log_reset_after_fork()` (src/lib/log.c) force-reinitializes +`log_mut` via the libpthread bypass handle. It is called from +`fast_multithreaded_fork()` (src/lib/dmtcp-callback.c -- see "Correction" +below), in the `childpid == 0` branch, immediately after `_Fork()` returns +and before `restart_child_threads_fast()` recreates any other thread -- +i.e. while this is still the only OS thread alive in the child, so the +reset can never race a concurrent lock/unlock attempt. + +Files and functions: + + include/mcmini/lib/log.h + - Declares mcmini_log_reset_after_fork(). + + src/lib/log.c + - log_mut moved from a mcmini_log()-local static to file scope, so + mcmini_log_reset_after_fork() can reach the same variable. + - mcmini_log_reset_after_fork() added, using libpthread_mutex_init() + (the bypass handle) -- see "Correction" below for why this matters. + + src/lib/dmtcp-callback.c + - fast_multithreaded_fork() calls mcmini_log_reset_after_fork() right + after _Fork() returns 0 (child) and the fiber switch, before + restart_child_threads_fast(). + +Correction (found while investigating a later, unrelated report) +------------------------------------------------------------------ +The original version of this fix wired mcmini_log_reset_after_fork() into +src/common/multithreaded_fork.c's multithreaded_fork() -- which turned out +to be dead code: superseded by fast_multithreaded_fork() in +src/lib/dmtcp-callback.c (commit 74679cf, "Use fast_multithreaded_fork()"), +the only function template/loop.c's real, active fork loop ever calls. The +fix was therefore never active on the real `--multithreaded-fork` path at +all -- confirmed by grepping every caller of mcmini_log_reset_after_fork() +and finding only the dead-code call site. This explains the "one hang +reproduced with the fix already applied" discrepancy below: the "fix" +wasn't actually running, so every observed hang (before AND supposedly +after) shared the same, never-actually-changed root cause. The 140 +hang-free runs were most likely an unlucky-for-testing lucky streak at the +true, still-untouched ~1-in-13-to-25 rate, not evidence the fix worked. + +Moving the call to the real fast_multithreaded_fork() immediately exposed a +second, previously-dormant bug in mcmini_log_reset_after_fork() itself: it +called the plain, public pthread_mutex_init() -- which, since libmcmini.so +intercepts that very symbol, recursively re-enters McMini's own +mc_pthread_mutex_init() wrapper for what should be a purely internal, +invisible mutex reset. That wrapper's restart-mode dispatch asserts the +calling thread already has a valid tid_self via thread_get_mailbox() -- +not yet true this early, immediately after _Fork(), before +restart_child_threads_fast() has run -- so the very first real exercise of +the (now correctly wired-up) reset crashed with `Assertion 'tid_self != +RID_INVALID' failed` on the first restart attempt. Fixed by switching to +libpthread_mutex_init() (the same bypass handle log_mut's own lock/unlock +already use), which never re-enters libmcmini's wrapper machinery. + +Verification after the correction +------------------------------------ +40 consecutive fresh-checkpoint `producer-consumer-tsan --multithreaded-fork` +restart cycles completed with zero hangs and zero crashes, using the +corrected call site and the corrected libpthread_mutex_init() reset. This +is now believed to be the first time this fix has actually been exercised +on the real code path. Given the original ~1-in-13-to-25 rate, 40 clean +runs is a smaller sample than the original (misleading) 140-run claim -- +worth another, larger batch if this area is revisited again. diff --git a/doc/pthread-exit-abort-and-fiber-crash.txt b/doc/pthread-exit-abort-and-fiber-crash.txt new file mode 100644 index 00000000..65eeccf3 --- /dev/null +++ b/doc/pthread-exit-abort-and-fiber-crash.txt @@ -0,0 +1,83 @@ +pthread_exit() crashes: two separate bugs, neither ever exercised before +========================================================================== + +Symptom +------- +No example in this repo called pthread_exit() explicitly (all just `return` +from their thread routine), so this path had never been tested. Added +producer-consumer-exit(-tsan) (identical to producer-consumer-safe, but +each worker calls pthread_exit(NULL) instead of returning) to test it, per +multithreaded-fork-tsan-2.0's test_exit.c standalone analog (which passes +cleanly, and was the reference used to diagnose both bugs below). + +Bug 1: mc_pthread_exit() falls through to libc_abort() (classic mode too) +-------------------------------------------------------------------------- +mc_pthread_exit()'s TARGET_BRANCH/TARGET_BRANCH_AFTER_RESTART case called +mc_exit_thread_in_child() (or mc_exit_main_thread_in_child()) and then, with +no further statement, fell through the switch into `default: { libc_abort(); +}` -- reproducible even in plain classic mode (no DMTCP at all): SIGABRT +every time. + +Root cause: mc_exit_thread_in_child() only performs the model-checker- +visible exit transition (mailbox message, then blocks until the eventual +joiner grants permission) and returns once the thread is safe to really +terminate -- it was never meant to be the final step. When a thread exits by +*returning* from its start routine, mc_thread_routine_wrapper calls +mc_exit_thread_in_child() from ITS OWN frame, so once it returns, +mc_thread_routine_wrapper itself returns and the real OS thread terminates +naturally via glibc. But mc_pthread_exit() is invoked from arbitrary depth +inside the application's own call stack (pthread_exit() must never return +to its caller), so simply returning from mc_pthread_exit() after +mc_exit_thread_in_child() would incorrectly hand control back to the +application -- something has to actually terminate the thread right there. + +Fix: call libpthread_pthread_exit() (the real, TSan-interceptor-bypassing +pthread_exit, already used elsewhere in this file) after +mc_exit_thread_in_child() returns. This alone fixes classic mode. + +Bug 2: real pthread_exit() crashes DMTCP-resurrected threads (SIGSEGV) +------------------------------------------------------------------------ +With bug 1 fixed, DMTCP+TSan restart of a target whose threads existed +before the checkpoint (and were therefore recreated via +restart_child_threads_fast()'s libc_clone()+setcontext(), not a real +pthread_create()) crashed instead: SIGSEGV, RIP=0, identical registers +across independent runs (deterministic, not corruption). Confirmed via a +targeted debug print that libpthread_pthread_exit_ptr itself was valid +(same address as during recording) -- the crash is inside the real +pthread_exit() call itself, not a null-pointer bug. + +Root cause: a clone()-resurrected thread is invisible to TSan's own thread +registry (see the R3/R4 fixes for the analogous clone()-vs-fork problem); +after resuming it's given a fresh TSan *fiber* rather than being a +first-class TSan thread. glibc's real pthread_exit() forces a cleanup- +unwind whose TSan-instrumented calls run against that fiber's state -- +which isn't a "real" thread's, and crashes. This is the exact, already- +documented "Known limitation" in multithreaded-fork-tsan-2.0's README, and +its tsan_join_shim.c's pthread_exit() override deliberately never calls +real pthread_exit() for a recreated thread: it stashes the return value, +switches onto a fresh (empty) TSan fiber, and terminates via a raw exit +syscall instead (no unwind, no TSan interceptor -- kernel's +CLONE_CHILD_CLEARTID still wakes any real joiner). + +This repo had no equivalent "is this thread one we recreated via clone()" +query at all (dmtcp-callback.c's threadInfos[]/threadIdx already track +exactly this, just with no accessor). Added mc_pthread_is_recreated_thread() +(wrappers.h/dmtcp-callback.c), and mc_pthread_exit() now branches on it: +a recreated thread gets the fiber-switch + raw `syscall(SYS_exit, 0)` +treatment (real pthread_exit() never called); a normal thread still gets +the real libpthread_pthread_exit() from the bug-1 fix. Unlike the tarball, +no explicit exit-value stash was needed: mc_exit_thread_in_child()'s +mailbox-based exit transition (executed before either termination path) +already makes the model checker's join mechanism (mc_pthread_join) resolve +correctly independent of any real futex-based join, so the raw exit syscall +doesn't need to preserve that. + +Verification +------------ +producer-consumer-exit-tsan: classic mode and 4/4 DMTCP+`--multithreaded-fork` +record/restart cycles now complete cleanly (`Deep debugging completed!`), +zero crashes, vs. 100% reproducible SIGABRT (classic) / SIGSEGV (restart) +before. producer-consumer-safe-tsan and cv-producer-consumer-safe-tsan +(neither calls pthread_exit() explicitly) regression-checked clean in both +classic mode and DMTCP+restart, confirming no behavior change for the +already-working return-based exit path. diff --git a/doc/tsan-mutex-annotation-false-positive.txt b/doc/tsan-mutex-annotation-false-positive.txt new file mode 100644 index 00000000..b439e186 --- /dev/null +++ b/doc/tsan-mutex-annotation-false-positive.txt @@ -0,0 +1,78 @@ +TSan reports false-positive races on mutex-protected globals +============================================================== + +Symptom +------- +Classic-mode model checking of a TSan-instrumented target reports data +races on globals that ARE correctly protected by a pthread_mutex_t in the +source (e.g. cv-producer-consumer's `count`/`buffer`, producer-consumer's +`buffer`) -- reproducibly, in most explored interleavings. The reports +carry TSan's "As if synchronized via sleep" heuristic annotation, meaning +TSan found no real synchronization primitive linking the two accesses and +fell back to a weak, real-time-based heuristic instead. + +Root cause +---------- +mc_pthread_mutex_lock()/mc_pthread_mutex_unlock() (and the mutex release/ +reacquire mc_pthread_cond_wait() does around its enqueue/wait mailbox +round-trip) really lock/unlock the application's own pthread_mutex_t via +libpthread_mutex_lock/unlock/timedlock -- resolved once at startup via a +raw dlopen("libpthread.so")+dlsym() in interception.c, which always lands +on real glibc, bypassing TSan's own pthread_mutex_lock/unlock interceptor +entirely (unlike pthread_mutex_init, which -- being called by the +application directly, not by one of these wrappers -- does reach TSan's +interceptor; see the "Mutex M0 created at: ... tsan_interceptors_posix.cpp" +frame in any of these race reports). + +Since TSan never sees the actual lock/unlock events, it has no way to +establish a happens-before edge between one thread's critical section and +another's, even though mcmini's own scheduler (in restart/branch modes) or +the real OS mutex (in RECORD mode) correctly serializes them. Every +variable the mutex protects looks racy to TSan as a result. + +This is the exact same class of bug already fixed once in this codebase, +for mcmini_log()'s own log_mut (see log.c's comment) -- just not +previously applied to the application's own mutex. + +Fix +--- +Same fix as log_mut: TSan's public annotation API (__tsan_acquire/ +__tsan_release, declared weak, so they're a no-op when no TSan runtime is +loaded). Call __tsan_acquire(mutex) right after every real lock succeeds, +and __tsan_release(mutex) right before every real unlock, in +mc_pthread_mutex_lock(), mc_pthread_mutex_unlock(), and around both real +unlock/lock calls in mc_pthread_cond_wait()'s restart-branch handling. + +Verification +------------ +producer-consumer-safe-tsan (see below) went from 79 false-positive races +across explored interleavings in classic mode to zero. cv-producer- +consumer-safe-tsan went from 208 (all of which turned out to be the +unrelated a[] race below, confirmed via grep -c "Location is global +'count'"/'buffer' returning 0) to zero. DMTCP+TSan record/restart cycles +for both -safe targets stayed clean (no crash, no hang, no races). + +Unrelated finding along the way: -safe example variants +--------------------------------------------------------- +While chasing this, a live gdb backtrace (during an unrelated DMTCP+TSan +restart hang, since resolved -- see below) caught TSan mid-report for a +genuine, pre-existing data race in producer-consumer.c/cv-producer- +consumer.c's main(): both example programs size their thread-argument +array to max(NUM_PRODUCERS, NUM_CONSUMERS) and reuse it across both +pthread_create loops, so main()'s second loop can overwrite a producer's +argument slot while that producer thread is still reading it. This race +is unrelated to the TSan-annotation issue above (confirmed: it reproduces +identically in ordinary, non-model-checked RECORD-mode runs), but its +report-generation overlapping with a DMTCP checkpoint signal is what +caused a genuine deadlock between DMTCP's checkpoint-quiescence signal +handler and TSan's own internal locking (a separate, narrow DMTCP/TSan +interaction hazard, not itself a mcmini bug). + +Rather than fix this race in the original example files, producer- +consumer-safe.c and cv-producer-consumer-safe.c were added alongside them +(and their own -tsan CMake targets): identical except each thread gets +its own distinct argument-storage slot instead of sharing one reused +array. This keeps the original files' latent race intact for whoever +wants to study the DMTCP/TSan interaction above, while giving future +McMini test/development work a race-free target to isolate other bugs +against without that noise. diff --git a/include/dmtcp.h b/include/dmtcp.h index d0e774e4..eec74ab6 100644 --- a/include/dmtcp.h +++ b/include/dmtcp.h @@ -16,6 +16,8 @@ #define DMTCP_H #include +#include +#include #include #include #include @@ -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,36 @@ 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 the virtual tid of TSAN's own background thread, 0 if there is +// none (including when not running under DMTCP or not a TSAN target), or +// -1 if that hasn't been determined yet (e.g. briefly after a restart, +// before TSAN respawns its background thread). +int dmtcp_tsan_background_thread_virtual_tid() __attribute((weak)); +#define dmtcp_tsan_background_thread_virtual_tid() \ + (dmtcp_tsan_background_thread_virtual_tid ? \ + dmtcp_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 +539,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)); diff --git a/include/mcmini/lib/log.h b/include/mcmini/lib/log.h index 9d1d570a..7cd9d12f 100644 --- a/include/mcmini/lib/log.h +++ b/include/mcmini/lib/log.h @@ -30,3 +30,9 @@ enum log_level { void mcmini_log_set_level(int level); void mcmini_log_toggle(bool enable); void mcmini_log(int level, const char *file, int line, const char *fmt, ...); + +// Force log_mut back to the unlocked state. Call this exactly once, as the +// sole surviving thread in a freshly-_Fork()-ed child, before any other +// thread is recreated in it. See the comment above log_mut in log.c for why +// this is needed. +void mcmini_log_reset_after_fork(void); diff --git a/include/mcmini/mem.h b/include/mcmini/mem.h index 0d2154ca..8cd2cfd8 100644 --- a/include/mcmini/mem.h +++ b/include/mcmini/mem.h @@ -9,6 +9,18 @@ extern "C" { volatile void *memset_v(volatile void *, int ch, size_t n); volatile void *memcpy_v(volatile void *, const volatile void *, size_t n); +// TSan-safe bump allocator over a fixed static (BSS) arena. +// +// Used for allocations that may execute on a thread BEFORE ThreadSanitizer has +// registered it -- specifically the mc_thread_routine_wrapper prologue when a +// TSAN'd target runs under DMTCP (libtsan wraps libmcmini, so libmcmini's +// wrapper runs before libtsan's thread-start trampoline). It performs NO libc +// call and NO syscall on the fast path -- only a static-memory atomic bump -- +// so it never enters a TSan interceptor and never dereferences an +// unregistered thread's (null) ThreadState. Never frees (nodes it backs are +// never freed; see record.c / wrappers.c). See TSAN-McMini-DMTCP.txt. +void *mc_ts_alloc(size_t n); + #ifdef __cplusplus } // extern "C" #endif diff --git a/include/mcmini/model/objects/condition_variables.hpp b/include/mcmini/model/objects/condition_variables.hpp index 5bec5cf0..1900cfa8 100644 --- a/include/mcmini/model/objects/condition_variables.hpp +++ b/include/mcmini/model/objects/condition_variables.hpp @@ -24,7 +24,7 @@ struct condition_variable : public model::visible_object_state { }; private: - state current_state = state::cv_uninitialized; + state current_state = state::cv_uninitialized; bool hadwaiters; mutable unsigned int numRemainingSpuriousWakeups = 0; runner_id_t running_thread; @@ -32,39 +32,46 @@ struct condition_variable : public model::visible_object_state { int waiting_count = 0; int prev_waiting_count = 0; int lost_wakeups = 0; - ConditionVariablePolicy* policy = new ConditionVariableArbitraryPolicy(); + ConditionVariablePolicy* policy; public: condition_variable() = default; ~condition_variable() = default; condition_variable(const condition_variable &) = default; - condition_variable(state s) : current_state(s) {} - condition_variable(state s, ConditionVariablePolicy* p) : current_state(s), policy(p) {} - condition_variable(state s, int count) : current_state(s) {} - condition_variable(state s, runner_id_t tid, pthread_mutex_t* mutex, int count) - : current_state(s), running_thread(tid), associated_mutex(mutex), waiting_count(count){} - - condition_variable(state s, runner_id_t tid, pthread_mutex_t* mutex, int count, - const std::vector>& thread_states) - : current_state(s), running_thread(tid), associated_mutex(mutex), waiting_count(count) { - // Initialize the policy according to the states of the threads in waiting queue - for (const auto& thread_with_state : thread_states) { - if (thread_with_state.second == CV_PREWAITING || thread_with_state.second == CV_WAITING) { - this->policy->add_waiter_with_state(thread_with_state.first,thread_with_state.second); - } - } - std::vector signaled_threads; - for (const auto& thread_with_state : thread_states) { - if (thread_with_state.second == CV_SIGNALED) { - signaled_threads.push_back(thread_with_state.first); - } - } - if (!signaled_threads.empty()) { - // If there are any threads that have been signaled, we should - // add them to the wake groups in the policy. - this->policy->add_to_wake_groups(signaled_threads); - } - } + // The only non-copy constructor: `tid`/`mutex` get sentinel defaults + // (RID_INVALID/nullptr) rather than being left uninitialized (there was + // previously no default member initializer for either, unlike `waiting_count`) + // -- e.g. mutex(state, location) needed the identical fix for `location` + // after commit 9bd9ecf. `p` (policy) defaults to nullptr meaning "create a + // fresh one"; every transition that advances an *existing* condition + // variable's state must still pass its own (possibly cloned) policy + // explicitly here or via set_policy() afterward -- see that setter's + // comment for why an omitted policy silently discards in-progress waiter + // tracking rather than just being harmlessly empty. + condition_variable(state s, runner_id_t tid = RID_INVALID, + pthread_mutex_t* mutex = nullptr, int count = 0, + const std::vector>& thread_states = {}, + ConditionVariablePolicy* p = nullptr) + : current_state(s), running_thread(tid), associated_mutex(mutex), waiting_count(count), + policy(p ? p : new ConditionVariableArbitraryPolicy()) { + // Initialize the policy according to the states of the threads in waiting queue + for (const auto& thread_with_state : thread_states) { + if (thread_with_state.second == CV_PREWAITING || thread_with_state.second == CV_WAITING) { + this->policy->add_waiter_with_state(thread_with_state.first,thread_with_state.second); + } + } + std::vector signaled_threads; + for (const auto& thread_with_state : thread_states) { + if (thread_with_state.second == CV_SIGNALED) { + signaled_threads.push_back(thread_with_state.first); + } + } + if (!signaled_threads.empty()) { + // If there are any threads that have been signaled, we should + // add them to the wake groups in the policy. + this->policy->add_to_wake_groups(signaled_threads); + } + } // ---- State Observation --- // bool operator==(const condition_variable &other) const { return this->current_state == other.current_state; @@ -82,6 +89,11 @@ struct condition_variable : public model::visible_object_state { ConditionVariablePolicy* get_policy() const {return this->policy;} + // Carries a cloned/mutated policy into the replacement CV object; + // omitting this silently resets it to an empty policy, discarding + // whatever was just mutated (e.g. a newly registered waiter). + void set_policy(ConditionVariablePolicy* p) { this->policy = p; } + void set_associated_mutex(pthread_mutex_t* mutex) { this->associated_mutex = mutex; } diff --git a/include/mcmini/model/objects/mutex.hpp b/include/mcmini/model/objects/mutex.hpp index 66789238..91d2fbac 100644 --- a/include/mcmini/model/objects/mutex.hpp +++ b/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 { diff --git a/include/mcmini/model/transitions/condition_variables/condition_variable_brdcast.hpp b/include/mcmini/model/transitions/condition_variables/condition_variable_brdcast.hpp index edcd2ea5..01f12eea 100644 --- a/include/mcmini/model/transitions/condition_variables/condition_variable_brdcast.hpp +++ b/include/mcmini/model/transitions/condition_variables/condition_variable_brdcast.hpp @@ -49,23 +49,33 @@ struct condition_variable_broadcast : public model::transition { } } + // Clone the policy rather than mutating cv->get_policy() in place -- see + // condition_variable_enqueue_thread::modify()'s identical comment for + // why: is_enabled_in() runs this same modify() against a throwaway + // diff_state just to check the status, and an in-place mutation on the + // old, shared policy would escape that throwaway diff and permanently + // corrupt the committed state. + ConditionVariablePolicy* new_policy = cv->get_policy()->clone(); + // Add only CV_WAITING threads to wake groups if (!waiting_threads.empty()) { - cv->get_policy()->add_to_wake_groups(waiting_threads); + new_policy->add_to_wake_groups(waiting_threads); } - - // Make the threads in the wake groups eligible to be woken up by adding them all to broadcast_eligible_threads - cv->send_broadcast_message(); - + + // Make the threads in the wake groups eligible to be woken up by adding + // them all to broadcast_eligible_threads + new_policy->receive_broadcast_message(); + // Update condition variable state - const int new_waiting_count = cv->get_policy()->return_wait_queue().size(); + const int new_waiting_count = new_policy->return_wait_queue().size(); condition_variable::state new_state = new_waiting_count > 0 ? condition_variable::cv_waiting : condition_variable::cv_signaled; - - s.add_state_for_obj(cond_id, new condition_variable(new_state, executor, + condition_variable* new_cv = new condition_variable(new_state, executor, cv->get_mutex(), - new_waiting_count)); + new_waiting_count); + new_cv->set_policy(new_policy); + s.add_state_for_obj(cond_id, new_cv); return status::exists; } state::objid_t get_id() const { return this->cond_id;} diff --git a/include/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hpp b/include/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hpp index 73f2b1b1..e6a8cb94 100644 --- a/include/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hpp +++ b/include/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hpp @@ -32,17 +32,35 @@ struct condition_variable_enqueue_thread : public model::transition{ return status::disabled; } - condition_variable_status current_state = cv->get_policy()->get_thread_cv_state(executor); + // Clone the policy rather than mutating cv->get_policy() in place: `cv` + // points into the *previous*, already-committed state's object. + // transition::is_enabled_in() calls modify() against a throwaway + // diff_state purely to check the returned status, discarding the diff -- + // but a mutation made directly on the old, shared policy object escapes + // that throwaway diff_state and permanently corrupts the committed + // state, even for a check that's never actually applied. See + // condition_variable::set_policy()'s comment for why the policy must be + // carried into the replacement object explicitly either way. + ConditionVariablePolicy* new_policy = cv->get_policy()->clone(); + condition_variable_status current_state = new_policy->get_thread_cv_state(executor); if (current_state == CV_PREWAITING) { // Thread not fully in wait state - update to WAITING before proceeding - cv->get_policy()->update_thread_cv_state(executor, CV_WAITING); + new_policy->update_thread_cv_state(executor, CV_WAITING); } - 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)); + new_policy->add_waiter_with_state(executor, CV_WAITING); + const int new_waiting_count = new_policy->return_wait_queue().size(); + + condition_variable* new_cv = new condition_variable(condition_variable::cv_waiting, executor, m->get_location(), new_waiting_count); + new_cv->set_policy(new_policy); + s.add_state_for_obj(cond_id, new_cv); + // 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; } diff --git a/include/mcmini/model/transitions/condition_variables/condition_variables_signal.hpp b/include/mcmini/model/transitions/condition_variables/condition_variables_signal.hpp index 7a0602ad..8d65256c 100644 --- a/include/mcmini/model/transitions/condition_variables/condition_variables_signal.hpp +++ b/include/mcmini/model/transitions/condition_variables/condition_variables_signal.hpp @@ -71,21 +71,34 @@ struct condition_variable_signal : public model::transition { waiting_threads.push_back(tid); } } - + + // Clone the policy rather than mutating cv->get_policy() in place -- see + // condition_variable_enqueue_thread::modify()'s identical comment for + // why: is_enabled_in() runs this same modify() against a throwaway + // diff_state just to check the status, and an in-place mutation on the + // old, shared policy would escape that throwaway diff and permanently + // corrupt the committed state. + ConditionVariablePolicy* new_policy = cv->get_policy()->clone(); // Add only CV_WAITING threads to wake groups if (!waiting_threads.empty()) { - cv->get_policy()->add_to_wake_groups(waiting_threads); + new_policy->add_to_wake_groups(waiting_threads); } // Update the condition variable state - const int new_waiting_count = cv->get_policy()->return_wait_queue().size(); + const int new_waiting_count = new_policy->return_wait_queue().size(); condition_variable::state new_state = new_waiting_count > 0 ? condition_variable::cv_waiting : condition_variable::cv_signaled; - - s.add_state_for_obj(cond_id, new condition_variable(new_state, new_waiting_count)); - condition_variable* mutable_cv = new condition_variable(new_state, new_waiting_count); - mutable_cv->check_for_lost_wakeup(true, prev_waiting_count); // Check for lost wakeup if this was a signal + // Also preserve the mutex association: it isn't passed through the + // constructor here, so it would otherwise be left uninitialized, making + // condition_variable_wait::modify()'s `m->get_location() == + // cv->get_mutex()` check fail forever afterward. + condition_variable* new_cv = new condition_variable(new_state, RID_INVALID, nullptr, new_waiting_count); + new_cv->set_policy(new_policy); + new_cv->set_associated_mutex(cv->get_mutex()); + s.add_state_for_obj(cond_id, new_cv); + condition_variable mutable_cv(new_state, RID_INVALID, nullptr, new_waiting_count); + mutable_cv.check_for_lost_wakeup(true, prev_waiting_count); // Check for lost wakeup if this was a signal return status::exists; } diff --git a/include/mcmini/model/transitions/condition_variables/condition_variables_wait.hpp b/include/mcmini/model/transitions/condition_variables/condition_variables_wait.hpp index 3944d796..079fcc16 100644 --- a/include/mcmini/model/transitions/condition_variables/condition_variables_wait.hpp +++ b/include/mcmini/model/transitions/condition_variables/condition_variables_wait.hpp @@ -43,14 +43,22 @@ struct condition_variable_wait : public model::transition { // Reacquire the mutex: update its state to "locked" with the executor. s.add_state_for_obj(mutex_id, new mutex(mutex::locked, m->get_location(), executor)); - // remove the executor from the wake group - cv->remove_waiter(executor); - - const int new_waiting_count = cv->get_policy()->return_wait_queue().size(); + // Clone the policy rather than mutating cv->get_policy() (via + // cv->remove_waiter()) in place -- see condition_variable_enqueue_thread + // ::modify()'s identical comment for why: is_enabled_in() runs this same + // modify() against a throwaway diff_state just to check the status, and + // an in-place mutation on the old, shared policy would escape that + // throwaway diff and permanently corrupt the committed state. + ConditionVariablePolicy* new_policy = cv->get_policy()->clone(); + new_policy->wake_thread(executor); + + const int new_waiting_count = new_policy->return_wait_queue().size(); condition_variable::state new_state = new_waiting_count > 0 ? condition_variable::cv_waiting : condition_variable::cv_signaled; - s.add_state_for_obj(cond_id, new condition_variable(new_state, executor, m->get_location(), new_waiting_count)); + condition_variable* new_cv = new condition_variable(new_state, executor, m->get_location(), new_waiting_count); + new_cv->set_policy(new_policy); + s.add_state_for_obj(cond_id, new_cv); return status::exists; } state::objid_t get_id() const { return this->cond_id; } diff --git a/include/mcmini/model/transitions/mutex/mutex_init.hpp b/include/mcmini/model/transitions/mutex/mutex_init.hpp index c664b2af..25dd1386 100644 --- a/include/mcmini/model/transitions/mutex/mutex_init.hpp +++ b/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_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; } diff --git a/include/mcmini/model/transitions/mutex/mutex_unlock.hpp b/include/mcmini/model/transitions/mutex/mutex_unlock.hpp index b100089a..4b50aabd 100644 --- a/include/mcmini/model/transitions/mutex/mutex_unlock.hpp +++ b/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; } diff --git a/include/mcmini/model/transitions/process/exit.hpp b/include/mcmini/model/transitions/process/exit.hpp index e822bc20..6e69804d 100644 --- a/include/mcmini/model/transitions/process/exit.hpp +++ b/include/mcmini/model/transitions/process/exit.hpp @@ -1,5 +1,6 @@ #pragma once +#include "mcmini/model/objects/thread.hpp" #include "mcmini/model/transition.hpp" namespace model { @@ -14,8 +15,14 @@ struct process_exit : public model::transition { ~process_exit() = default; status modify(model::mutable_state& s) const override { - // We ensure that exiting is never enabled. This ensures that it will never - // be explored by any model checking algorithm + // Mark the executor exited, like thread_exit does (minus its + // RID_MAIN_THREAD restriction, since exit()/abort() can end the process + // from any thread). Without this, is_active() keeps reporting true, so + // classic_dpor re-selects this same "enabled" transition forever for any + // exit code its program_exit_code() > 0 check doesn't already catch + // (i.e. exit code 0). + using namespace model::objects; + s.add_state_for_runner(executor, new thread(thread::exited)); return status::exists; } diff --git a/include/mcmini/real_world/mailbox/runner_mailbox.h b/include/mcmini/real_world/mailbox/runner_mailbox.h index 8f864b55..d3bd644b 100644 --- a/include/mcmini/real_world/mailbox/runner_mailbox.h +++ b/include/mcmini/real_world/mailbox/runner_mailbox.h @@ -8,8 +8,21 @@ extern "C" { #include typedef struct { + // Waited on by the verifier (mcmini), posted by the target thread. The + // verifier is never checkpointed, so a plain glibc sem_t is safe here. sem_t model_side_sem; - sem_t child_side_sem; + // Waited on by the target thread, posted by the verifier. A DMTCP-restored + // target thread can be genuinely blocked (kernel-level) on this exact + // futex word while glibc's own userspace "is anyone really waiting" + // bookkeeping is desynced from that -- because this memory gets + // reinitialized (see mc_runner_mailbox_init()/_destroy()) before every new + // DMTCP-restarted branch, and glibc's NPTL sem_t packs that bookkeeping + // into the same memory sem_post() checks to decide whether to skip the + // underlying futex(FUTEX_WAKE) syscall. A plain futex word (see + // mc_wake_thread()/mc_wait_for_scheduler() in runner_mailbox.c, which + // always call FUTEX_WAKE unconditionally) has no such bookkeeping to + // desync. + uint32_t child_side_sem; uint32_t type; uint8_t cnts[64]; // TODO: How much space should each thread have to write // payloads? diff --git a/include/mcmini/real_world/process/dmtcp_process_source.hpp b/include/mcmini/real_world/process/dmtcp_process_source.hpp index 7e334f75..c9806d9a 100644 --- a/include/mcmini/real_world/process/dmtcp_process_source.hpp +++ b/include/mcmini/real_world/process/dmtcp_process_source.hpp @@ -6,7 +6,7 @@ #include "mcmini/defines.h" #include "mcmini/forwards.hpp" -#include "mcmini/real_world/process/dmtcp_coordinator.hpp" +#include "mcmini/real_world/dmtcp_target.hpp" #include "mcmini/real_world/process/local_linux_process.hpp" #include "mcmini/real_world/process_source.hpp" #include "mcmini/real_world/shm.hpp" @@ -27,14 +27,12 @@ class dmtcp_process_source : public process_source { private: std::string ckpt_file; dmtcp_target dmtcp_restart_target; - dmtcp_coordinator coordinator_target; private: pid_t make_new_branch(); public: dmtcp_process_source(const std::string &ckpt_file); - virtual ~dmtcp_process_source(); public: std::unique_ptr make_new_process() override; diff --git a/include/mcmini/spy/checkpointing/objects.h b/include/mcmini/spy/checkpointing/objects.h index 413224f4..c6380e51 100644 --- a/include/mcmini/spy/checkpointing/objects.h +++ b/include/mcmini/spy/checkpointing/objects.h @@ -19,11 +19,20 @@ typedef enum visible_object_type { CV_WAITERS_QUEUE } visible_object_type; -typedef enum mutex_state { +typedef enum mutex_status { UNINITIALIZED, UNLOCKED, LOCKED, DESTROYED +} mutex_status; + +// The thread holding the mutex while LOCKED, so a checkpoint taken mid +// pthread_cond_wait() (mutex still modeled as LOCKED until the enqueue +// transition runs -- see condition_variable_enqueue_thread::modify()) can +// be reconstructed with the correct owner instead of undefined data. +typedef struct mutex_state { + mutex_status status; + runner_id_t owner; } mutex_state; typedef enum thread_status { diff --git a/include/mcmini/spy/checkpointing/record.h b/include/mcmini/spy/checkpointing/record.h index 59388a26..6bfb40af 100644 --- a/include/mcmini/spy/checkpointing/record.h +++ b/include/mcmini/spy/checkpointing/record.h @@ -60,14 +60,19 @@ extern "C" { * behave as in `PRE_DMTCP_INIT`. For `pthread_create`, the call is _only_ * forwarded into DMTCP instead -- the checkpoint thread is NOT recorded. * - * CHECKPOINT_THREAD: - * In this mode, DMTCP has created the checkpoint thread. The checkpoint - * thread should not interact with McMini in any way. But the two interact - * because the functions `libmcmini.so` overrides are used by `dmtcp` - * extensively (e.g. `sem_wait()`). This mode, special to the library when - * executing from the perspective of the checkpoint thread, indicates that DMTCP - * has called directly into McMini. In most cases, this probably means - * forwarding the call to DMTCP's wrapper functions of to `libpthread.so`. + * EXTERNAL_THREAD: + * In this mode, the calling thread is not one of the target program's own + * threads: it is external "plumbing" that happens to call into functions + * `libmcmini.so` overrides (e.g. `sem_wait()`), even though it should not + * otherwise interact with McMini at all. `get_current_mode()` reports this + * mode for such a thread regardless of the global recording/restart state. + * Two cases are known to hit this: (1) DMTCP's own checkpoint thread, which + * uses `libmcmini.so`'s overridden functions extensively as part of its own + * bookkeeping, and (2) ThreadSanitizer's internal background thread (present + * when the target is TSan-instrumented), which can likewise call into these + * same overridden functions for its own purposes. In either case, wrapper + * functions should simply forward the call, typically to DMTCP's own wrapper + * or to `libpthread.so`. * * RECORD: * In this mode, `libmcmini.so` performs a light-weight recording of the @@ -124,7 +129,7 @@ extern "C" { enum libmcmini_mode { PRE_DMTCP_INIT, PRE_CHECKPOINT_THREAD, - CHECKPOINT_THREAD, + EXTERNAL_THREAD, RECORD, PRE_CHECKPOINT, DMTCP_RESTART_INTO_BRANCH, @@ -160,6 +165,9 @@ void set_current_mode(enum libmcmini_mode); extern pthread_t ckpt_pthread_descriptor; extern volatile atomic_bool libmcmini_has_recorded_checkpoint_thread; bool is_checkpoint_thread(void); +void record_checkpoint_thread(void); +void mark_ckpt_window_candidate(int virtual_tid); +void resolve_ckpt_window_candidate_if_pending(void); typedef struct visible_object visible_object; typedef struct rec_list rec_list; @@ -187,6 +195,11 @@ bool is_dmtcp_object(void *addr); /// @note you must acquire `rec_list_lock` before calling this function rec_list *add_rec_entry(const visible_object *, rec_list **, rec_list **); rec_list *add_rec_entry_record_mode(const visible_object *); +// Like add_rec_entry_record_mode, but allocates the node with mc_ts_alloc +// instead of malloc, so it is safe to call from a thread that libtsan has not +// yet registered (the mc_thread_routine_wrapper prologue under DMTCP). See +// TSAN-McMini-DMTCP.txt. +rec_list *add_rec_entry_record_mode_ts(const visible_object *); void print_rec_list(const rec_list *); rec_list *add_dmctp_object(const visible_object *); diff --git a/include/mcmini/spy/checkpointing/tsan_support.h b/include/mcmini/spy/checkpointing/tsan_support.h new file mode 100644 index 00000000..b560acd0 --- /dev/null +++ b/include/mcmini/spy/checkpointing/tsan_support.h @@ -0,0 +1,41 @@ +#pragma once + +#ifdef __cplusplus +extern "C" { +#endif + +#include +#include + +/** + * Returns nonzero if thread `tid` currently has signal `signo` blocked, per + * the "SigBlk" field of /proc/self/task//status. Returns 0 if `tid` + * cannot be inspected (e.g. it has already exited). + */ +int thread_blocks_signal(pid_t tid, int signo); + +/** + * Returns the calling thread's real (unvirtualized) kernel tid. DMTCP + * virtualizes gettid() -- including the raw syscall(SYS_gettid) path -- so + * neither can be used here. Implemented via the kernel's /proc/thread-self + * magic symlink, whose target string is populated by the kernel itself and + * is not subject to userspace interception. This is a placeholder: replace + * the implementation with a DMTCP-provided "real tid" utility if/when one + * becomes available, without needing to change any caller. + */ +pid_t mc_real_tid(void); + +/** + * Returns true if the calling thread is an "external" thread as described + * for EXTERNAL_THREAD in record.h -- currently, specifically, ThreadSanitizer's + * own internal background thread (identified via thread_blocks_signal(), + * since that thread blocks all signals at creation). The result is computed + * once per thread and cached in thread-local storage, since the property + * being tested does not change over a thread's lifetime and the underlying + * check is not cheap enough to repeat on every call. + */ +bool mc_is_current_thread_tsan_internal(void); + +#ifdef __cplusplus +} +#endif diff --git a/include/mcmini/spy/intercept/interception.h b/include/mcmini/spy/intercept/interception.h index 105f024f..66c6640c 100644 --- a/include/mcmini/spy/intercept/interception.h +++ b/include/mcmini/spy/intercept/interception.h @@ -25,10 +25,30 @@ int libpthread_pthread_create(pthread_t *thread, const pthread_attr_t *attr, void *(*routine)(void *), void *arg); int libdmtcp_pthread_create(pthread_t *thread, const pthread_attr_t *attr, void *(*routine)(void *), void *arg); +// Resolved via dlsym(RTLD_NEXT, "pthread_create"), NOT libpthread_pthread_create's +// dlopen("libpthread")+dlsym handle: RTLD_NEXT finds whatever comes after +// libmcmini.so in this process's specific load order, which is TSan's own +// pthread_create interceptor in classic (non-DMTCP) mode -- where libmcmini +// loads ahead of libtsan -- letting a genuinely new thread register with TSan +// the normal way. Under DMTCP, where libtsan loads ahead of libmcmini, this +// still resolves past it to real glibc, identical to libpthread_pthread_create +// -- see mc_pthread_create()'s TARGET_BRANCH/TARGET_BRANCH_AFTER_RESTART case. +int tsan_or_real_pthread_create(pthread_t *thread, const pthread_attr_t *attr, + void *(*routine)(void *), void *arg); int pthread_join(pthread_t thread, void**); int libpthread_pthread_join(pthread_t thread, void**); int libdmtcp_pthread_join(pthread_t thread, void**); +// TSan-safe (libtsan-bypassing) handle for pthread_timedjoin_np, used by +// mc_pthread_join's RECORD loop. Calling pthread_timedjoin_np directly resolves +// to libtsan's interceptor, which trips a thread-registry CHECK. See +// TSAN-McMini-DMTCP.txt. +int libpthread_timedjoin_np(pthread_t thread, void**, const struct timespec*); + +MCMINI_NO_RETURN void pthread_exit(void *); +// TSan-safe (libtsan-bypassing) handle for pthread_exit, used by +// mc_pthread_exit's pre-restart forwarding path. +MCMINI_NO_RETURN void libpthread_pthread_exit(void *); int libpthread_mutex_init(pthread_mutex_t *, const pthread_mutexattr_t *); int libpthread_mutex_lock(pthread_mutex_t *); @@ -65,3 +85,12 @@ void abort(void); MCMINI_NO_RETURN void libc_abort(void); pid_t libc_fork(void); + +// Real, uninterposed __clone. Both libtsan (via its public clone() +// interceptor) and DMTCP (via its own strongly-exported __clone in +// threadwrappers.cpp, which aborts unless called from DMTCP's own +// pthread_create) intercept thread creation; restart_child_threads_fast() +// (dmtcp-callback.c) needs the raw libc primitive to bypass both. See +// TSAN-McMini-DMTCP.txt. +int libc_clone(int (*fn)(void *), void *child_stack, int flags, void *arg, + void *ptid, void *newtls, void *ctid); diff --git a/include/mcmini/spy/intercept/wrappers.h b/include/mcmini/spy/intercept/wrappers.h index 88208668..b36913b1 100644 --- a/include/mcmini/spy/intercept/wrappers.h +++ b/include/mcmini/spy/intercept/wrappers.h @@ -1,16 +1,28 @@ #pragma once #include +#include +#include "mcmini/defines.h" #include "mcmini/lib/entry.h" #include "mcmini/real_world/mailbox/runner_mailbox.h" +// See definition in wrappers.c: set while libmcmini creates one of its own +// helper threads, so mc_pthread_create creates it plainly (TSAN-visible, +// DMTCP-known) instead of as a model-checked user thread. +extern MCMINI_THREAD_LOCAL int mc_creating_internal_thread; + void thread_await_scheduler(void); void thread_wake_scheduler_and_wait(void); void thread_awake_scheduler_for_thread_finish_transition(void); void thread_handle_after_dmtcp_restart(void); volatile runner_mailbox *thread_get_mailbox(void); +// Whether `t` is one of the threads DMTCP resurrected via libc_clone() + +// setcontext() (see restart_child_threads_fast()), rather than one created +// normally after restart. Used by mc_pthread_exit() -- see that function. +bool mc_pthread_is_recreated_thread(pthread_t t); + int mc_pthread_mutex_init(pthread_mutex_t *mutex, const pthread_mutexattr_t *mutexattr); int mc_pthread_mutex_lock(pthread_mutex_t *mutex); @@ -20,6 +32,13 @@ int mc_pthread_create(pthread_t *thread, const pthread_attr_t *attr, int mc_sem_post(sem_t *); int mc_sem_wait(sem_t *); int mc_pthread_join(pthread_t, void**); +// See mc_pthread_join_impl() in wrappers.c: like mc_pthread_join(), but +// whenever a real join is warranted, defers the actual join call to the +// caller (setting *deferred = true) instead of performing it via +// libpthread_pthread_join(). For use only by __wrap_pthread_join() (see +// pthread_join_wrap.c), which must be linked directly into the target +// alongside `-Wl,--wrap=pthread_join`, so it can call __real_pthread_join(). +int mc_pthread_join_maybe_defer(pthread_t, void**, bool *deferred); int mc_sem_init(sem_t*, int, unsigned); int mc_sem_post(sem_t*); int mc_sem_wait(sem_t*); @@ -40,3 +59,4 @@ int mc_pthread_cond_destroy(pthread_cond_t *cond); void mc_exit_main_thread_in_child(void); MCMINI_NO_RETURN void mc_transparent_abort(void); MCMINI_NO_RETURN void mc_transparent_exit(int status); +MCMINI_NO_RETURN void mc_pthread_exit(void *retval); diff --git a/src/common/mem.c b/src/common/mem.c index 6e1b61aa..97cf8e43 100644 --- a/src/common/mem.c +++ b/src/common/mem.c @@ -1,5 +1,29 @@ #include "mcmini/mem.h" +#include +#include +#include + +// See mc_ts_alloc() in mem.h for why this allocator exists and why it must not +// call libc or a wrapped syscall on its fast path. +#define MC_TS_ARENA_SIZE (4u * 1024 * 1024) +static char mc_ts_arena[MC_TS_ARENA_SIZE]; +static atomic_size_t mc_ts_off = 0; + +void *mc_ts_alloc(size_t n) { + n = (n + 15u) & ~(size_t)15u; // 16-byte align + size_t off = atomic_fetch_add(&mc_ts_off, n); + if (off + n > MC_TS_ARENA_SIZE) { + // Exhausted. We may be running before this thread is registered with TSan, + // so we cannot fall back to malloc (it would enter a TSan interceptor). + // Fail via raw syscalls only (no interceptors). + static const char msg[] = "libmcmini: mc_ts_alloc arena exhausted\n"; + syscall(SYS_write, 2, msg, sizeof(msg) - 1); + syscall(SYS_exit_group, 1); + } + return &mc_ts_arena[off]; +} + volatile void *memset_v(volatile void *dst, int ch, size_t n) { volatile unsigned char *dstc = dst; while ((n--) > 0) dstc[n] = ch; diff --git a/src/common/multithreaded_fork.c b/src/common/multithreaded_fork.c index 7d60a103..6e80f760 100644 --- a/src/common/multithreaded_fork.c +++ b/src/common/multithreaded_fork.c @@ -72,6 +72,7 @@ #include "dmtcp.h" #ifdef MC_SHARED_LIBRARY +#include "mcmini/lib/log.h" #include "mcmini/real_world/process/template_process.h" #include "mcmini/spy/checkpointing/record.h" #include "mcmini/spy/intercept/interception.h" @@ -177,7 +178,7 @@ pid_t get_tid_from_pthread_descriptor(pthread_t pthread_descriptor) { int offset = pthreadDescriptorTidOffset(); pid_t ctid = *(pid_t*)((char*)(pthread_descriptor) + offset); #ifdef DMTCP - pid_t virttid = dmtcp_real_to_virtual_pid(ctid); + pid_t virttid = dmtcp_pid_real_to_virtual(ctid); ctid = (virttid ? virttid : ctid); #endif return ctid; @@ -209,7 +210,7 @@ int get_child_threads(int child_threads[]) { if (atoi(entry->d_name) != 0) { pid_t nexttid = atoi(entry->d_name); #ifdef DMTCP - pid_t virttid = dmtcp_real_to_virtual_pid(nexttid); + pid_t virttid = dmtcp_pid_real_to_virtual(nexttid); nexttid = (virttid ? virttid : nexttid); #endif child_threads[i++] = nexttid; @@ -401,6 +402,15 @@ int clone(int (*fn)(void *arg), void *child_stack, int flags, void *arg, #endif initialized = 0; // Reset for next call to multithreaded_fork() if (childpid == 0) { // child process + // _Fork() deliberately skips pthread_atfork() handlers, so any lock + // some other (non-surviving) thread held at the instant of forking is + // now permanently, unrecoverably stuck locked in this child -- nothing + // here ever calls the matching unlock. mcmini_log()'s own serialization + // mutex is a confirmed instance of this (see doc/log-mutex-fork-desync.txt); + // reset it here, while this is still the only thread alive in the + // child, before restart_child_threads() below recreates any others that + // might call it. + mcmini_log_reset_after_fork(); restart_child_threads(num_threads); } return childpid; diff --git a/src/common/runner_mailbox.c b/src/common/runner_mailbox.c index e11af991..25d9ec57 100644 --- a/src/common/runner_mailbox.c +++ b/src/common/runner_mailbox.c @@ -1,22 +1,55 @@ #include "mcmini/real_world/mailbox/runner_mailbox.h" -#include +#include +#include +#include #include -#include "mcmini/lib/log.h" #include "mcmini/defines.h" #include "mcmini/spy/intercept/interception.h" #include "string.h" +// child_side_sem's own raw-futex protocol (see runner_mailbox.h for why it +// isn't a glibc sem_t): a plain futex word used as a counting semaphore, +// always waking unconditionally on post, so there's no userspace "is anyone +// really waiting" bookkeeping to desync. +static long mc_futex(volatile uint32_t *uaddr, int futex_op, uint32_t val) { + return syscall(SYS_futex, uaddr, futex_op, val, NULL, NULL, 0); +} + +static int mc_raw_sem_wait(volatile uint32_t *sem) { + while (1) { + uint32_t cur = __atomic_load_n(sem, __ATOMIC_SEQ_CST); + if (cur > 0) { + uint32_t expected = cur; + if (__atomic_compare_exchange_n(sem, &expected, cur - 1, 0, + __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST)) { + return 0; + } + continue; + } + errno = 0; + long rc = mc_futex(sem, FUTEX_WAIT, 0); + if (rc == -1 && errno != EAGAIN && errno != EINTR) { + return -1; + } + } +} + +static int mc_raw_sem_post(volatile uint32_t *sem) { + __atomic_fetch_add(sem, 1, __ATOMIC_SEQ_CST); + mc_futex(sem, FUTEX_WAKE, 1); + return 0; +} + void mc_runner_mailbox_init(volatile runner_mailbox* r) { runner_mailbox_ref ref = (runner_mailbox_ref)(r); #ifdef MC_SHARED_LIBRARY libpthread_sem_init(&ref->model_side_sem, SEM_FLAG_SHARED, 0); - libpthread_sem_init(&ref->child_side_sem, SEM_FLAG_SHARED, 0); #else sem_init(&ref->model_side_sem, SEM_FLAG_SHARED, 0); - sem_init(&ref->child_side_sem, SEM_FLAG_SHARED, 0); #endif + __atomic_store_n(&ref->child_side_sem, 0, __ATOMIC_SEQ_CST); memset(ref->cnts, 0, sizeof(ref->cnts)); } @@ -24,11 +57,11 @@ void mc_runner_mailbox_destroy(volatile runner_mailbox* r) { runner_mailbox_ref ref = (runner_mailbox_ref)(r); #ifdef MC_SHARED_LIBRARY libpthread_sem_destroy(&ref->model_side_sem); - libpthread_sem_destroy(&ref->child_side_sem); #else sem_destroy(&ref->model_side_sem); - sem_destroy(&ref->child_side_sem); #endif + // child_side_sem is a plain futex word, not a glibc sem_t: nothing to + // destroy. } int mc_wait_for_thread(volatile runner_mailbox* r) { @@ -42,20 +75,12 @@ int mc_wait_for_thread(volatile runner_mailbox* r) { int mc_wait_for_scheduler(volatile runner_mailbox* r) { runner_mailbox_ref ref = (runner_mailbox_ref)(r); -#ifdef MC_SHARED_LIBRARY - return libpthread_sem_wait(&ref->child_side_sem); -#else - return sem_wait(&ref->child_side_sem); -#endif + return mc_raw_sem_wait(&ref->child_side_sem); } int mc_wake_thread(volatile runner_mailbox* r) { runner_mailbox_ref ref = (runner_mailbox_ref)(r); -#ifdef MC_SHARED_LIBRARY - return libpthread_sem_post(&ref->child_side_sem); -#else - return sem_post(&ref->child_side_sem); -#endif + return mc_raw_sem_post(&ref->child_side_sem); } int mc_wake_scheduler(volatile runner_mailbox* r) { diff --git a/src/examples/CMakeLists.txt b/src/examples/CMakeLists.txt index 5c056b14..90098b88 100644 --- a/src/examples/CMakeLists.txt +++ b/src/examples/CMakeLists.txt @@ -4,8 +4,165 @@ add_executable(cv-test cv-test.c) add_executable(deadly-embrace deadly-embrace.c) add_executable(fifo-example fifo.cpp) add_executable(producer-consumer producer-consumer.c) +add_executable(producer-consumer-safe producer-consumer-safe.c) +add_executable(producer-consumer-exit producer-consumer-exit.c) +add_executable(producer-consumer-park producer-consumer-park.c) +add_executable(exit-stress exit-stress.c) +add_executable(cv-producer-consumer cv-producer-consumer.c) +add_executable(cv-producer-consumer-safe cv-producer-consumer-safe.c) target_link_libraries(hello-world PUBLIC -pthread) target_link_libraries(cv-hello-world PUBLIC -pthread) target_link_libraries(cv-test PUBLIC -pthread) target_link_libraries(deadly-embrace PUBLIC -pthread) target_link_libraries(producer-consumer PUBLIC -pthread) +target_link_libraries(producer-consumer-safe PUBLIC -pthread) +target_link_libraries(producer-consumer-exit PUBLIC -pthread) +target_link_libraries(producer-consumer-park PUBLIC -pthread) +target_link_libraries(exit-stress PUBLIC -pthread) +target_link_libraries(cv-producer-consumer PUBLIC -pthread) +target_link_libraries(cv-producer-consumer-safe PUBLIC -pthread) + +# TSan-instrumented variant of producer-consumer, for testing McMini's +# DMTCP-restart + TSan integration (see CLAUDE.md, "DMTCP-restart under +# ThreadSanitizer"). Needs -Wl,--wrap=pthread_join plus +# src/lib/pthread_join_wrap.c compiled directly into this binary -- see +# that file's header comment for why it can't live in libmcmini.so itself. +add_executable(producer-consumer-tsan producer-consumer.c + ../lib/pthread_join_wrap.c) +target_compile_options(producer-consumer-tsan PUBLIC -fsanitize=thread) +target_link_options(producer-consumer-tsan PUBLIC -fsanitize=thread + -Wl,--wrap=pthread_join) +target_link_libraries(producer-consumer-tsan PUBLIC -pthread libmcmini) + +# do_recording() (src/mcmini/mcmini.cpp) resolves the plugin path as +# getcwd()/libmcmini.so, so mcmini must be invoked from the directory holding +# libmcmini.so -- i.e. CMAKE_BINARY_DIR, not this target's own output +# directory. Keep a copy there in sync on every build, so it can't silently +# go stale relative to libmcmini.so. +add_custom_command(TARGET producer-consumer-tsan POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy + $ + ${CMAKE_BINARY_DIR}/producer-consumer-tsan) + +# Same as producer-consumer-tsan, but built from producer-consumer-safe.c -- +# identical except for a fixed pre-existing data race in main() (each thread +# gets its own argument slot instead of sharing one reused array). Use this +# target when a test needs to rule out that race, so it can focus on other +# bugs (see doc/cond-wait-tsan-interceptor-bypass.txt's "Follow-on issue"). +add_executable(producer-consumer-safe-tsan producer-consumer-safe.c + ../lib/pthread_join_wrap.c) +target_compile_options(producer-consumer-safe-tsan PUBLIC -fsanitize=thread) +target_link_options(producer-consumer-safe-tsan PUBLIC -fsanitize=thread + -Wl,--wrap=pthread_join) +target_link_libraries(producer-consumer-safe-tsan PUBLIC -pthread libmcmini) + +add_custom_command(TARGET producer-consumer-safe-tsan POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy + $ + ${CMAKE_BINARY_DIR}/producer-consumer-safe-tsan) + +# Same as producer-consumer-safe-tsan, but worker threads terminate via an +# explicit pthread_exit() call instead of returning -- deliberately WITHOUT +# a pthread_exit wrap yet, to test whether that path has the same +# libtsan-loads-ahead-of-libmcmini load-order bypass already fixed for +# pthread_join/pthread_cond_wait (see those files' header comments). Our own +# pthread_exit is currently defined inside libmcmini.so itself +# (src/lib/interception.c), not compiled into the target the way join/ +# cond_wait are, which is exactly the pattern that bypass needs. +add_executable(producer-consumer-exit-tsan producer-consumer-exit.c + ../lib/pthread_join_wrap.c) +target_compile_options(producer-consumer-exit-tsan PUBLIC -fsanitize=thread) +target_link_options(producer-consumer-exit-tsan PUBLIC -fsanitize=thread + -Wl,--wrap=pthread_join) +target_link_libraries(producer-consumer-exit-tsan PUBLIC -pthread libmcmini) + +add_custom_command(TARGET producer-consumer-exit-tsan POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy + $ + ${CMAKE_BINARY_DIR}/producer-consumer-exit-tsan) + +# producer-consumer-park.c never calls pthread_join() or pthread_exit() -- +# main returns while both workers are still alive, parked forever on an +# unposted semaphore (see that file's own comment, modeled on +# multithreaded-fork-tsan-2.0's test_park.c). No --wrap needed: neither +# pthread_join nor pthread_cond_wait is ever called here. +add_executable(producer-consumer-park-tsan producer-consumer-park.c) +target_compile_options(producer-consumer-park-tsan PUBLIC -fsanitize=thread) +target_link_options(producer-consumer-park-tsan PUBLIC -fsanitize=thread) +target_link_libraries(producer-consumer-park-tsan PUBLIC -pthread libmcmini) + +add_custom_command(TARGET producer-consumer-park-tsan POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy + $ + ${CMAKE_BINARY_DIR}/producer-consumer-park-tsan) + +# Stress test for pthread_exit() robustness at higher thread counts (see +# exit-stress.c) -- same build recipe as producer-consumer-exit-tsan above. +add_executable(exit-stress-tsan exit-stress.c + ../lib/pthread_join_wrap.c) +target_compile_options(exit-stress-tsan PUBLIC -fsanitize=thread) +target_link_options(exit-stress-tsan PUBLIC -fsanitize=thread + -Wl,--wrap=pthread_join) +target_link_libraries(exit-stress-tsan PUBLIC -pthread libmcmini) + +add_custom_command(TARGET exit-stress-tsan POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy + $ + ${CMAKE_BINARY_DIR}/exit-stress-tsan) + +# Same build recipe as exit-stress-tsan above. Reliably (4/4 attempts) +# reproduces a genuine checkpoint-thread/TSAN-background-thread startup +# race, distinct from the restoreBuf={0,0} bug this was originally meant to +# help isolate -- see exit-stress-noop.c's own header comment. +add_executable(exit-stress-noop-tsan exit-stress-noop.c + ../lib/pthread_join_wrap.c) +target_compile_options(exit-stress-noop-tsan PUBLIC -fsanitize=thread) +target_link_options(exit-stress-noop-tsan PUBLIC -fsanitize=thread + -Wl,--wrap=pthread_join) +target_link_libraries(exit-stress-noop-tsan PUBLIC -pthread libmcmini) + +add_custom_command(TARGET exit-stress-noop-tsan POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy + $ + ${CMAKE_BINARY_DIR}/exit-stress-noop-tsan) + +# TSan-instrumented variant of cv-producer-consumer, for testing McMini's +# DMTCP-restart + TSan integration against condition variables (see +# doc/glibc-cond-var-desync.txt). Same recipe as producer-consumer-tsan +# above, plus -Wl,--wrap=pthread_cond_wait + pthread_cond_wait_wrap.c and +# -Wl,--wrap=pthread_cond_signal + pthread_cond_signal_wrap.c -- see those +# files' header comments for why each is needed. +add_executable(cv-producer-consumer-tsan cv-producer-consumer.c + ../lib/pthread_join_wrap.c + ../lib/pthread_cond_wait_wrap.c + ../lib/pthread_cond_signal_wrap.c) +target_compile_options(cv-producer-consumer-tsan PUBLIC -fsanitize=thread) +target_link_options(cv-producer-consumer-tsan PUBLIC -fsanitize=thread + -Wl,--wrap=pthread_join + -Wl,--wrap=pthread_cond_wait + -Wl,--wrap=pthread_cond_signal) +target_link_libraries(cv-producer-consumer-tsan PUBLIC -pthread libmcmini) + +# See the producer-consumer-tsan POST_BUILD step above for why this is needed. +add_custom_command(TARGET cv-producer-consumer-tsan POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy + $ + ${CMAKE_BINARY_DIR}/cv-producer-consumer-tsan) + +# Same as cv-producer-consumer-tsan, but built from cv-producer-consumer-safe.c +# -- see producer-consumer-safe-tsan above for why. +add_executable(cv-producer-consumer-safe-tsan cv-producer-consumer-safe.c + ../lib/pthread_join_wrap.c + ../lib/pthread_cond_wait_wrap.c + ../lib/pthread_cond_signal_wrap.c) +target_compile_options(cv-producer-consumer-safe-tsan PUBLIC -fsanitize=thread) +target_link_options(cv-producer-consumer-safe-tsan PUBLIC -fsanitize=thread + -Wl,--wrap=pthread_join + -Wl,--wrap=pthread_cond_wait + -Wl,--wrap=pthread_cond_signal) +target_link_libraries(cv-producer-consumer-safe-tsan PUBLIC -pthread libmcmini) + +add_custom_command(TARGET cv-producer-consumer-safe-tsan POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy + $ + ${CMAKE_BINARY_DIR}/cv-producer-consumer-safe-tsan) diff --git a/src/examples/cv-producer-consumer-safe.c b/src/examples/cv-producer-consumer-safe.c new file mode 100644 index 00000000..d6c890c8 --- /dev/null +++ b/src/examples/cv-producer-consumer-safe.c @@ -0,0 +1,97 @@ +#include +#include +#include +#include +#include + +#define MaxItems 1 +#define BufferSize 2 + +int in = 0; +int out = 0; +int count = 0; +int buffer[BufferSize]; +pthread_mutex_t mutex; +pthread_cond_t cond; +int DEBUG; + +void *producer(void *pno) +{ + int item; + for(int i = 0; i < MaxItems; i++) { + sleep(3); + item = rand(); // Produce an random item + pthread_mutex_lock(&mutex); + while (count == BufferSize) { + pthread_cond_wait(&cond, &mutex); + } + buffer[in] = item; + if (DEBUG) { + printf("Producer %d: Insert Item %d at %d\n", + *((int *)pno),buffer[in],in); + } + in = (in+1)%BufferSize; + count++; + pthread_cond_signal(&cond); + pthread_mutex_unlock(&mutex); + } + return NULL; +} + +void *consumer(void *cno) +{ + for(int i = 0; i < MaxItems; i++) { + sleep(3); + pthread_mutex_lock(&mutex); + while (count == 0) { + pthread_cond_wait(&cond, &mutex); + } + int item = buffer[out]; + if (DEBUG) { + printf("Consumer %d: Remove Item %d from %d\n", + *((int *)cno),item, out); + } + out = (out+1)%BufferSize; + count--; + pthread_cond_signal(&cond); + pthread_mutex_unlock(&mutex); + } + return NULL; +} + +int main(int argc, char* argv[]) +{ + int NUM_PRODUCERS = 1; + int NUM_CONSUMERS = 1; + DEBUG = 1; + + pthread_t pro[NUM_PRODUCERS],con[NUM_CONSUMERS]; + + pthread_mutex_init(&mutex, NULL); + pthread_cond_init(&cond, NULL); + + // Separate arrays per role: reusing one shared array (sized to the + // larger of the two counts) let main()'s second loop overwrite a + // producer's argument slot while that producer thread could still be + // reading it via pno -- a real, TSan-detectable data race. + int prod_ids[NUM_PRODUCERS]; + int cons_ids[NUM_CONSUMERS]; + + for(int i = 0; i < NUM_PRODUCERS; i++) { + prod_ids[i] = i+1; + pthread_create(&pro[i], NULL, producer, (void *)&prod_ids[i]); + } + for(int i = 0; i < NUM_CONSUMERS; i++) { + cons_ids[i] = i+1; + pthread_create(&con[i], NULL, consumer, (void *)&cons_ids[i]); + } + + for(int i = 0; i < NUM_PRODUCERS; i++) { + pthread_join(pro[i], NULL); + } + for(int i = 0; i < NUM_CONSUMERS; i++) { + pthread_join(con[i], NULL); + } + + return 0; +} diff --git a/src/examples/cv-producer-consumer.c b/src/examples/cv-producer-consumer.c new file mode 100644 index 00000000..68ef71cd --- /dev/null +++ b/src/examples/cv-producer-consumer.c @@ -0,0 +1,92 @@ +#include +#include +#include +#include +#include + +#define MaxItems 1 +#define BufferSize 2 + +int in = 0; +int out = 0; +int count = 0; +int buffer[BufferSize]; +pthread_mutex_t mutex; +pthread_cond_t cond; +int DEBUG; + +void *producer(void *pno) +{ + int item; + for(int i = 0; i < MaxItems; i++) { + sleep(3); + item = rand(); // Produce an random item + pthread_mutex_lock(&mutex); + while (count == BufferSize) { + pthread_cond_wait(&cond, &mutex); + } + buffer[in] = item; + if (DEBUG) { + printf("Producer %d: Insert Item %d at %d\n", + *((int *)pno),buffer[in],in); + } + in = (in+1)%BufferSize; + count++; + pthread_cond_signal(&cond); + pthread_mutex_unlock(&mutex); + } + return NULL; +} + +void *consumer(void *cno) +{ + for(int i = 0; i < MaxItems; i++) { + sleep(3); + pthread_mutex_lock(&mutex); + while (count == 0) { + pthread_cond_wait(&cond, &mutex); + } + int item = buffer[out]; + if (DEBUG) { + printf("Consumer %d: Remove Item %d from %d\n", + *((int *)cno),item, out); + } + out = (out+1)%BufferSize; + count--; + pthread_cond_signal(&cond); + pthread_mutex_unlock(&mutex); + } + return NULL; +} + +int main(int argc, char* argv[]) +{ + int NUM_PRODUCERS = 1; + int NUM_CONSUMERS = 1; + DEBUG = 1; + + pthread_t pro[NUM_PRODUCERS],con[NUM_CONSUMERS]; + + pthread_mutex_init(&mutex, NULL); + pthread_cond_init(&cond, NULL); + + int a[NUM_PRODUCERS > NUM_CONSUMERS ? NUM_PRODUCERS : NUM_CONSUMERS]; + + for(int i = 0; i < NUM_PRODUCERS; i++) { + a[i] = i+1; + pthread_create(&pro[i], NULL, producer, (void *)&a[i]); + } + for(int i = 0; i < NUM_CONSUMERS; i++) { + a[i] = i+1; + pthread_create(&con[i], NULL, consumer, (void *)&a[i]); + } + + for(int i = 0; i < NUM_PRODUCERS; i++) { + pthread_join(pro[i], NULL); + } + for(int i = 0; i < NUM_CONSUMERS; i++) { + pthread_join(con[i], NULL); + } + + return 0; +} diff --git a/src/examples/exit-stress-noop.c b/src/examples/exit-stress-noop.c new file mode 100644 index 00000000..d03b087d --- /dev/null +++ b/src/examples/exit-stress-noop.c @@ -0,0 +1,48 @@ +// Originally written as a bisection variant of exit-stress.c, to isolate +// whether the DMTCP restoreBuf={0,0} race needs the wrapped mutex/ +// pthread_exit calls specifically, or just needs 4 libmcmini-wrapped +// threads. It turned out NOT to be useful for that (see conversation): this +// binary instead reliably (4/4 attempts) crashes much earlier, at +// library-load time, before main() or any worker code ever runs -- a +// genuine race between DMTCP's checkpoint-thread creation and TSAN's own +// background-thread spawning, both interacting with libmcmini's lazy-init +// safety net (every wrapped pthread/sem call unconditionally calls +// libmcmini_init(), which takes a pthread_once guard). Concretely: the +// just-created checkpoint thread calls a wrapped sem_post() essentially +// immediately, whose libmcmini_init() safety check crashes inside TSAN's +// own __tsan::SlotLock, apparently because TSAN's own per-thread setup for +// this brand-new thread hasn't finished yet. The original exit-stress.c +// (mutex + explicit pthread_exit) does NOT hit this in the same testing +// session -- with ASLR disabled, a different binary's code/data layout +// shifts the race's timing enough to reliably resolve the other way, even +// though the underlying mechanism is presumably the same for both. Kept as +// a *reliable* reproducer for this crash (much easier to work with than the +// original, rarely-reproducing "elusive heisenbug" noted elsewhere in +// project history), not as a bisection tool. +#include +#include +#include + +#define NUM_THREADS 4 + +void *worker(void *arg) { + int id = *(int *)arg; + sleep(10); + printf("Worker %d: done\n", id); + pthread_exit(NULL); +} + +int main(void) { + pthread_t threads[NUM_THREADS]; + int ids[NUM_THREADS]; + + for (int i = 0; i < NUM_THREADS; i++) { + ids[i] = i; + pthread_create(&threads[i], NULL, worker, &ids[i]); + } + for (int i = 0; i < NUM_THREADS; i++) { + pthread_join(threads[i], NULL); + } + + return 0; +} diff --git a/src/examples/exit-stress.c b/src/examples/exit-stress.c new file mode 100644 index 00000000..36205583 --- /dev/null +++ b/src/examples/exit-stress.c @@ -0,0 +1,52 @@ +// Stress test for pthread_exit() robustness at higher thread counts under +// DMTCP+TSan restart. doc/pthread-exit-abort-and-fiber-crash.txt and +// multithreaded-fork-tsan-2.0's own README flag a rare (~1-in-30) fiber +// shadow-call-stack overflow specifically at 8+ concurrently-exiting +// threads -- never exercised at this thread count in this repo before +// (every other -exit target here uses 1-2 threads). +// +// NUM_THREADS workers each sleep briefly (giving a checkpoint interval, +// `mcmini -i N`, a reliable window to land while every thread is still +// alive, pre-exit), do a small amount of real, TSan-instrumented work, +// then terminate via an explicit pthread_exit() call. After a +// --multithreaded-fork restart, all NUM_THREADS recreated threads +// (clone()+setcontext()'d onto a fresh TSan fiber -- see +// mc_pthread_is_recreated_thread() in dmtcp-callback.c) hit +// mc_pthread_exit()'s recreated-thread path concurrently. +#include +#include +#include + +#define NUM_THREADS 4 + +pthread_mutex_t counter_mutex; +int counter = 0; + +void *worker(void *arg) { + int id = *(int *)arg; + sleep(10); // original bug-reproduction timing: short enough that a tight + // -i 5 checkpoint interval used to race against DMTCP's own + // ProcessInfo::init() and lose (restoreBuf={0,0}) at 4+ threads. + pthread_mutex_lock(&counter_mutex); + counter++; + printf("Worker %d: counter now %d\n", id, counter); + pthread_mutex_unlock(&counter_mutex); + pthread_exit(NULL); +} + +int main(void) { + pthread_t threads[NUM_THREADS]; + int ids[NUM_THREADS]; + + pthread_mutex_init(&counter_mutex, NULL); + + for (int i = 0; i < NUM_THREADS; i++) { + ids[i] = i; + pthread_create(&threads[i], NULL, worker, &ids[i]); + } + for (int i = 0; i < NUM_THREADS; i++) { + pthread_join(threads[i], NULL); + } + + return 0; +} diff --git a/src/examples/producer-consumer-exit.c b/src/examples/producer-consumer-exit.c new file mode 100644 index 00000000..12f80f15 --- /dev/null +++ b/src/examples/producer-consumer-exit.c @@ -0,0 +1,93 @@ +#include +#include +#include +#include +#include +#include + +#define MaxItems 1 +#define BufferSize 2 + +sem_t empty; +sem_t full; +int in = 0; +int out = 0; +int buffer[BufferSize]; +pthread_mutex_t mutex; +int DEBUG; + +// Same as producer-consumer-safe.c, except each worker thread terminates via +// an explicit pthread_exit() call instead of returning from its start +// routine -- exercises the pthread_exit path specifically (see +// multithreaded-fork-tsan-2.0's test_exit.c for the standalone analog). +void *producer(void *pno) +{ + int item; + for(int i = 0; i < MaxItems; i++) { + sleep(3); + item = rand(); // Produce an random item + sem_wait(&empty); + pthread_mutex_lock(&mutex); + buffer[in] = item; + if (DEBUG) { + printf("Producer %d: Insert Item %d at %d\n", + *((int *)pno),buffer[in],in); + } + in = (in+1)%BufferSize; + pthread_mutex_unlock(&mutex); + sem_post(&full); + } + pthread_exit(NULL); +} + +void *consumer(void *cno) +{ + for(int i = 0; i < MaxItems; i++) { + sleep(3); + sem_wait(&full); + pthread_mutex_lock(&mutex); + int item = buffer[out]; + if (DEBUG) { + printf("Consumer %d: Remove Item %d from %d\n", + *((int *)cno),item, out); + } + out = (out+1)%BufferSize; + pthread_mutex_unlock(&mutex); + sem_post(&empty); + } + pthread_exit(NULL); +} + +int main(int argc, char* argv[]) +{ + int NUM_PRODUCERS = 1; + int NUM_CONSUMERS = 1; + DEBUG = 1; + + pthread_t pro[NUM_PRODUCERS],con[NUM_CONSUMERS]; + + pthread_mutex_init(&mutex, NULL); + sem_init(&empty,0,BufferSize); + sem_init(&full,0,0); + + int prod_ids[NUM_PRODUCERS]; + int cons_ids[NUM_CONSUMERS]; + + for(int i = 0; i < NUM_PRODUCERS; i++) { + prod_ids[i] = i+1; + pthread_create(&pro[i], NULL, producer, (void *)&prod_ids[i]); + } + for(int i = 0; i < NUM_CONSUMERS; i++) { + cons_ids[i] = i+1; + pthread_create(&con[i], NULL, consumer, (void *)&cons_ids[i]); + } + + for(int i = 0; i < NUM_PRODUCERS; i++) { + pthread_join(pro[i], NULL); + } + for(int i = 0; i < NUM_CONSUMERS; i++) { + pthread_join(con[i], NULL); + } + + return 0; +} diff --git a/src/examples/producer-consumer-park.c b/src/examples/producer-consumer-park.c new file mode 100644 index 00000000..67e0c26d --- /dev/null +++ b/src/examples/producer-consumer-park.c @@ -0,0 +1,147 @@ +#include +#include +#include +#include +#include +#include +#include + +#define MaxItems 1 +#define BufferSize 2 + +// Unlike sleep(), a busy-wait can't be cut short by DMTCP's own +// pre-checkpoint signal (which interrupts blocking syscalls, returning +// early with time remaining) -- needed below so main's own delay reliably +// lasts the full duration instead of racing main's return/exit() against +// an in-progress checkpoint write. +static void busy_wait_seconds(int seconds) { + struct timespec start, now; + clock_gettime(CLOCK_MONOTONIC, &start); + do { + clock_gettime(CLOCK_MONOTONIC, &now); + } while (now.tv_sec - start.tv_sec < seconds); +} + +// Same as producer-consumer-safe.c, except main never calls pthread_join(): +// each worker signals `done` once its real work finishes, then parks forever +// on an unposted semaphore, and main returns as soon as both `done`s are +// posted -- exercising a real, alive-but-parked (never joined/exited) thread +// still present when the process ends, matching multithreaded-fork-tsan-2.0's +// test_park.c ("thread resurrection ... without pthread_join/pthread_exit"). +// +// NOTE: main() returning here (rather than e.g. pthread_exit()) triggers +// POSIX exit() semantics -- the whole process tears down immediately, +// including TSan's own exit-time Finalize()/ThreadCount() bookkeeping, which +// reuses main()'s now-dead stack slots (confirmed live via a gdb hardware +// watchpoint: the exact address that held prod_ids[0] gets overwritten by +// TSan's internal "alive thread count" a moment later). This can show up as +// an apparently-garbled producer/consumer id in RECORD-mode's printf output +// on a rare timing -- harmless (pure RECORD-mode artifact, before any +// checkpoint/restart), not a libmcmini/DMTCP bug, and not the subject of +// this file's actual test (multithreaded_fork's recreated-but-unjoined +// thread handling under --multithreaded-fork restart). +sem_t empty; +sem_t full; +sem_t producer_done; +sem_t consumer_done; +sem_t park; // never posted +int in = 0; +int out = 0; +int buffer[BufferSize]; +pthread_mutex_t mutex; +int DEBUG; + +static void sem_wait_retry(sem_t *s) { while (sem_wait(s) != 0) /* EINTR */; } + +void *producer(void *pno) +{ + int item; + for(int i = 0; i < MaxItems; i++) { + sleep(3); + item = rand(); // Produce an random item + sem_wait(&empty); + pthread_mutex_lock(&mutex); + buffer[in] = item; + if (DEBUG) { + printf("Producer %d: Insert Item %d at %d\n", + *((int *)pno),buffer[in],in); + } + in = (in+1)%BufferSize; + pthread_mutex_unlock(&mutex); + sem_post(&full); + } + sem_post(&producer_done); + sem_wait_retry(&park); // park forever; never exit/join + return NULL; +} + +void *consumer(void *cno) +{ + for(int i = 0; i < MaxItems; i++) { + sleep(3); + sem_wait(&full); + pthread_mutex_lock(&mutex); + int item = buffer[out]; + if (DEBUG) { + printf("Consumer %d: Remove Item %d from %d\n", + *((int *)cno),item, out); + } + out = (out+1)%BufferSize; + pthread_mutex_unlock(&mutex); + sem_post(&empty); + } + sem_post(&consumer_done); + sem_wait_retry(&park); // park forever; never exit/join + return NULL; +} + +int main(int argc, char* argv[]) +{ + int NUM_PRODUCERS = 1; + int NUM_CONSUMERS = 1; + DEBUG = 1; + + pthread_t pro[NUM_PRODUCERS],con[NUM_CONSUMERS]; + + pthread_mutex_init(&mutex, NULL); + sem_init(&empty,0,BufferSize); + sem_init(&full,0,0); + sem_init(&producer_done,0,0); + sem_init(&consumer_done,0,0); + sem_init(&park,0,0); + + int prod_ids[NUM_PRODUCERS]; + int cons_ids[NUM_CONSUMERS]; + + for(int i = 0; i < NUM_PRODUCERS; i++) { + prod_ids[i] = i+1; + pthread_create(&pro[i], NULL, producer, (void *)&prod_ids[i]); + } + for(int i = 0; i < NUM_CONSUMERS; i++) { + cons_ids[i] = i+1; + pthread_create(&con[i], NULL, consumer, (void *)&cons_ids[i]); + } + + for(int i = 0; i < NUM_PRODUCERS; i++) { + sem_wait(&producer_done); + } + for(int i = 0; i < NUM_CONSUMERS; i++) { + sem_wait(&consumer_done); + } + // Widen the window where both workers are safely parked (on `park`) + // but main hasn't yet returned/exited -- gives a checkpoint interval + // (mcmini -i N) a reliable target to land on, instead of racing main's + // own exit() against the workers' last bit of real work (see this + // file's top-of-file comment on the TSan-Finalize()/stack-reuse + // artifact that showed up when that race was too tight). + busy_wait_seconds(10); + // Deliberately no pthread_join(): producer/consumer are still alive, + // parked on `park`, when the process ends. + + // Plain return, deliberately NOT an explicit exit(0) call: this is now + // a live regression test for interception.c's __libc_start_main hook, + // which runs mc_transparent_exit() after main() returns however it + // returns, catching exactly this case -- see that file for why a plain + // return used to bypass every exit()/_exit() interposition technique. + return 0; +} diff --git a/src/examples/producer-consumer-safe.c b/src/examples/producer-consumer-safe.c new file mode 100644 index 00000000..ab8214be --- /dev/null +++ b/src/examples/producer-consumer-safe.c @@ -0,0 +1,93 @@ +#include +#include +#include +#include +#include +#include + +#define MaxItems 1 +#define BufferSize 2 + +sem_t empty; +sem_t full; +int in = 0; +int out = 0; +int buffer[BufferSize]; +pthread_mutex_t mutex; +int DEBUG; + +void *producer(void *pno) +{ + int item; + for(int i = 0; i < MaxItems; i++) { + sleep(3); + item = rand(); // Produce an random item + sem_wait(&empty); + pthread_mutex_lock(&mutex); + buffer[in] = item; + if (DEBUG) { + printf("Producer %d: Insert Item %d at %d\n", + *((int *)pno),buffer[in],in); + } + in = (in+1)%BufferSize; + pthread_mutex_unlock(&mutex); + sem_post(&full); + } + return NULL; +} + +void *consumer(void *cno) +{ + for(int i = 0; i < MaxItems; i++) { + sleep(3); + sem_wait(&full); + pthread_mutex_lock(&mutex); + int item = buffer[out]; + if (DEBUG) { + printf("Consumer %d: Remove Item %d from %d\n", + *((int *)cno),item, out); + } + out = (out+1)%BufferSize; + pthread_mutex_unlock(&mutex); + sem_post(&empty); + } + return NULL; +} + +int main(int argc, char* argv[]) +{ + int NUM_PRODUCERS = 1; + int NUM_CONSUMERS = 1; + DEBUG = 1; + + pthread_t pro[NUM_PRODUCERS],con[NUM_CONSUMERS]; + + pthread_mutex_init(&mutex, NULL); + sem_init(&empty,0,BufferSize); + sem_init(&full,0,0); + + // Separate arrays per role: reusing one shared array (sized to the + // larger of the two counts) let main()'s second loop overwrite a + // producer's argument slot while that producer thread could still be + // reading it via pno -- a real, TSan-detectable data race. + int prod_ids[NUM_PRODUCERS]; + int cons_ids[NUM_CONSUMERS]; + + for(int i = 0; i < NUM_PRODUCERS; i++) { + prod_ids[i] = i+1; + pthread_create(&pro[i], NULL, producer, (void *)&prod_ids[i]); + } + for(int i = 0; i < NUM_CONSUMERS; i++) { + cons_ids[i] = i+1; + pthread_create(&con[i], NULL, consumer, (void *)&cons_ids[i]); + } + + for(int i = 0; i < NUM_PRODUCERS; i++) { + pthread_join(pro[i], NULL); + } + for(int i = 0; i < NUM_CONSUMERS; i++) { + pthread_join(con[i], NULL); + } + + return 0; +} diff --git a/src/lib/dmtcp-callback.c b/src/lib/dmtcp-callback.c index 7438c57a..7190cb08 100644 --- a/src/lib/dmtcp-callback.c +++ b/src/lib/dmtcp-callback.c @@ -1,6 +1,5 @@ #define _GNU_SOURCE #include -#include #include #include // man 2 open #include @@ -23,6 +22,53 @@ #define SIG_MULTITHREADED_FORK (SIGRTMIN+6) +// ThreadSanitizer Fiber API (weak: resolves to the TSAN runtime only for TSAN +// targets, NULL no-op otherwise). A "fiber" is a TSAN ThreadState decoupled +// from the OS thread. `multithreaded_fork` recreates the pre-checkpoint threads +// with clone() + setcontext (so their original %fs/TLS and pthread descriptor +// are preserved and libmcmini does not intercept the creation). But clone() +// means libtsan never registered these threads, so their cur_thread() is torn +// on resume and the first TSAN-intercepted call (e.g. munmap) faults. Mirroring +// DMTCP's own restore fix (threadlist.cpp: "fresh fiber on restart"), each +// resurrected thread switches onto a fresh fiber the moment it resumes, giving +// it a valid ThreadState before any traced call. +extern void *__tsan_create_fiber(unsigned flags) __attribute__((weak)); +extern void __tsan_switch_to_fiber(void *fiber, unsigned flags) + __attribute__((weak)); +extern void __tsan_destroy_fiber(void *fiber) __attribute__((weak)); + +// TSan fork syscall hooks (weak). `_Fork()` bypasses libtsan's fork() +// interceptor, so TSan's BeforeFork/AfterFork pipeline never runs: the child +// inherits TSan's runtime with internal locks still held and a ThreadRegistry +// full of now-dead parent threads. The first instrumented call in the child +// then faults. Bracketing `_Fork()` with these hooks re-runs that pipeline -- +// pre acquires TSan's internal locks (parent), post releases them and, in the +// child, scrubs the registry down to the calling thread. This MUST happen +// before any fiber work, since __tsan_create_fiber itself touches the registry. +extern void __sanitizer_syscall_pre_impl_fork(void) __attribute__((weak)); +extern void __sanitizer_syscall_post_impl_fork(long res) __attribute__((weak)); + +// libc's internal clone (NOT intercepted by libtsan, unlike the public +// clone()). libtsan's clone() interceptor treats every call as a fork +// (ForkChildAfter), which corrupts its thread-slot state for the +// CLONE_THREAD clone restart_child_threads_fast() performs below. +// +// DMTCP ALSO strongly exports its own `__clone` (threadwrappers.cpp), which +// unconditionally aborts unless the calling thread is mid-DMTCP's-own +// pthread_create -- confirmed empirically against a real DMTCP restart: +// "ASSERT ... Thread creation with clone syscall is not supported". Calling +// the symbol `__clone` directly hits THAT interceptor, not libc's real +// implementation, exactly like the public clone() hits libtsan's. Bypass +// both via libc_clone() (interception.c), which resolves the real __clone +// once at process startup -- well before any fork/checkpoint -- alongside +// this codebase's other real-libc forwarding functions (libc_abort(), +// libc_fork(), ...), rather than re-resolving it here via a fresh +// post-_Fork() dlopen/dlsym (which would risk deadlocking on a dynamic- +// linker lock left held by a thread that no longer exists in the child). +// The recreated thread's own fiber switch (see +// thread_handle_after_dmtcp_restart()) then gives it a valid TSan +// ThreadState. + // We probably won't need the '#undef', but just in case a .h file defined it: #undef dmtcp_mcmini_is_loaded int dmtcp_mcmini_is_loaded(void) { return 1; } @@ -42,8 +88,6 @@ struct threadinfo { // tlsAddr only used in __aarch64__ and __riscv // In fact, __riscv has the address in a normal register, restored w/ context. unsigned long int tlsAddr; - // The kernel has a process-wide sigmask, and also a per-thread sigmask. - sigset_t thread_sigmask; // glibc:pthread_create and pthread_self use this, but not the clone call: pthread_t pthread_descriptor; } threadInfos[1000]; @@ -125,27 +169,40 @@ static void saveThreadStateBeforeFork(struct threadinfo* threadInfo) { threadInfo->pthread_descriptor = pthread_self(); getTLSPointer(threadInfo); - // FIXME: Add func fo get/set signals in child thread of child process. - // and restore thread sigmask sfter setcontext. - pthread_sigmask(SIG_BLOCK, NULL, &threadInfo->thread_sigmask); - sigset_t sigtest; - pthread_sigmask(SIG_BLOCK, NULL, &sigtest); - sigdelset(&sigtest, SIG_MULTITHREADED_FORK); - if (! sigisemptyset(&sigtest)) { - fprintf(stderr, "PID %d: multithreaded_fork() not yet implemented" - " for non-empty thread signaks\n", getpid()); - libc_abort(); - } + // No manual signal-mask save/restore needed: getcontext()/setcontext() + // already save/restore the blocked-signal set via ucontext_t's uc_sigmask, + // even when setcontext() resumes on a brand-new clone()'d OS thread (see + // child_setcontext_fast() below). } static int child_setcontext_fast(void *arg) { struct threadinfo* threadInfo = arg; setTLSPointer(threadInfo); patchThreadDescriptor(threadInfo->pthread_descriptor); + // Does not return: jumps to the getcontext() call site in + // thread_handle_after_dmtcp_restart(), restoring uc_sigmask (see + // saveThreadStateBeforeFork() above) along with the rest of the context. setcontext(&(threadInfo->context)); return 0; // not reached } +// See wrappers.h for the rationale. threadInfos[0..threadIdx) holds one +// entry per real application thread alive at checkpoint time, NOT one per +// fast_multithreaded_fork() call: the atomic_fetch_add() below runs once +// per thread, before this function's own getcontext(); every later branch +// just resumes that same saved context via setcontext(). Verified: +// threadIdx never exceeded 5 across 13,794 fork() calls in one DPOR run. +bool mc_pthread_is_recreated_thread(pthread_t t) { + int maxThreadIdx = atomic_load(&threadIdx); + for (int i = 0; i < maxThreadIdx; i++) { + if (threadInfos[i].origTid != 0 && + pthread_equal(threadInfos[i].pthread_descriptor, t)) { + return true; + } + } + return false; +} + void restart_child_threads_fast(void) { int maxThreadIdx = atomic_load(&threadIdx); for (int i = 0; i < maxThreadIdx; i++) { @@ -163,10 +220,13 @@ void restart_child_threads_fast(void) { pid_t *ctid = (pid_t*)((char*)threadInfos[i].pthread_descriptor + offset); pid_t *ptid = ctid; // For more insight, read 'man set_tid_address'. - clone(child_setcontext_fast, - stack, - clone_flags, - (void *)&threadInfos[i], ptid, threadInfos[i].fs, ctid); + // R3: use libc's raw __clone (via libc_clone(), see above), bypassing + // both libtsan's and DMTCP's own interception of the public clone()/ + // __clone symbols. + libc_clone(child_setcontext_fast, + stack, + clone_flags, + (void *)&threadInfos[i], ptid, (void *)threadInfos[i].fs, ctid); } } @@ -191,40 +251,42 @@ pid_t fast_multithreaded_fork(void) { * ret = INLINE_SYSCALL_CALL (clone, flags, 0, NULL, ctid, 0); *} *********************************************************************/ -#if 1 pid_t _Fork(); + // Re-run TSan's fork pipeline around the raw _Fork() (see the syscall-hook + // declarations above). pre = BeforeFork (parent acquires TSan locks); + // post = AfterFork (release; child scrubs the ThreadRegistry). + if (__sanitizer_syscall_pre_impl_fork != NULL) { + __sanitizer_syscall_pre_impl_fork(); + } int childpid = _Fork(); -#else - // NOT YET FULLY DEVELOPED: - int flags = CLONE_CHILD_SETTID | CLONE_CHILD_CLEARTID | SIGCHLD; - int childpid; -// syscall(SYS_clone, ...); -// stack must be NULL -// https://stackoverflow.com/questions/2898579/clone-equivalent-of-fork -// But that says to use only SIGCHLD for flags, and glibc uses the above. -// But it's okay, since we're setting ctid and tls to NULL. -// FIXME: If we're going to set the last 3 args to NULL, who cares in what order they're found! -# ifdef __x86_64__ - long clone(unsigned long flags, void *stack, - int *parent_tid, int *child_tid, - unsigned long tls); -# elif defined(__aarch64__) - long clone(unsigned long flags, void *stack, - int *parent_tid, unsigned long tls, - int *child_tid); -# elif defined(__riscv) -# error Unimplemented CPU architecture -https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/riscv/clone.S -int clone(int (*fn)(void *arg), void *child_stack, int flags, void *arg, - void *parent_tidptr, void *tls, void *child_tidptr) */ - /* The syscall expects the args to be in different slots. */ - mv a0,a2 - mv a2,a4 - mv a3,a5 - mv a4,a6 -# endif -#endif + if (__sanitizer_syscall_post_impl_fork != NULL) { + __sanitizer_syscall_post_impl_fork(childpid); + } if (childpid == 0) { // child process + // R4 (remainder): the forking thread keeps its inherited (fork-copied) + // TSan ThreadState, whose shadow call stack starts at the parent's + // fork-time depth and can overflow as this thread keeps running. Switch + // it onto a fresh fiber too, mirroring the recreated-thread fiber switch + // in thread_handle_after_dmtcp_restart(). Weak symbol: a no-op for + // non-TSan targets. + if (__tsan_switch_to_fiber != NULL) { + __tsan_switch_to_fiber(__tsan_create_fiber(0), 0); + } + // _Fork() (unlike a real fork()) deliberately skips pthread_atfork() + // handlers, so mcmini_log()'s own log_mut can be left stranded locked + // if some other thread held it at the exact instant _Fork() snapshotted + // memory for this branch -- see doc/log-mutex-fork-desync.txt. This + // reset MUST run here, before restart_child_threads_fast() recreates + // any other thread, while this is still the only OS thread alive in + // the child, so it can never race a concurrent lock/unlock attempt. + // + // NOTE: an earlier version of this fix was applied to + // src/common/multithreaded_fork.c's multithreaded_fork() instead -- + // that function turned out to be dead code (superseded by this one, + // fast_multithreaded_fork(), per commit 74679cf), so the fix was never + // actually active on the real --multithreaded-fork path. This is the + // correct location. + mcmini_log_reset_after_fork(); restart_child_threads_fast(); } return childpid; @@ -256,7 +318,17 @@ void thread_handle_after_dmtcp_restart(void) { notify_template_thread(); } else { - // Returned from `getcontext()` in the forked child + // Returned from `getcontext()` in the forked child: this thread was just + // resurrected by `restart_child_threads_fast` via clone() + setcontext. + // Before any TSAN-intercepted call below, switch onto a fresh TSAN + // fiber, so this OS thread has a valid ThreadState (clone() bypassed + // libtsan's pthread_create registration, leaving cur_thread() torn). + // Weak symbol: a no-op for non-TSAN targets. The fiber is intentionally + // not destroyed -- the branch process is short-lived and exits after + // one trace. + if (__tsan_switch_to_fiber != NULL) { + __tsan_switch_to_fiber(__tsan_create_fiber(0), 0); + } } switch (mode_on_entry) { @@ -321,23 +393,27 @@ static void *template_thread(void *unused) { // to ensure a stable recorded state is pointless: we're not going to read it // anyway! This is an only a potential optimization for later though. + // Counting via a live /proc/self/task scan is a TOCTOU race: DMTCP + // recreates checkpointed threads asynchronously via clone(), so a scan + // that runs before it has finished recreating all of them undercounts, + // and this barrier then releases before every thread has actually + // restarted (confirmed empirically: one thread's own restart-completion + // signal can arrive after this barrier already declared a "consistent + // state"). + // + // head_record_mode instead gives an exact, race-free count: it only ever + // gets THREAD entries for genuine target threads (the template thread and + // the checkpoint thread never go through libmcmini's wrapped + // pthread_create(), so neither is ever recorded here), and -- since a + // DMTCP checkpoint is a full memory snapshot -- this list is preserved + // exactly as it was at record time across every restart, with no + // dependence on restart-time scheduling. int thread_count = 0; - struct dirent *entry; - DIR *dp = opendir("/proc/self/task"); - if (dp == NULL) { - perror("opendir"); - mc_exit(EXIT_FAILURE); - } - - while ((entry = readdir(dp))) - if (strcmp(entry->d_name, ".") != 0 && strcmp(entry->d_name, "..") != 0) + for (rec_list *entry = head_record_mode; entry != NULL; entry = entry->next) { + if (entry->vo.type == THREAD && entry->vo.thrd_state.status == ALIVE) { thread_count++; - - // We don't want to count the template thread nor - // the checkpoint thread, but these will appear in - // `/proc/self/tasks` - thread_count -= 2; - closedir(dp); + } + } log_debug( "There are %d threads... waiting for them to get into a consistent " "state...\n", @@ -403,8 +479,8 @@ static void *template_thread(void *unused) { for (rec_list *entry = head_record_mode; entry != NULL; entry = entry->next) { if (entry->vo.type == MUTEX) { - log_verbose("Writing mutex entry %p (state %d)\n", entry->vo.location, - entry->vo.mut_state); + log_verbose("Writing mutex entry %p (state %d, owner %d)\n", entry->vo.location, + entry->vo.mut_state.status, entry->vo.mut_state.owner); } else if (entry->vo.type == THREAD) { log_verbose("Writing thread entry %p (id %d, status: %d)\n", (void *)entry->vo.thrd_state.pthread_desc, @@ -535,7 +611,16 @@ __attribute__((constructor)) void libmcmini_event_late_init() { pthread_attr_init(&attr); pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); libpthread_sem_init(&template_thread_sem, 0, 0); - libdmtcp_pthread_create(&template_thread_id, &attr, &template_thread, NULL); + // Create via the PUBLIC pthread_create (not libdmtcp_pthread_create), so + // that, when the target is instrumented, libtsan's pthread_create + // interceptor sees and registers this thread -- otherwise its (absent) + // TSAN ThreadState makes restart crash inside libtsan's setjmp/longjmp + // restore. mc_pthread_create recognizes the flag and still routes the + // actual creation through DMTCP without user-thread machinery. See + // TSAN-McMini-DMTCP.txt. + mc_creating_internal_thread = 1; + pthread_create(&template_thread_id, &attr, &template_thread, NULL); + mc_creating_internal_thread = 0; pthread_attr_destroy(&attr); } @@ -579,6 +664,14 @@ static void presuspend_eventHook(DmtcpEvent_t event, DmtcpEventData_t *data) { } else { set_current_mode(DMTCP_RESTART_INTO_BRANCH); log_debug("`MCMINI_TEMPLATE_LOOP` was not set at restart-time\n"); + + // A DMTCP_RESTART_INTO_BRANCH process explores exactly one trace, + // then this process is discarded -- it will never legitimately be + // asked to checkpoint again. Left alone, the checkpoint thread would + // resume its normal sleep-checkpoint-resume loop and block forever + // waiting for a checkpoint request from this restart's (one-shot, + // otherwise idle) coordinator. + dmtcp_skip_post_restart_checkpoint_loop(); } // During record mode, the shared memory diff --git a/src/lib/interception.c b/src/lib/interception.c index 6d0b66f6..ff9b5a4f 100644 --- a/src/lib/interception.c +++ b/src/lib/interception.c @@ -12,10 +12,21 @@ pthread_once_t libmcini_init = PTHREAD_ONCE_INIT; +// Set once mc_load_intercepted_pthread_functions() has fully completed. +// Plain flag, not itself pthread_once/TSAN-intercepted. libmcmini_init() +// checks this directly (see below), so that once initialization has truly +// completed, EVERY later call -- from any thread, including ones with no +// TSAN ThreadState of their own -- takes a plain-memory fast path and never +// touches pthread_once()/TSAN's interceptor at all. +static bool libmcmini_init_done = false; + typeof(&pthread_create) libpthread_pthread_create_ptr; typeof(&pthread_create) libdmtcp_pthread_create_ptr; +typeof(&pthread_create) tsan_or_real_pthread_create_ptr; typeof(&pthread_join) libpthread_pthread_join_ptr; typeof(&pthread_join) libdmtcp_pthread_join_ptr; +typeof(&pthread_timedjoin_np) libpthread_timedjoin_np_ptr; +__attribute__((__noreturn__)) typeof(&pthread_exit) libpthread_pthread_exit_ptr; typeof(&pthread_mutex_init) pthread_mutex_init_ptr; typeof(&pthread_mutex_lock) pthread_mutex_lock_ptr; typeof(&pthread_mutex_trylock) pthread_mutex_trylock_ptr; @@ -37,9 +48,24 @@ typeof(&sleep) sleep_ptr; __attribute__((__noreturn__)) typeof(&exit) exit_ptr; __attribute__((__noreturn__)) typeof(&abort) abort_ptr; typeof(&fork) fork_ptr; +typeof(&libc_clone) clone_ptr; void libmcmini_init(void) { + // Fast path: a brand-new thread that hasn't gone through TSAN's own + // pthread_create interceptor yet (e.g. TSAN's own lazily-spawned + // background thread, or a DMTCP-under-TSAN worker thread whose prologue + // runs before TSAN's registration trampoline -- see wrappers.c's + // dmtcp_create_checkpoint_thread_wrapper() and mc_thread_routine_wrapper()) + // has no TSAN ThreadState. pthread_once() itself is TSAN-intercepted and + // crashes dereferencing that nonexistent ThreadState. Once initialization + // has genuinely completed once (by anyone), there's nothing left to do, so + // skip pthread_once() -- and the TSAN interceptor call it would make -- + // entirely. + if (libmcmini_init_done) { + return; + } pthread_once(&libmcini_init, &mc_load_intercepted_pthread_functions); + libmcmini_init_done = true; } void mc_load_intercepted_pthread_functions(void) { @@ -68,7 +94,12 @@ void mc_load_intercepted_pthread_functions(void) { } libpthread_pthread_create_ptr = dlsym(libpthread_handle, "pthread_create"); + // See tsan_or_real_pthread_create()'s header comment for why this must be + // RTLD_NEXT, not the libpthread_handle dlsym above. + tsan_or_real_pthread_create_ptr = dlsym(RTLD_NEXT, "pthread_create"); libpthread_pthread_join_ptr = dlsym(libpthread_handle, "pthread_join"); + libpthread_timedjoin_np_ptr = dlsym(libpthread_handle, "pthread_timedjoin_np"); + libpthread_pthread_exit_ptr = dlsym(libpthread_handle, "pthread_exit"); pthread_mutex_init_ptr = dlsym(libpthread_handle, "pthread_mutex_init"); pthread_mutex_lock_ptr = dlsym(libpthread_handle, "pthread_mutex_lock"); pthread_mutex_trylock_ptr = dlsym(libpthread_handle, "pthread_mutex_trylock"); @@ -91,6 +122,7 @@ void mc_load_intercepted_pthread_functions(void) { exit_ptr = dlsym(libc_handle, "exit"); abort_ptr = dlsym(libc_handle, "abort"); fork_ptr = dlsym(libc_handle, "fork"); + clone_ptr = dlsym(libc_handle, "__clone"); dlclose(libpthread_handle); dlclose(libc_handle); @@ -221,6 +253,12 @@ int libdmtcp_pthread_create(pthread_t *thread, const pthread_attr_t *attr, return (*libdmtcp_pthread_create_ptr)(thread, attr, routine, arg); } +int tsan_or_real_pthread_create(pthread_t *thread, const pthread_attr_t *attr, + void *(*routine)(void *), void *arg) { + libmcmini_init(); + return (*tsan_or_real_pthread_create_ptr)(thread, attr, routine, arg); +} + int pthread_join(pthread_t thread, void **rv) { return mc_pthread_join(thread, rv); } @@ -228,11 +266,24 @@ int libpthread_pthread_join(pthread_t thread, void **rv) { libmcmini_init(); return (*libpthread_pthread_join_ptr)(thread, rv); } +int libpthread_timedjoin_np(pthread_t thread, void **rv, + const struct timespec *abstime) { + libmcmini_init(); + return (*libpthread_timedjoin_np_ptr)(thread, rv, abstime); +} int libdmtcp_pthread_join(pthread_t thread, void **rv) { libmcmini_init(); return (*libdmtcp_pthread_join_ptr)(thread, rv); } +MCMINI_NO_RETURN void pthread_exit(void *retval) { + mc_pthread_exit(retval); +} +MCMINI_NO_RETURN void libpthread_pthread_exit(void *retval) { + libmcmini_init(); + (*libpthread_pthread_exit_ptr)(retval); +} + void exit(int status) { mc_transparent_exit(status); } @@ -263,6 +314,11 @@ pid_t libc_fork(void) { libmcmini_init(); return (*fork_ptr)(); } +int libc_clone(int (*fn)(void *), void *child_stack, int flags, void *arg, + void *ptid, void *newtls, void *ctid) { + libmcmini_init(); + return (*clone_ptr)(fn, child_stack, flags, arg, ptid, newtls, ctid); +} int sem_init(sem_t*sem, int p, unsigned count) { return mc_sem_init(sem, p, count); @@ -301,3 +357,44 @@ int libpthread_sem_wait_loop(sem_t *sem) { rc = libpthread_sem_wait(sem); return rc; } + +// __libc_start_main() is called from _start (crt1.o), not glibc-internal +// code, so unlike exit()/_exit() -- whose implicit post-main() call chain +// resolves through glibc-internal hidden aliases that bypass any public- +// symbol interposition (see test/tsan_support/test_implicit_exit_interposition.c) +// -- this call site goes through ordinary dynamic symbol resolution and can +// be intercepted like any other public symbol. Used to guarantee +// mc_transparent_exit()'s exit-transition/restart-quiescence handling runs +// after the target's main() returns, even when it returns plainly +// (`return N;`) rather than calling exit()/pthread_exit() explicitly -- +// verified in test/tsan_support/test_libc_start_main_hook.c. +typedef int (*main_fn)(int, char **, char **); +typedef int (*libc_start_main_fn)(main_fn, int, char **, void (*)(void), + void (*)(void), void (*)(void), void *); + +static main_fn real_main; + +static int wrapped_main(int argc, char **argv, char **envp) { + int rc = real_main(argc, argv, envp); + // libmcmini_init() itself is safe to call any time after main() has run + // (pthreads are certainly initialized by now), unlike at the top of + // __libc_start_main() below, which runs before glibc's own internal + // pthread-subsystem setup and must stay free of any pthread_once/mutex + // use until the real __libc_start_main() has had a chance to run. + libmcmini_init(); + // Route a plain return from main() through the exact same model-checking + // exit protocol as an explicit exit(rc) call: mc_transparent_exit() + // itself performs the real, final process termination in every mode + // (see wrappers.c), so this call never returns. + mc_transparent_exit(rc); +} + +int __libc_start_main(main_fn main, int argc, char **argv, void (*init)(void), + void (*fini)(void), void (*rtld_fini)(void), + void *stack_end) { + real_main = main; + libc_start_main_fn real_start_main = + (libc_start_main_fn)dlsym(RTLD_NEXT, "__libc_start_main"); + return real_start_main(wrapped_main, argc, argv, init, fini, rtld_fini, + stack_end); +} diff --git a/src/lib/log.c b/src/lib/log.c index 3cc30140..552b07b2 100644 --- a/src/lib/log.c +++ b/src/lib/log.c @@ -4,6 +4,7 @@ #include #include #include +#include #include "dmtcp.h" static int global_log_level = MCMINI_LOG_MINIMUM_LEVEL; @@ -11,6 +12,29 @@ static const char *log_level_strs[] = { "VERBOSE", "DEBUG", "INFO", "WARN", "ERROR", "FATAL", "DISABLE" }; +// mcmini_log() is called concurrently, unsynchronized, by any thread. Its +// call to localtime() below can trigger glibc's lazy timezone-database +// initialization (tzset_internal), which is not safe to race across +// threads on its first call. Constructors run once, before main(), with +// only a single thread, so calling tzset() here forces that lazy init to +// happen safely ahead of any concurrent mcmini_log() call. +__attribute__((constructor)) static void mcmini_log_init_tz(void) { + tzset(); +} + +// TSan's public annotation API (see ), used +// below to tell TSan about log_mut's happens-before edge. libmcmini.so +// itself is normally uninstrumented, and log_mut is locked/unlocked via +// libpthread_mutex_lock/unlock, which resolve straight to libpthread's own +// symbols (bypassing TSan's interceptor entirely -- see +// mc_load_intercepted_pthread_functions() in interception.c) -- so without +// this, a real, correctly-held lock is still invisible to TSan, and it +// reports a false race on glibc's internal tzset_internal() state instead. +// Declared weak (same idiom as dmtcp_mcmini_plugin_is_loaded() in main.c), +// so this is a no-op when the process has no TSan runtime loaded at all. +extern void __tsan_acquire(void *addr) __attribute__((weak)); +extern void __tsan_release(void *addr) __attribute__((weak)); + typedef struct log_record { int level; int line; @@ -32,10 +56,42 @@ void mcmini_log_toggle(bool enable) { mcmini_log_set_level(MCMINI_LOG_DISABLE); } +// glibc's tzset_internal() (invoked by localtime_r() below on every call, +// not just the first) is not safe to run concurrently from multiple threads +// even when TZ never changes -- it races on its own internal TZ database +// cache. mcmini_log() is called unsynchronized from any thread, so +// serialize the whole body with this lock. +// +// multithreaded_fork()'s process-duplicating step is a raw _Fork(), chosen +// specifically to skip the work a real fork() would do -- including running +// any registered pthread_atfork() handlers. That means this lock gets no +// fork-safety net: if some other thread (most plausibly the template +// thread, which logs constantly) holds it at the exact instant _Fork() +// snapshots memory for a new branch, every thread in that branch that later +// calls mcmini_log() deadlocks on it forever -- the true owner's +// continuation isn't part of this child's process at all, so nothing can +// ever unlock it. See doc/log-mutex-fork-desync.txt. +static pthread_mutex_t log_mut = PTHREAD_MUTEX_INITIALIZER; + +void mcmini_log_reset_after_fork(void) { + // Must use the bypass handle, not the plain pthread_mutex_init(): that + // symbol is libmcmini's own interposed mc_pthread_mutex_init(), which + // dispatches on get_current_mode() and (in TARGET_BRANCH_AFTER_RESTART/ + // DMTCP_RESTART_INTO_BRANCH modes) asserts this thread already has a + // valid tid_self via thread_get_mailbox() -- not yet true this early, + // right after _Fork(), before restart_child_threads_fast() runs. log_mut + // is a purely internal implementation detail, never meant to be + // model-checked in the first place. + libpthread_mutex_init(&log_mut, NULL); +} + void mcmini_log(int level, const char *file, int line, const char *fmt, ...) { if (level < global_log_level) { return; } + libpthread_mutex_lock(&log_mut); + if (__tsan_acquire) __tsan_acquire(&log_mut); + log_record rc; time_t t = time(NULL); rc.format = fmt; @@ -43,7 +99,9 @@ void mcmini_log(int level, const char *file, int line, const char *fmt, ...) { rc.line = line; rc.level = level; rc.target = stdout; - rc.time = localtime(&t); + struct tm tm_buf; + localtime_r(&t, &tm_buf); + rc.time = &tm_buf; va_start(rc.var_args, fmt); char buf[20]; buf[strftime(buf, sizeof(buf), "%H:%M:%S", rc.time)] = '\0'; @@ -57,4 +115,7 @@ void mcmini_log(int level, const char *file, int line, const char *fmt, ...) { fprintf(rc.target, "\n"); fflush(rc.target); va_end(rc.var_args); + + if (__tsan_release) __tsan_release(&log_mut); + libpthread_mutex_unlock(&log_mut); } diff --git a/src/lib/main.c b/src/lib/main.c index 3909c0c8..1c785ff5 100644 --- a/src/lib/main.c +++ b/src/lib/main.c @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -19,6 +20,25 @@ volatile void *global_shm_start = NULL; +// Weak: __tsan_acquire resolves to non-NULL only when libtsan.so is loaded +// in this process (i.e. the target is TSan-instrumented); __wrap_pthread_join +// resolves to non-NULL only if the target itself was linked with +// -Wl,--wrap=pthread_join (see src/lib/pthread_join_wrap.c). Without that +// flag, a DMTCP-resurrected thread's pthread_join() can hang forever under +// TSan -- see TSAN-pthread-join.md -- so warn as early as possible if a +// TSan target is missing it. +extern void __tsan_acquire(void *addr) __attribute__((weak)); +extern int __wrap_pthread_join(pthread_t thread, void **retval) + __attribute__((weak)); + +__attribute__((constructor)) static void warn_if_tsan_target_missing_wrap(void) { + if (__tsan_acquire != NULL && __wrap_pthread_join == NULL) { + fprintf(stderr, + "WARNING: TSAN target not compiled with --wrap. Potential hang " + "during pthread_join\n"); + } +} + void mc_allocate_shared_memory_region(const char *shm_name) { int fd = shm_open(shm_name, O_RDWR, S_IRUSR | S_IWUSR); if (fd == -1) { diff --git a/src/lib/pthread_cond_signal_wrap.c b/src/lib/pthread_cond_signal_wrap.c new file mode 100644 index 00000000..b22442c4 --- /dev/null +++ b/src/lib/pthread_cond_signal_wrap.c @@ -0,0 +1,19 @@ +// Like pthread_cond_wait_wrap.c: must be compiled into the TARGET binary +// itself (not libmcmini.so), linked with `-Wl,--wrap=pthread_cond_signal`. +// +// TSan's pthread_cond_signal interceptor has the same interposition-chain +// bypass as pthread_cond_wait's -- confirmed live, contrary to +// doc/cond-wait-tsan-interceptor-bypass.txt's original assumption that +// signal/broadcast delegate normally. Without this wrap, +// mc_pthread_cond_signal() is never reached under DMTCP. +// +// No __real_pthread_cond_signal() fallback needed: mc_pthread_cond_signal() +// already handles every libmcmini_mode itself. + +#include + +extern int mc_pthread_cond_signal(pthread_cond_t *cond); + +int __wrap_pthread_cond_signal(pthread_cond_t *cond) { + return mc_pthread_cond_signal(cond); +} diff --git a/src/lib/pthread_cond_wait_wrap.c b/src/lib/pthread_cond_wait_wrap.c new file mode 100644 index 00000000..fd56ae4a --- /dev/null +++ b/src/lib/pthread_cond_wait_wrap.c @@ -0,0 +1,20 @@ +// Like pthread_join_wrap.c: must be compiled into the TARGET binary itself +// (not libmcmini.so), linked with `-Wl,--wrap=pthread_cond_wait`. +// +// Unlike pthread_join/mutex_lock/sem_wait, TSan's pthread_cond_wait +// interceptor never delegates to the next implementation in the +// interposition chain -- it implements the wait atomically inside its own +// runtime. So under DMTCP, mc_pthread_cond_wait() is never reached at all +// without this wrap. See doc/cond-wait-tsan-interceptor-bypass.txt. +// +// No __real_pthread_cond_wait() fallback needed here (unlike +// __wrap_pthread_join()): mc_pthread_cond_wait() already handles every +// libmcmini_mode itself. + +#include + +extern int mc_pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mutex); + +int __wrap_pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mutex) { + return mc_pthread_cond_wait(cond, mutex); +} diff --git a/src/lib/pthread_join_wrap.c b/src/lib/pthread_join_wrap.c new file mode 100644 index 00000000..99e0aee5 --- /dev/null +++ b/src/lib/pthread_join_wrap.c @@ -0,0 +1,38 @@ +// NOTE: unlike every other file in src/lib/, this one must NOT be compiled +// into libmcmini.so. It must be compiled directly into the TARGET program's +// own binary, linked alongside the flag `-Wl,--wrap=pthread_join`. +// +// Why: under DMTCP, libtsan.so is loaded ahead of libmcmini.so, so a +// target's own calls to the public pthread_join() symbol reach TSan's +// interceptor first, before libmcmini's. For a thread DMTCP resurrected via +// clone() (bypassing TSan's own pthread_create() interceptor), TSan's +// interceptor can never resolve that thread's Tid and hangs forever -- see +// TSAN-pthread-join.md. `-Wl,--wrap=pthread_join`, applied when linking the +// target itself, sidesteps this entirely: it rewrites the target's own +// pthread_join() calls, at link time, to call __wrap_pthread_join() below +// instead -- before the dynamic linker (and hence TSan's interceptor) is +// even involved. When a real join is safe, this calls __real_pthread_join(), +// which the linker's --wrap rewriting resolves to the original pthread_join +// symbol -- so TSan still gets to see it and do its own happens-before +// bookkeeping for that call, same as it would without any of this. +// +// __real_pthread_join is not a real, independently-resolvable symbol: it +// only exists as a link-time rewrite within this same --wrap=pthread_join +// link, which is why this must be compiled directly into the target and +// cannot live inside libmcmini.so (a separately-built shared library). + +#include +#include + +extern int __real_pthread_join(pthread_t thread, void **retval); +extern int mc_pthread_join_maybe_defer(pthread_t thread, void **retval, + bool *deferred); + +int __wrap_pthread_join(pthread_t thread, void **retval) { + bool deferred; + int result = mc_pthread_join_maybe_defer(thread, retval, &deferred); + if (deferred) { + return __real_pthread_join(thread, retval); + } + return result; +} diff --git a/src/lib/record.c b/src/lib/record.c index 2023f7cc..a468aa96 100644 --- a/src/lib/record.c +++ b/src/lib/record.c @@ -1,7 +1,9 @@ +#include "mcmini/mem.h" #include "mcmini/spy/checkpointing/record.h" #include "mcmini/spy/checkpointing/rec_list.h" #include "mcmini/spy/checkpointing/objects.h" #include "mcmini/spy/checkpointing/transitions.h" +#include "mcmini/spy/checkpointing/tsan_support.h" #include "mcmini/spy/intercept/interception.h" #include @@ -78,6 +80,24 @@ rec_list *add_rec_entry_record_mode(const visible_object *vo) { return new_node; } +// TSan-safe variant: identical to add_rec_entry_record_mode but backed by +// mc_ts_alloc (no malloc), for use before libtsan has registered the calling +// thread. `new_node->vo = *vo` compiles to inline stores (verified: no memcpy +// libcall), so this whole function stays free of TSan interceptors. +rec_list *add_rec_entry_record_mode_ts(const visible_object *vo) { + rec_list *new_node = (rec_list *)mc_ts_alloc(sizeof(rec_list)); + new_node->vo = *vo; + new_node->next = NULL; + if (head_record_mode == NULL) { + head_record_mode = new_node; + current_record_mode = new_node; + } else { + current_record_mode->next = new_node; + current_record_mode = new_node; + } + return new_node; +} + //debugging puropses, will remove later // void print_rec_list(const rec_list *head) { // const rec_list *current = head; @@ -152,16 +172,48 @@ bool is_in_restart_mode(void) { } enum libmcmini_mode get_current_mode() { + resolve_ckpt_window_candidate_if_pending(); + // INVARIANT (imposed by the code in `mc_pthread_create()`): // The checkpoint thread is guaranteed to reach the line // "is_checkpoint_thread()", because the only time in which the atomic is // stored is BEFORE the checkpoint thread has executed DMTCP code. if (atomic_load(&libmcmini_has_recorded_checkpoint_thread)) { if (is_checkpoint_thread()) { - return CHECKPOINT_THREAD; + return EXTERNAL_THREAD; } } - return atomic_load(&libmcmini_mode); + enum libmcmini_mode raw_mode = atomic_load(&libmcmini_mode); + // TEMPORARY DIAGNOSTIC FIX: mc_is_current_thread_tsan_internal() is only + // ever needed to distinguish TSan's internal thread in the four modes + // below (the same four every wrapper function's own switch groups + // together for this purpose) -- never during PRE_DMTCP_INIT, + // PRE_CHECKPOINT_THREAD, RECORD, or PRE_CHECKPOINT. Its first call per + // thread leaks an fd (see tsan_support.c), and calling it outside these + // four modes means that leaked fd is open at checkpoint time, so DMTCP + // must checkpoint/restore a /proc//task//status path whose + // pid/tid cannot exist after restart. Restricting the check to only these + // four modes empirically confirmed this fd is why template_thread() was + // failing to wake up after restart (it now does). Still open: WHY does + // this cause template_thread() to hang, specifically -- i.e. is this + // DMTCP failing to restore the fd correctly, or something else entirely? + // Do not remove this gate until that is understood and a permanent fix + // (e.g. never opening the fd in the first place) replaces it. + bool tsan_internal_check_applies = + raw_mode == TARGET_BRANCH || raw_mode == TARGET_BRANCH_AFTER_RESTART || + raw_mode == DMTCP_RESTART_INTO_BRANCH || + raw_mode == DMTCP_RESTART_INTO_TEMPLATE; + if (!tsan_internal_check_applies) { + return raw_mode; + } + // ThreadSanitizer's own internal background thread (present only when the + // target is TSan-instrumented) can likewise call into libmcmini.so's + // overridden functions for its own purposes, on a thread that is not part + // of the target program. See EXTERNAL_THREAD's definition in record.h. + if (mc_is_current_thread_tsan_internal()) { + return EXTERNAL_THREAD; + } + return raw_mode; } void set_current_mode(enum libmcmini_mode new_mode) { atomic_store(&libmcmini_mode, new_mode); diff --git a/src/lib/sem-wrappers.c b/src/lib/sem-wrappers.c index 6ac27820..2318de9a 100644 --- a/src/lib/sem-wrappers.c +++ b/src/lib/sem-wrappers.c @@ -13,7 +13,7 @@ int mc_sem_init(sem_t *sem, int p, unsigned count) { switch (get_current_mode()) { case PRE_DMTCP_INIT: case PRE_CHECKPOINT_THREAD: - case CHECKPOINT_THREAD: { + case EXTERNAL_THREAD: { return libpthread_sem_init(sem, p, count); } case RECORD: @@ -69,7 +69,7 @@ int mc_sem_destroy(sem_t *sem) { switch (get_current_mode()) { case PRE_DMTCP_INIT: case PRE_CHECKPOINT_THREAD: - case CHECKPOINT_THREAD: { + case EXTERNAL_THREAD: { return libpthread_sem_destroy(sem); } case RECORD: @@ -125,7 +125,7 @@ mc_sem_post(sem_t *sem) { switch (get_current_mode()) { case PRE_DMTCP_INIT: case PRE_CHECKPOINT_THREAD: - case CHECKPOINT_THREAD: { + case EXTERNAL_THREAD: { return libpthread_sem_post(sem); } case RECORD: @@ -178,7 +178,7 @@ int mc_sem_wait(sem_t *sem) { switch (get_current_mode()) { case PRE_DMTCP_INIT: case PRE_CHECKPOINT_THREAD: - case CHECKPOINT_THREAD: { + case EXTERNAL_THREAD: { return libpthread_sem_wait(sem); } case RECORD: diff --git a/src/lib/tsan_support.c b/src/lib/tsan_support.c new file mode 100644 index 00000000..481fe2fb --- /dev/null +++ b/src/lib/tsan_support.c @@ -0,0 +1,116 @@ +#include "mcmini/spy/checkpointing/tsan_support.h" + +#include +#include +#include +#include +#include +#include + +#include "mcmini/defines.h" + +// Matches dmtcp-callback.c's / multithreaded_fork.c's own definition. Not +// shared via a common header since, today, this is the only other file that +// needs it; worth hoisting to a shared header if a fourth user shows up. +#define SIG_MULTITHREADED_FORK (SIGRTMIN + 6) + +// Both functions below may run on a thread ThreadSanitizer has not yet +// registered (i.e. before that OS thread's TSan trampoline has executed) -- +// specifically, mc_is_current_thread_tsan_internal() is called from +// get_current_mode(), which runs on every wrapped call, including a brand +// new worker thread's very first one. Empirically isolated by bisecting this +// file's original fopen/fgets/sscanf/fclose-based implementation one call at +// a time: raw syscalls for openat/read/readlink are safe here, but even a +// raw syscall(SYS_close, fd) crashes (null ThreadState dereference inside +// libtsan), so the one fd this code opens is deliberately never closed (see +// below). Buffered stdio and sscanf/snprintf were replaced with hand-rolled +// equivalents in the course of this isolation, not because they were +// individually confirmed unsafe. + +int thread_blocks_signal(pid_t tid, int signo) { + (void)signo; + char path[64]; + { + const char *prefix = "/proc/self/task/"; + const char *suffix = "/status"; + size_t i = 0; + for (const char *p = prefix; *p; p++) path[i++] = *p; + char digits[16]; + int nd = 0; + unsigned int v = (tid < 0) ? 0 : (unsigned int)tid; + if (v == 0) digits[nd++] = '0'; + while (v > 0) { digits[nd++] = (char)('0' + (v % 10)); v /= 10; } + while (nd > 0) path[i++] = digits[--nd]; + for (const char *p = suffix; *p; p++) path[i++] = *p; + path[i] = '\0'; + } + + int fd = syscall(SYS_openat, AT_FDCWD, path, O_RDONLY); + if (fd < 0) { + return 0; + } + + char buf[4096]; + ssize_t total = 0; + ssize_t n; + while (total < (ssize_t)sizeof(buf) - 1 && + (n = syscall(SYS_read, fd, buf + total, sizeof(buf) - 1 - total)) > 0) { + total += n; + } + // Deliberately not closed: empirically, even a raw syscall(SYS_close, fd) + // crashes here (unlike openat/read/readlink), when called -- as this + // function can be -- from a thread ThreadSanitizer has not yet + // registered. Leaking this one fd is an acceptable, bounded cost: this + // path only ever runs once per thread for the process's entire lifetime + // (see mc_is_current_thread_tsan_internal()'s caching). + buf[total] = '\0'; + + const char *sigblk_line = strstr(buf, "SigBlk:"); + if (sigblk_line == NULL) { + return 0; + } + const char *p = sigblk_line + strlen("SigBlk:"); + while (*p == ' ' || *p == '\t') p++; + unsigned long long sigblk = 0; + int any_digit = 0; + while (*p) { + int d; + if (*p >= '0' && *p <= '9') d = *p - '0'; + else if (*p >= 'a' && *p <= 'f') d = *p - 'a' + 10; + else if (*p >= 'A' && *p <= 'F') d = *p - 'A' + 10; + else break; + sigblk = (sigblk << 4) | (unsigned long long)d; + any_digit = 1; + p++; + } + if (!any_digit) { + return 0; + } + return (int)((sigblk >> (signo - 1)) & 1ULL); +} + +pid_t mc_real_tid(void) { + char linkbuf[64]; + ssize_t n = syscall(SYS_readlink, "/proc/thread-self", linkbuf, + sizeof(linkbuf) - 1); + if (n < 0) { + return -1; + } + linkbuf[n] = '\0'; + const char *task = strstr(linkbuf, "/task/"); + if (task == NULL) { + return -1; + } + return (pid_t)atoi(task + strlen("/task/")); +} + +bool mc_is_current_thread_tsan_internal(void) { + // -1: not yet computed; 0: no; 1: yes. Cached per-thread since the + // property being tested never changes over a thread's lifetime, and + // thread_blocks_signal() is not cheap enough to call on every wrapped op. + static MCMINI_THREAD_LOCAL int cached = -1; + if (cached == -1) { + cached = thread_blocks_signal(mc_real_tid(), SIG_MULTITHREADED_FORK) ? 1 : 0; + } + return cached == 1; +} diff --git a/src/lib/wrappers.c b/src/lib/wrappers.c index a385c012..76861b89 100644 --- a/src/lib/wrappers.c +++ b/src/lib/wrappers.c @@ -3,11 +3,13 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include @@ -15,41 +17,132 @@ #include "mcmini/Thread_queue.h" #include "mcmini/mcmini.h" +// TSan's public annotation API (see ), used +// below to tell TSan about the application mutex's happens-before edge -- +// same idiom already used for log_mut in log.c (see that file's comment for +// the full rationale). mc_pthread_mutex_lock()/unlock()/mc_pthread_cond_wait() +// all really lock/unlock the application's own pthread_mutex_t via +// libpthread_mutex_lock/unlock/timedlock, which resolve straight to +// libpthread's own symbols (bypassing TSan's interceptor entirely -- see +// mc_load_intercepted_pthread_functions() in interception.c). Without these +// annotations a real, correctly-held lock is invisible to TSan, so it can +// never establish a happens-before edge between critical sections in +// different threads -- reporting a false race on every application variable +// the mutex protects (e.g. cv-producer-consumer's count/buffer), even though +// mcmini's own scheduler serializes access to them correctly. +extern void __tsan_acquire(void *addr) __attribute__((weak)); +extern void __tsan_release(void *addr) __attribute__((weak)); + +// TSan's Fiber API (weak: no-ops for non-TSan targets), used by +// mc_pthread_exit() to give a DMTCP-resurrected thread a fresh, empty TSan +// shadow call stack before terminating it via a raw exit syscall -- see +// that function and mc_pthread_is_recreated_thread()'s doc comment in +// wrappers.h. Mirrors the same idiom already used in dmtcp-callback.c for +// resurrected/forking threads' first resumption. +extern void *__tsan_create_fiber(unsigned flags) __attribute__((weak)); +extern void __tsan_switch_to_fiber(void *fiber, unsigned flags) + __attribute__((weak)); + typedef struct pthread_map { pthread_t thread; runner_id_t value; + // Posted by whichever thread eventually calls pthread_join() on this + // one (mc_pthread_join()'s TARGET_BRANCH*/DMTCP_RESTART_INTO_BRANCH-ish + // cases), waited on by this thread itself before it may really + // terminate (mc_exit_thread_in_child()). See the two functions for the + // full rationale. + sem_t exit_permission_sem; + // Posted by this thread right after exit_permission_sem, immediately + // before it actually finishes. mc_pthread_join() waits on this instead + // of performing a real join for threads a real join could never + // resolve for anyway (see registered_post_restart below). Posting it + // unconditionally is harmless on paths where nothing waits on it. + sem_t really_exited_sem; + // Whether this thread's own (one-time) registration happened while + // get_current_mode() was already TARGET_BRANCH_AFTER_RESTART -- i.e. + // whether it was created via a real pthread_create() call made after a + // restart, as opposed to being one of the threads DMTCP resurrected via + // clone() (which never re-registers: such a thread resumes mid- + // execution via setcontext(), so its only-ever registration is from the + // original, pre-restart RECORD-mode run). A clone()-recreated thread + // never went through TSan's own pthread_create() interceptor, so TSan + // has no Tid for it and a real pthread_join() on it can never resolve + // (confirmed against TSan's own source -- see TSAN-pthread-join.md). + // mc_pthread_join() uses this flag to decide whether a real join is + // even possible for a given target thread. + bool registered_post_restart; struct pthread_map *next; } pthread_map_t; -static pthread_rwlock_t pthread_map_lock = PTHREAD_RWLOCK_INITIALIZER; +// NOTE: This lock and allocator must be TSan-safe: insert_pthread_map runs from +// mc_thread_routine_wrapper's prologue, which under DMTCP executes before +// libtsan has registered the thread. So, we use libmcmini's libpthread_* handle +// wrappers (which bypass libtsan) rather than the raw pthread_rwlock_* symbols +// (which resolve to libtsan's interceptors), and mc_ts_alloc rather than malloc. +// See TSAN-McMini-DMTCP.txt. +static pthread_mutex_t pthread_map_lock = PTHREAD_MUTEX_INITIALIZER; static pthread_map_t *head = NULL; void insert_pthread_map(pthread_t t, runner_id_t v) { - pthread_rwlock_wrlock(&pthread_map_lock); - pthread_map_t *n = malloc(sizeof *n); + libpthread_mutex_lock(&pthread_map_lock); + pthread_map_t *n = mc_ts_alloc(sizeof *n); n->thread = t; n->value = v; + libpthread_sem_init(&n->exit_permission_sem, 0, 0); + libpthread_sem_init(&n->really_exited_sem, 0, 0); + n->registered_post_restart = (get_current_mode() == TARGET_BRANCH_AFTER_RESTART); n->next = head; head = n; - pthread_rwlock_unlock(&pthread_map_lock); + libpthread_mutex_unlock(&pthread_map_lock); } -runner_id_t search_pthread_map(pthread_t t) { - pthread_rwlock_rdlock(&pthread_map_lock); - pthread_map_t *cur = head; - while (cur) { +// Returns the given thread's own pthread_map_t entry, or NULL if the thread +// is not (yet) registered. The returned pointer stays valid to use after +// unlocking: entries are only ever appended, never removed or reallocated. +static pthread_map_t *find_pthread_map_entry(pthread_t t) { + libpthread_mutex_lock(&pthread_map_lock); + pthread_map_t *result = NULL; + for (pthread_map_t *cur = head; cur != NULL; cur = cur->next) { if (pthread_equal(cur->thread, t)) { - return cur->value; + result = cur; + break; } - cur = cur->next; } - pthread_rwlock_unlock(&pthread_map_lock); - return RID_INVALID; + libpthread_mutex_unlock(&pthread_map_lock); + return result; +} + +runner_id_t search_pthread_map(pthread_t t) { + pthread_map_t *entry = find_pthread_map_entry(t); + return entry == NULL ? RID_INVALID : entry->value; +} + +// Returns the given thread's own exit_permission_sem (see pthread_map_t), +// or NULL if the thread is not (yet) registered. +sem_t *find_exit_permission_sem(pthread_t t) { + pthread_map_t *entry = find_pthread_map_entry(t); + return entry == NULL ? NULL : &entry->exit_permission_sem; +} + +// Returns the given thread's own really_exited_sem (see pthread_map_t), or +// NULL if the thread is not (yet) registered. +sem_t *find_really_exited_sem(pthread_t t) { + pthread_map_t *entry = find_pthread_map_entry(t); + return entry == NULL ? NULL : &entry->really_exited_sem; } MCMINI_THREAD_LOCAL runner_id_t tid_self = RID_INVALID; +// Set (on the creating thread) while libmcmini creates one of its OWN helper +// threads (e.g. the template thread) via the public pthread_create. It tells +// mc_pthread_create to create the thread plainly -- routed through DMTCP and +// visible to any sanitizer's pthread_create interceptor -- rather than treating +// it as a user thread to be model-checked. Keeping it visible to +// ThreadSanitizer is what lets its ThreadState round-trip checkpoint/restart. +// See TSAN-McMini-DMTCP.txt. +MCMINI_THREAD_LOCAL int mc_creating_internal_thread = 0; + runner_id_t mc_register_this_thread(void) { static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER; static runner_id_t tid_next = 0; @@ -122,7 +215,7 @@ int mc_pthread_mutex_init(pthread_mutex_t *mutex, switch (get_current_mode()) { case PRE_DMTCP_INIT: case PRE_CHECKPOINT_THREAD: - case CHECKPOINT_THREAD: { + case EXTERNAL_THREAD: { return libpthread_mutex_init(mutex, attr); } case RECORD: @@ -136,7 +229,8 @@ int mc_pthread_mutex_init(pthread_mutex_t *mutex, // FIXME: We assume that this is a normal mutex. For other mutex // types, we'd need to behave differently visible_object vo = { - .type = MUTEX, .location = mutex, .mut_state = UNINITIALIZED}; + .type = MUTEX, .location = mutex, + .mut_state = {.status = UNINITIALIZED, .owner = RID_INVALID}}; mutex_record = add_rec_entry_record_mode(&vo); } libpthread_mutex_unlock(&rec_list_lock); @@ -144,7 +238,7 @@ int mc_pthread_mutex_init(pthread_mutex_t *mutex, int rc = libpthread_mutex_init(mutex, attr); if (rc == 0) { // Init libpthread_mutex_lock(&rec_list_lock); - mutex_record->vo.mut_state = UNLOCKED; + mutex_record->vo.mut_state.status = UNLOCKED; libpthread_mutex_unlock(&rec_list_lock); } return rc; @@ -201,8 +295,10 @@ int mc_pthread_mutex_lock(pthread_mutex_t *mutex) { switch (get_current_mode()) { case PRE_DMTCP_INIT: case PRE_CHECKPOINT_THREAD: - case CHECKPOINT_THREAD: { - return libpthread_mutex_lock(mutex); + case EXTERNAL_THREAD: { + int rc = libpthread_mutex_lock(mutex); + if (rc == 0 && __tsan_acquire) __tsan_acquire(mutex); + return rc; } case RECORD: case PRE_CHECKPOINT: { @@ -213,18 +309,33 @@ int mc_pthread_mutex_lock(pthread_mutex_t *mutex) { rec_list *mutex_record = find_object_record_mode(mutex); if (mutex_record == NULL) { visible_object vo = { - .type = MUTEX, .location = mutex, .mut_state = UNINITIALIZED}; + .type = MUTEX, .location = mutex, + .mut_state = {.status = UNINITIALIZED, .owner = RID_INVALID}}; mutex_record = add_rec_entry_record_mode(&vo); } libpthread_mutex_unlock(&rec_list_lock); - struct timespec time = {.tv_sec = 2}; while (1) { + // Recompute the deadline every iteration -- see mc_sem_wait()'s + // identical pattern in sem-wrappers.c. A hoisted, one-time-computed + // {.tv_sec = 2} here would be interpreted as an absolute deadline of + // 2 seconds past the epoch, already long past, making every + // subsequent call return ETIMEDOUT immediately instead of ever + // genuinely blocking. + struct timespec time; + clock_gettime(CLOCK_REALTIME, &time); + time.tv_sec += 2; int rc = libpthread_mutex_timedlock(mutex, &time); if (rc == 0) { // Lock succeeded libpthread_mutex_lock(&rec_list_lock); - mutex_record->vo.mut_state = LOCKED; + // Record the owner too: a checkpoint can land while this thread is + // mid pthread_cond_wait(), still modeled as the mutex's holder + // until the enqueue transition runs (condition_variable_enqueue_ + // thread::modify() requires mutex->is_locked_by(executor)). + mutex_record->vo.mut_state.status = LOCKED; + mutex_record->vo.mut_state.owner = tid_self; libpthread_mutex_unlock(&rec_list_lock); + if (__tsan_acquire) __tsan_acquire(mutex); return rc; } else if (rc == ETIMEDOUT) { // If the lock failed. // Here, the user-space thread did not manage to acquire @@ -254,7 +365,9 @@ int mc_pthread_mutex_lock(pthread_mutex_t *mutex) { mb->type = MUTEX_LOCK_TYPE; memcpy_v(mb->cnts, &mutex, sizeof(mutex)); thread_handle_after_dmtcp_restart(); - return libpthread_mutex_lock(mutex); + int rc = libpthread_mutex_lock(mutex); + if (rc == 0 && __tsan_acquire) __tsan_acquire(mutex); + return rc; } case TARGET_BRANCH: case TARGET_BRANCH_AFTER_RESTART: { @@ -262,7 +375,9 @@ int mc_pthread_mutex_lock(pthread_mutex_t *mutex) { mb->type = MUTEX_LOCK_TYPE; memcpy_v(mb->cnts, &mutex, sizeof(mutex)); thread_wake_scheduler_and_wait(); - return libpthread_mutex_lock(mutex); + int rc = libpthread_mutex_lock(mutex); + if (rc == 0 && __tsan_acquire) __tsan_acquire(mutex); + return rc; } default: { // Wrapper functions should not be executing @@ -279,7 +394,8 @@ int mc_pthread_mutex_unlock(pthread_mutex_t *mutex) { switch (get_current_mode()) { case PRE_DMTCP_INIT: case PRE_CHECKPOINT_THREAD: - case CHECKPOINT_THREAD: { + case EXTERNAL_THREAD: { + if (__tsan_release) __tsan_release(mutex); return libpthread_mutex_unlock(mutex); } case RECORD: @@ -296,10 +412,12 @@ int mc_pthread_mutex_unlock(pthread_mutex_t *mutex) { libc_abort(); } libpthread_mutex_unlock(&rec_list_lock); + if (__tsan_release) __tsan_release(mutex); int rc = libpthread_mutex_unlock(mutex); if (rc == 0) { // Unlock succeeded libpthread_mutex_lock(&rec_list_lock); - mutex_record->vo.mut_state = UNLOCKED; + mutex_record->vo.mut_state.status = UNLOCKED; + mutex_record->vo.mut_state.owner = RID_INVALID; libpthread_mutex_unlock(&rec_list_lock); } return rc; @@ -310,6 +428,7 @@ int mc_pthread_mutex_unlock(pthread_mutex_t *mutex) { mb->type = MUTEX_UNLOCK_TYPE; memcpy_v(mb->cnts, &mutex, sizeof(mutex)); thread_handle_after_dmtcp_restart(); + if (__tsan_release) __tsan_release(mutex); return libpthread_mutex_unlock(mutex); } case TARGET_BRANCH: @@ -318,6 +437,7 @@ int mc_pthread_mutex_unlock(pthread_mutex_t *mutex) { mb->type = MUTEX_UNLOCK_TYPE; memcpy_v(mb->cnts, &mutex, sizeof(mutex)); thread_wake_scheduler_and_wait(); + if (__tsan_release) __tsan_release(mutex); return libpthread_mutex_unlock(mutex); } default: { @@ -335,7 +455,23 @@ void mc_exit_thread_in_child(void) { thread_get_mailbox()->type = THREAD_EXIT_TYPE; thread_wake_scheduler_and_wait(); thread_awake_scheduler_for_thread_finish_transition(); - thread_block_indefinitely(); + + // Wait for whichever thread eventually calls pthread_join() on this one + // (see mc_pthread_join()'s TARGET_BRANCH*/DMTCP_RESTART_INTO_BRANCH-ish + // cases) to grant permission before this thread is allowed to really + // terminate. This keeps this thread's pthread_t/tid valid for exactly as + // long as a real, unjoined POSIX thread's would be -- no longer -- + // rather than parking it forever regardless of whether anyone ever + // joins it. + sem_t *exit_permission = find_exit_permission_sem(pthread_self()); + assert(exit_permission != NULL); + libpthread_sem_wait(exit_permission); + + // Signal genuine completion, in case the joiner can't use a real join + // (see mc_pthread_join()'s TARGET_BRANCH_AFTER_RESTART case). + sem_t *really_exited = find_really_exited_sem(pthread_self()); + assert(really_exited != NULL); + libpthread_sem_post(really_exited); } void mc_exit_main_thread_in_child(void) { @@ -389,7 +525,14 @@ MCMINI_NO_RETURN void mc_transparent_exit(int status) { volatile runner_mailbox *mb = thread_get_mailbox(); mb->type = PROCESS_EXIT_TYPE; memcpy_v(mb->cnts, &status, sizeof(status)); - thread_await_scheduler(); + // Unlike every other wrapper's TARGET_BRANCH* case, this used to call + // the wait-only thread_await_scheduler(), which never posts + // model_side_sem. That's harmless for a thread that fell through from + // DMTCP_RESTART_INTO_BRANCH/TEMPLATE above (thread_handle_after_dmtcp_restart() + // already posted it), but a thread already in TARGET_BRANCH_AFTER_RESTART + // (e.g. main(), calling exit() for the first time since restart) jumps + // straight here and hangs the coordinator's execute_runner() forever. + thread_wake_scheduler_and_wait(); // After "exiting", don't actually exit yet: // the model checker will prevent the process @@ -397,7 +540,7 @@ MCMINI_NO_RETURN void mc_transparent_exit(int status) { // branch as "useless" since at this point mb->type = PROCESS_EXIT_TYPE; memcpy_v(mb->cnts, &status, sizeof(status)); - thread_await_scheduler(); + thread_wake_scheduler_and_wait(); } default: { libc_exit(status); @@ -450,6 +593,49 @@ MCMINI_NO_RETURN void mc_transparent_abort(void) { } } +MCMINI_NO_RETURN void mc_pthread_exit(void *retval) { + switch (get_current_mode()) { + case PRE_DMTCP_INIT: + case PRE_CHECKPOINT_THREAD: + case EXTERNAL_THREAD: + case RECORD: + case PRE_CHECKPOINT: { + libpthread_pthread_exit(retval); + } + case DMTCP_RESTART_INTO_BRANCH: + case DMTCP_RESTART_INTO_TEMPLATE: { + thread_get_mailbox()->type = THREAD_EXIT_TYPE; + thread_handle_after_dmtcp_restart(); + // Fallthrough + } + case TARGET_BRANCH: + case TARGET_BRANCH_AFTER_RESTART: { + if (tid_self == RID_MAIN_THREAD) { + mc_exit_main_thread_in_child(); // never returns + } else { + mc_exit_thread_in_child(); + } + // mc_exit_thread_in_child() only returns once safe to really + // terminate -- unlike returning from a start routine, pthread_exit() + // must never return to its caller, so do that for real here. A + // DMTCP-resurrected thread runs on a TSan fiber and crashes in + // glibc's real pthread_exit() (see doc/pthread-exit-abort-and-fiber- + // crash.txt); any other thread gets a real pthread_exit() as usual. + if (mc_pthread_is_recreated_thread(pthread_self())) { + if (__tsan_switch_to_fiber) { + __tsan_switch_to_fiber(__tsan_create_fiber(0), 0); + } + syscall(SYS_exit, 0); + libc_abort(); // unreachable + } + libpthread_pthread_exit(retval); + } + default: { + libc_abort(); + } + } +} + struct mc_thread_routine_arg { void *arg; thread_routine routine; @@ -462,7 +648,7 @@ void *mc_thread_routine_wrapper(void *arg) { switch (get_current_mode()) { case PRE_DMTCP_INIT: case PRE_CHECKPOINT_THREAD: - case CHECKPOINT_THREAD: { + case EXTERNAL_THREAD: { fprintf(stderr, "In `PRE_DMTCP_INIT` mode, `mc_pthread_create` always directly calls DMTCP." "Reaching this point would be an error.\n"); @@ -491,7 +677,9 @@ void *mc_thread_routine_wrapper(void *arg) { .thrd_state.pthread_desc = this_thread, .thrd_state.status = ALIVE, .thrd_state.id = rid}; - thread_record = add_rec_entry_record_mode(&vo); + // TSan-safe allocation: under DMTCP this runs before libtsan has + // registered the thread (see TSAN-McMini-DMTCP.txt). + thread_record = add_rec_entry_record_mode_ts(&vo); libpthread_mutex_unlock(&rec_list_lock); libpthread_sem_post(&unwrapped_arg->mc_pthread_create_binary_sem); break; @@ -575,10 +763,74 @@ void record_checkpoint_thread(void) { set_current_mode(RECORD); } +// Set (per-thread) by dmtcp_create_checkpoint_thread_wrapper() below, and +// resolved lazily by resolve_ckpt_window_candidate_if_pending() (called from +// get_current_mode()) on this thread's own first wrapped call. +static MCMINI_THREAD_LOCAL bool is_ckpt_window_candidate = false; +static MCMINI_THREAD_LOCAL int ckpt_window_candidate_virtual_tid; + +// Both the real checkpoint thread and TSAN's nested background-thread spawn +// (see dmtcp_create_checkpoint_thread_wrapper()'s comment) reach the same +// wrapper, and neither can tell, at creation time, which one it is. We can't +// resolve that here by calling dmtcp_tsan_background_thread_virtual_tid() +// and waiting: classification requires DMTCP's endCkptThreadCreationWindow(), +// which does not run until DMTCP's createCkptThread() sees its own +// pthread_create() call return -- which itself requires this thread to be +// registered with TSan, which happens only once this thread runs its real +// routine (below). Blocking here to wait for classification would therefore +// deadlock. +// +// So instead: mark this thread as a candidate and return immediately, +// letting it proceed into its real routine. Classification then happens +// lazily, on this same thread's first subsequent wrapped call (necessarily +// after TSan has registered it, so no longer deadlock-prone). +void mark_ckpt_window_candidate(int virtual_tid) { + ckpt_window_candidate_virtual_tid = virtual_tid; + is_ckpt_window_candidate = true; +} + +void resolve_ckpt_window_candidate_if_pending(void) { + if (!is_ckpt_window_candidate) return; + is_ckpt_window_candidate = false; + + int tsan_bg_virtual_tid = dmtcp_tsan_background_thread_virtual_tid(); + while (tsan_bg_virtual_tid == -1) { + sched_yield(); + tsan_bg_virtual_tid = dmtcp_tsan_background_thread_virtual_tid(); + } + if (tsan_bg_virtual_tid != ckpt_window_candidate_virtual_tid) { + record_checkpoint_thread(); + } + // else: this is TSAN's own background thread (tsan_bg_virtual_tid == + // ckpt_window_candidate_virtual_tid), not the checkpoint thread -- no + // checkpoint-thread bookkeeping needed. +} + void *dmtcp_create_checkpoint_thread_wrapper(void *arg) { - record_checkpoint_thread(); struct mc_thread_routine_arg *unwrapped_arg = arg; + + // Unblock mc_pthread_create()'s caller immediately; see + // mark_ckpt_window_candidate()'s comment for why classification can't + // happen here, before running the real routine. + // + // Trade-off: this leaves a window, from here until this thread's first + // subsequent wrapped call, during which ckpt_pthread_descriptor is not + // yet set (previously guaranteed set by the time createCkptThread() + // returned). + // + // This thread may be TSAN's own background thread, nested-spawned from + // inside TSAN's own pthread_create interceptor while it was still handling + // the real checkpoint thread's creation request, and such a thread never + // goes through TSAN's own thread registration -- so it has no TSAN + // ThreadState of its own. That's safe here: mc_pthread_create()'s + // PRE_CHECKPOINT_THREAD case already forced libmcmini_init() to complete + // on the creating thread before spawning this one, so libmcmini_init()'s + // own fast path (see interception.c) means this call never reaches + // pthread_once()/TSAN's interceptor at all. libpthread_sem_post(&unwrapped_arg->mc_pthread_create_binary_sem); + + mark_ckpt_window_candidate((int)syscall(SYS_gettid)); + void *rv = unwrapped_arg->routine(unwrapped_arg->arg); free(arg); return rv; @@ -591,6 +843,17 @@ int mc_pthread_create(pthread_t *thread, const pthread_attr_t *attr, // and creates no other threads during execution static pthread_once_t main_thread_once = PTHREAD_ONCE_INIT; + // libmcmini's own helper thread (e.g. the template thread). It reached us + // through the sanitizer's pthread_create interceptor (if any) -- so TSAN has + // already registered it and wrapped `routine` -- and we now just hand it to + // DMTCP, so it is DMTCP-known, with no user-thread machinery. Creating it via + // the public pthread_create (rather than libdmtcp_pthread_create directly) + // is deliberate: it keeps the thread visible to libtsan, so its ThreadState + // survives checkpoint/restart. See TSAN-McMini-DMTCP.txt. + if (mc_creating_internal_thread) { + return libdmtcp_pthread_create(thread, attr, routine, arg); + } + // TODO: Reduce code duplication here! switch (get_current_mode()) { case PRE_DMTCP_INIT: { @@ -603,18 +866,25 @@ int mc_pthread_create(pthread_t *thread, const pthread_attr_t *attr, case PRE_CHECKPOINT_THREAD: { pthread_once(&main_thread_once, &record_main_thread); - // We must be able to at runtime determine which thread is the checkpoint - // thread. Here we record the `pthread_t` struct assigned to the - // checkpoint thread and later compare it with the value returned by - // `pthread_create()`. + // This call may be the real checkpoint thread's own creation, or a + // nested spawn of TSAN's background thread happening as a side effect + // of it (see dmtcp_create_checkpoint_thread_wrapper()'s own comment). + // Either way, the new thread figures out which one it is for itself, + // once running; we just need to synchronize enough to know it has + // started before returning here. // - // NOTE: The semaphore is necessary here as the child thread in this - // instance will be the checkpoint thread and will hence call into DMTCP. - // Since the goal of detecting if the caller is the checkpoint thread - // inside of wrappers is to prevent the checkpoint thread from interacting - // with McMini wrappers, and since we write the checkpoint thread's - // `pthread_t` into a globally accessible location, we must synchronize - // with the checkpoint thread. + // Force libmcmini_init()'s lazy resolution to complete HERE, on this + // (already TSAN-registered) thread, before the new thread is spawned. + // The new thread may turn out to be TSAN's own background thread, + // which never gets its own TSAN ThreadState (see + // dmtcp_create_checkpoint_thread_wrapper()) -- calling libmcmini_init() + // there would reach pthread_once(), which TSAN's own interceptor + // intercepts and crashes on for exactly that reason. pthread_create() + // below is itself a synchronization point, so completing the + // resolution here guarantees it's visible on the new thread without + // it ever needing to call libmcmini_init() itself. + libmcmini_init(); + struct mc_thread_routine_arg *wrapped_arg = malloc(sizeof(struct mc_thread_routine_arg)); wrapped_arg->arg = arg; @@ -625,7 +895,7 @@ int mc_pthread_create(pthread_t *thread, const pthread_attr_t *attr, libpthread_sem_wait(&wrapped_arg->mc_pthread_create_binary_sem); return rc; } - case CHECKPOINT_THREAD: { + case EXTERNAL_THREAD: { log_warn( "The checkpoint thread is creating another thread. Calls from this " "thread should probably be ignored by the McMini library, as " @@ -640,6 +910,18 @@ int mc_pthread_create(pthread_t *thread, const pthread_attr_t *attr, case PRE_CHECKPOINT: case DMTCP_RESTART_INTO_BRANCH: case DMTCP_RESTART_INTO_TEMPLATE: { + // Force libmcmini_init()'s lazy resolution to complete HERE, on this + // (already TSAN-registered) thread, before the new thread is spawned. + // Under DMTCP, the new thread's own prologue (mc_thread_routine_wrapper + // -> mc_register_this_thread(), wrappers.c) runs before TSAN's own + // registration trampoline does (see TSAN-McMini-DMTCP.txt), so it has + // no TSAN ThreadState yet when it makes its own first wrapped call. + // pthread_create() below is itself a synchronization point, so + // completing the resolution here guarantees libmcmini_init()'s fast + // path (see interception.c) applies on the new thread, and it never + // needs to reach pthread_once()/TSAN's interceptor itself. + libmcmini_init(); + // TODO: add support for thread attributes struct mc_thread_routine_arg *libmcmini_controlled_thread_arg = malloc(sizeof(struct mc_thread_routine_arg)); @@ -678,10 +960,18 @@ int mc_pthread_create(pthread_t *thread, const pthread_attr_t *attr, // unless we later want to checkpoint this branch, we don't need to // inform DMTCP of these new threads. Indeed, in classic model checking // mode, `libdmtcp.so` is not even loaded (so we'd have to check first anyway). - // Calling `libpthread_pthread_create` simplifies all this. - const int rv = - libpthread_pthread_create(thread, attr, &mc_thread_routine_wrapper, - libmcmini_controlled_thread_arg); + // + // Use tsan_or_real_pthread_create(), NOT libpthread_pthread_create(): + // the latter always resolves straight to real glibc, bypassing TSan's + // own pthread_create interceptor entirely, so this thread never gets a + // TSan ThreadState. In classic mode that's a live landmine: this very + // thread's own first call into a libpthread_* wrapper triggers + // libmcmini_init()'s pthread_once(), which TSan also intercepts, and + // TSan dereferences this thread's (nonexistent) ThreadState and SEGVs. + // See doc/classic-mode-thread-registration-segv.txt. + const int rv = tsan_or_real_pthread_create( + thread, attr, &mc_thread_routine_wrapper, + libmcmini_controlled_thread_arg); // IMPORTANT: We need to ensure that the thread that is // created has been assigned an id; otherwise, there is a race condition @@ -701,7 +991,29 @@ int mc_pthread_create(pthread_t *thread, const pthread_attr_t *attr, } } -int mc_pthread_join(pthread_t t, void **rv) { +// Shared implementation for mc_pthread_join() and mc_pthread_join_maybe_defer(). +// +// If defer_real_join is false, behaves exactly as a normal join wrapper: +// whenever a real join is warranted, it performs it directly (via +// libpthread_pthread_join(), which bypasses TSan's own interceptor) and +// *deferred is set to false. +// +// If defer_real_join is true, then whenever a real join is warranted, this +// function does everything BUT the actual join call -- model bookkeeping, +// granting exit permission -- and sets *deferred to true, returning a +// value the caller must ignore. The caller (see __wrap_pthread_join(), in +// pthread_join_wrap.c, linked directly into the target alongside +// `-Wl,--wrap=pthread_join`) must then call __real_pthread_join() itself +// and return its result. This exists because __real_pthread_join is only +// a valid symbol within the target's own -Wl,--wrap=pthread_join link; +// libmcmini.so (a separate, already-built shared library) cannot call it +// directly. Doing the real join this way -- through TSan's own +// interceptor, via __real_pthread_join, rather than bypassing it via +// libpthread_pthread_join() -- lets TSan establish its own happens-before +// edge for this join, instead of never seeing it at all. +static int mc_pthread_join_impl(pthread_t t, void **rv, bool defer_real_join, + bool *deferred) { + *deferred = false; switch (get_current_mode()) { case PRE_DMTCP_INIT: { // This case implies that DMTCP attempted to join @@ -711,7 +1023,7 @@ int mc_pthread_join(pthread_t t, void **rv) { assert(0); } case PRE_CHECKPOINT_THREAD: - case CHECKPOINT_THREAD: { + case EXTERNAL_THREAD: { return libdmtcp_pthread_join(t, rv); } case RECORD: @@ -724,9 +1036,36 @@ int mc_pthread_join(pthread_t t, void **rv) { assert(thread_record != NULL); libpthread_mutex_unlock(&rec_list_lock); - struct timespec time = {.tv_sec = 2, .tv_nsec = 0}; while (1) { - int rc = pthread_timedjoin_np(t, rv, &time); + // Recompute the deadline every iteration -- see mc_sem_wait()'s + // identical pattern in sem-wrappers.c and mc_pthread_mutex_lock() + // above; a hoisted, one-time {.tv_sec = 2} is an already-past + // absolute deadline, not "2 seconds from now". + struct timespec time; + clock_gettime(CLOCK_REALTIME, &time); + time.tv_sec += 2; + // Use the libtsan-bypassing handle: a direct pthread_timedjoin_np would + // hit libtsan's interceptor and trip its thread-registry CHECK under + // DMTCP. See TSAN-McMini-DMTCP.txt. + // + // FIXME: bypassing TSAN's interceptor here means TSAN never sees this + // join, so at exit it reports "ThreadSanitizer: thread leak" for + // every thread joined this way, even though the real join above + // genuinely succeeded and reaped the thread. TSAN labels this a + // WARNING (not "ERROR: ThreadSanitizer: ..."), but it still counts + // toward TSAN's report tally and triggers TSAN's default nonzero + // exitcode (66) same as any other detected issue. If TSAN_OPTIONS + // includes halt_on_error:true (or halt_on_error=1), TSAN treats ANY + // detected issue -- this leak included -- as fatal and can exit + // immediately at the point of detection rather than letting the + // program run to completion. Accepted trade-off for now: the + // registry CHECK this bypass avoids is a hard abort, strictly worse + // than a leak report. A TSAN suppressions file line of the form + // "thread:PATH_TO/libmcmini.so" may suppress this specific leak + // report if it becomes a problem (not yet tried/verified here). + // "called_from_lib:libmcmini.so" is an alternate suppression line + // that may work instead (also not yet tried/verified). + int rc = libpthread_timedjoin_np(t, rv, &time); if (rc == 0) { // Join succeeded libpthread_mutex_lock(&rec_list_lock); thread_record->vo.thrd_state.status = EXITED; @@ -763,12 +1102,65 @@ int mc_pthread_join(pthread_t t, void **rv) { thread_handle_after_dmtcp_restart(); return 0; } - case TARGET_BRANCH: + case TARGET_BRANCH: { + // Never restarted: the target can only be a thread created via a + // real pthread_create(), so it always has a valid TSan Tid and a + // real join always works. + runner_id_t rid = search_pthread_map(t); + memcpy_v(thread_get_mailbox()->cnts, &rid, sizeof(runner_id_t)); + thread_get_mailbox()->type = THREAD_JOIN_TYPE; + thread_wake_scheduler_and_wait(); + + // Grant the target thread permission to really terminate now that + // it's been joined (see mc_exit_thread_in_child()), then perform a + // genuine join, so its OS thread actually dies -- and *rv is + // genuinely populated -- before we return, instead of only + // simulating success at the model level. + sem_t *exit_permission = find_exit_permission_sem(t); + assert(exit_permission != NULL); + libpthread_sem_post(exit_permission); + if (defer_real_join) { + *deferred = true; + return 0; + } + return libpthread_pthread_join(t, rv); + } case TARGET_BRANCH_AFTER_RESTART: { runner_id_t rid = search_pthread_map(t); memcpy_v(thread_get_mailbox()->cnts, &rid, sizeof(runner_id_t)); thread_get_mailbox()->type = THREAD_JOIN_TYPE; thread_wake_scheduler_and_wait(); + + // Grant the target thread permission to really terminate now that + // it's been joined (see mc_exit_thread_in_child()). + sem_t *exit_permission = find_exit_permission_sem(t); + assert(exit_permission != NULL); + libpthread_sem_post(exit_permission); + + pthread_map_t *target = find_pthread_map_entry(t); + assert(target != NULL); + if (target->registered_post_restart) { + // Created fresh after the restart via a real pthread_create(): + // has a valid TSan Tid, so a real join works normally. + if (defer_real_join) { + *deferred = true; + return 0; + } + return libpthread_pthread_join(t, rv); + } + if (!mc_pthread_is_recreated_thread(t)) { + // Never recreated: it already exited during RECORD mode, before + // any checkpoint. really_exited_sem is only posted post-restart, + // so waiting here would block forever -- the model already knows + // it's exited (see translate_recorded_runner_to_model()). + return 0; + } + // This thread predates the restart AND was recreated by DMTCP via + // clone(), bypassing TSan's own pthread_create() interceptor + // entirely. TSan has no Tid for it, so a real pthread_join() on it + // can never resolve (see TSAN-pthread-join.md). Wait for its own + // signal of genuine completion instead of joining it for real. + libpthread_sem_wait(&target->really_exited_sem); return 0; } default: { @@ -778,6 +1170,15 @@ int mc_pthread_join(pthread_t t, void **rv) { } +int mc_pthread_join(pthread_t t, void **rv) { + bool deferred; + return mc_pthread_join_impl(t, rv, false, &deferred); +} + +int mc_pthread_join_maybe_defer(pthread_t t, void **rv, bool *deferred) { + return mc_pthread_join_impl(t, rv, true, deferred); +} + unsigned mc_sleep(unsigned duration) { switch (get_current_mode()) { case TARGET_BRANCH: @@ -838,7 +1239,13 @@ int mc_pthread_cond_init(pthread_cond_t *cond, // notify_template_thread(); // thread_await_scheduler(); thread_handle_after_dmtcp_restart(); - return libpthread_cond_init(cond, attr); + // Do NOT call the real libpthread_cond_init() here: see + // doc/glibc-cond-var-desync.txt. Past this point mc_pthread_cond_wait() + // never touches the real cond_t again either, so reinitializing it + // would only risk racing a clone()-resurrected thread that is still + // genuinely blocked inside a pre-restart real pthread_cond_timedwait() + // call, for no benefit. + return 0; } case TARGET_BRANCH: case TARGET_BRANCH_AFTER_RESTART: { @@ -846,7 +1253,9 @@ int mc_pthread_cond_init(pthread_cond_t *cond, mb->type = COND_INIT_TYPE; memcpy_v(mb->cnts, &cond, sizeof(cond)); thread_wake_scheduler_and_wait(); - return libpthread_cond_init(cond, attr); + // See the DMTCP_RESTART_INTO_BRANCH/TEMPLATE case above: no real + // libpthread_cond_init() call here either. + return 0; } default: { libc_abort(); @@ -888,9 +1297,16 @@ int mc_pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mutex) { cond_record->vo.cond_state.count++; libpthread_mutex_unlock(&rec_list_lock); - struct timespec wait_time = {.tv_sec = 2, .tv_nsec = 0}; int rc; while (1) { + // Recompute the deadline every iteration -- see mc_sem_wait()'s + // identical pattern in sem-wrappers.c; a hoisted, one-time + // {.tv_sec = 2} is an already-past absolute deadline, not "2 + // seconds from now", so every call would return ETIMEDOUT + // immediately instead of ever genuinely blocking. + struct timespec wait_time; + clock_gettime(CLOCK_REALTIME, &wait_time); + wait_time.tv_sec += 2; rc = libpthread_cond_timedwait(cond, mutex, &wait_time); if (rc == 0) { // The thread has successfully entered the waiting state. @@ -959,12 +1375,25 @@ int mc_pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mutex) { memcpy_v(mb->cnts, &cond, sizeof(cond)); memcpy_v(mb->cnts + sizeof(cond), &mutex, sizeof(mutex)); thread_handle_after_dmtcp_restart(); + if (__tsan_release) __tsan_release(mutex); libpthread_mutex_unlock(mutex); mb->type = COND_WAIT_TYPE; memcpy_v(mb->cnts, &cond, sizeof(cond)); memcpy_v(mb->cnts + sizeof(cond), &mutex, sizeof(mutex)); - thread_handle_after_dmtcp_restart(); + // thread_handle_after_dmtcp_restart() (used for the COND_ENQUEUE_TYPE + // round above) is a one-time-per-thread restart check-in: it captures + // a getcontext() snapshot and posts the quiescence barrier that the + // background template thread waits on exactly once per real thread. By + // the time this second round runs, that barrier has already released + // and set mode to TARGET_BRANCH_AFTER_RESTART, so a second call falls + // into thread_handle_after_dmtcp_restart()'s `default: libc_abort()` + // (its mode_on_entry no longer matches DMTCP_RESTART_INTO_BRANCH/ + // TEMPLATE) -- confirmed live via dmesg showing a SIGABRT here. Use the + // ordinary wake+wait call instead, matching every other wrapper's + // second-and-later round with the coordinator. + thread_wake_scheduler_and_wait(); libpthread_mutex_lock(mutex); + if (__tsan_acquire) __tsan_acquire(mutex); return 0; } case TARGET_BRANCH: @@ -974,12 +1403,14 @@ int mc_pthread_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mutex) { memcpy_v(mb->cnts, &cond, sizeof(cond)); memcpy_v(mb->cnts + sizeof(cond), &mutex, sizeof(mutex)); thread_wake_scheduler_and_wait(); + if (__tsan_release) __tsan_release(mutex); libpthread_mutex_unlock(mutex); mb->type = COND_WAIT_TYPE; memcpy_v(mb->cnts, &cond, sizeof(cond)); memcpy_v(mb->cnts + sizeof(cond), &mutex, sizeof(mutex)); thread_wake_scheduler_and_wait(); libpthread_mutex_lock(mutex); + if (__tsan_acquire) __tsan_acquire(mutex); return 0; } default: { @@ -1004,11 +1435,23 @@ int mc_pthread_cond_signal(pthread_cond_t *cond) { libpthread_mutex_lock(&rec_list_lock); rec_list *cond_record = find_object_record_mode(cond); if (cond_record == NULL) { - fprintf(stderr, - "Undefined behavior: attempting to signal an uninitialized" - "condition variable %p", - cond); - libc_abort(); + // A real pthread_cond_init() call has the exact same TSan- + // interceptor-bypass problem pthread_cond_wait/signal do (never + // reaches mc_pthread_cond_init() under DMTCP), so cond_record can + // legitimately not exist yet here -- e.g. a producer that signals + // before any consumer has ever waited on this cond var. Mirror + // mc_pthread_cond_wait()'s own lazy-init instead of aborting: a + // signal with no one having waited yet is a well-defined no-op + // (matches condition_variable_signal::modify()'s own "signal with + // no waiters is a valid lost wakeup" semantics). + pthread_t this_thread = pthread_self(); + rec_list *thrd_record = find_thread_record_mode(this_thread); + visible_object vo = { + .type = CONDITION_VARIABLE, .location = cond, .cond_state = { .status = CV_INITIALIZED, + .interacting_thread = thrd_record->vo.thrd_state.id, + .associated_mutex = NULL, .count = 0, .waiting_threads = create_thread_queue() } + }; + cond_record = add_rec_entry_record_mode(&vo); } // Store pre-signal waiting count (only count CV_WAITING threads) int cv_waiting_count = 0; @@ -1061,7 +1504,17 @@ int mc_pthread_cond_signal(pthread_cond_t *cond) { // notify_template_thread(); // thread_await_scheduler(); thread_handle_after_dmtcp_restart(); - return libpthread_cond_signal(cond); + // Do NOT call the real libpthread_cond_signal() here: see + // doc/glibc-cond-var-desync.txt. mc_pthread_cond_wait() never blocks on + // the real cond_t in any post-restart mode (it is fully simulated via + // the mailbox handshake above), so a real signal here serves no + // purpose for the model checker -- it is pure risk. Specifically, a + // clone()-resurrected thread can still be genuinely, kernel-level + // blocked inside a pre-restart real pthread_cond_timedwait() call; a + // real signal reaching it here would wake it for real, letting it run + // further application code without the model checker's scheduling + // permission and breaking DPOR's single-stepping invariant. + return 0; } case TARGET_BRANCH: case TARGET_BRANCH_AFTER_RESTART: { @@ -1069,7 +1522,9 @@ int mc_pthread_cond_signal(pthread_cond_t *cond) { mb->type = COND_SIGNAL_TYPE; memcpy_v(mb->cnts, &cond, sizeof(cond)); thread_wake_scheduler_and_wait(); - return libpthread_cond_signal(cond); + // See the DMTCP_RESTART_INTO_BRANCH/TEMPLATE case above: no real + // libpthread_cond_signal() call here either. + return 0; } default: { // Wrapper functions should not be executing @@ -1093,11 +1548,19 @@ int mc_pthread_cond_broadcast(pthread_cond_t *cond) { libpthread_mutex_lock(&rec_list_lock); rec_list *cond_record = find_object_record_mode(cond); if (cond_record == NULL) { - fprintf(stderr, - "Undefined behavior: attempting to broadcast an uninitialized" - "condition variable %p", - cond); - libc_abort(); + // See mc_pthread_cond_signal()'s identical lazy-init: a broadcast + // with no one having waited yet is a well-defined no-op, not + // undefined behavior -- cond_record can legitimately not exist yet + // if pthread_cond_init() hasn't reached the model (same TSan- + // interceptor-bypass problem wait/signal have). + pthread_t this_thread = pthread_self(); + rec_list *thrd_record = find_thread_record_mode(this_thread); + visible_object vo = { + .type = CONDITION_VARIABLE, .location = cond, .cond_state = { .status = CV_INITIALIZED, + .interacting_thread = thrd_record->vo.thrd_state.id, + .associated_mutex = NULL, .count = 0, .waiting_threads = create_thread_queue() } + }; + cond_record = add_rec_entry_record_mode(&vo); } libpthread_mutex_unlock(&rec_list_lock); int rc = libpthread_cond_broadcast(cond); @@ -1123,7 +1586,10 @@ int mc_pthread_cond_broadcast(pthread_cond_t *cond) { mb->type = COND_BROADCAST_TYPE; memcpy_v(mb->cnts, &cond, sizeof(cond)); thread_handle_after_dmtcp_restart(); - return libpthread_cond_broadcast(cond); + // Do NOT call the real libpthread_cond_broadcast() here: see + // mc_pthread_cond_signal()'s identical reasoning above and + // doc/glibc-cond-var-desync.txt. + return 0; } case TARGET_BRANCH: case TARGET_BRANCH_AFTER_RESTART: { @@ -1131,7 +1597,9 @@ int mc_pthread_cond_broadcast(pthread_cond_t *cond) { mb->type = COND_BROADCAST_TYPE; memcpy_v(mb->cnts, &cond, sizeof(cond)); thread_wake_scheduler_and_wait(); - return libpthread_cond_broadcast(cond); + // See the DMTCP_RESTART_INTO_BRANCH/TEMPLATE case above: no real + // libpthread_cond_broadcast() call here either. + return 0; } default: { libc_abort(); @@ -1181,7 +1649,12 @@ int mc_pthread_cond_destroy(pthread_cond_t *cond) { mb->type = COND_DESTROY_TYPE; memcpy_v(mb->cnts, &cond, sizeof(cond)); thread_handle_after_dmtcp_restart(); - return libpthread_cond_destroy(cond); + // Do NOT call the real libpthread_cond_destroy() here: see + // mc_pthread_cond_signal()'s identical reasoning above and + // doc/glibc-cond-var-desync.txt. The model checker already tracks + // CV_DESTROYED itself; the real cond_t's memory is never touched + // again regardless. + return 0; } case TARGET_BRANCH: case TARGET_BRANCH_AFTER_RESTART: { @@ -1189,7 +1662,9 @@ int mc_pthread_cond_destroy(pthread_cond_t *cond) { mb->type = COND_DESTROY_TYPE; memcpy_v(mb->cnts, &cond, sizeof(cond)); thread_wake_scheduler_and_wait(); - return libpthread_cond_destroy(cond); + // See the DMTCP_RESTART_INTO_BRANCH/TEMPLATE case above: no real + // libpthread_cond_destroy() call here either. + return 0; } default: { libc_abort(); diff --git a/src/mcmini/mcmini.cpp b/src/mcmini/mcmini.cpp index 025c28e3..39c5b870 100644 --- a/src/mcmini/mcmini.cpp +++ b/src/mcmini/mcmini.cpp @@ -52,11 +52,18 @@ visible_object_state* translate_recorded_object_to_model( // fine too. switch (recorded_object.type) { case MUTEX: { - auto mutex_state = - static_cast(recorded_object.mut_state); + auto mutex_state = static_cast( + recorded_object.mut_state.status); pthread_mutex_t* mutex_location = (pthread_mutex_t*)recorded_object.location; - return new objects::mutex(mutex_state, mutex_location); + // Restore the owner too: a checkpoint can land while a thread is mid + // pthread_cond_wait(), still modeled as the mutex's holder until the + // enqueue transition runs (condition_variable_enqueue_thread::modify() + // requires mutex->is_locked_by(executor)). Without this, owner is + // left default-constructed/unset, so that check always fails and the + // restored state deadlocks immediately. + return new objects::mutex(mutex_state, mutex_location, + recorded_object.mut_state.owner); } case CONDITION_VARIABLE: { // Create the condition variable model object with full state information @@ -142,14 +149,64 @@ void found_abnormal_termination( for (const auto& t : program_model.get_trace()) { ss << "thread " << t->get_executor() << ": " << t->to_string() << "\n"; } + // `ub.culprit` may not have a pending transition in the model's current + // view: this callback can also fire for a termination_error raised while + // `coordinator::return_to_depth()` is replaying already-recorded history + // against a freshly restarted process, and by the replay's target depth + // the culprit thread may already have exited in the model. const transition* terminator = program_model.get_pending_transition_for(ub.culprit); - ss << "thread " << terminator->get_executor() << ": " - << terminator->to_string() << "\n"; + if (terminator != nullptr) { + ss << "thread " << terminator->get_executor() << ": " + << terminator->to_string() << "\n"; + } else { + ss << "thread " << ub.culprit << ": (no longer pending)\n"; + } + + ss << "\nNEXT THREAD OPERATIONS\n"; + for (const auto& tpair : program_model.get_pending_transitions()) { + if (terminator != nullptr && tpair.first == terminator->get_executor()) { + ss << "thread " << tpair.first << ": executing" + << "\n"; + } else { + ss << "thread " << tpair.first << ": " << tpair.second->to_string() + << "\n"; + } + } + ss << stats.total_transitions + 1 << " total transitions executed" + << "\n"; + std::cout << ss.str(); + std::cout.flush(); +} + +void found_nonzero_exit_code( + const coordinator& c, const stats& stats, + const real_world::process::nonzero_exit_code_error& nzec) { + std::cerr << "NONZERO EXIT CODE (" << nzec.exit_code << "):\n" + << nzec.what() << std::endl; + + std::stringstream ss; + const auto& program_model = c.get_current_program_model(); + ss << "TRACE " << stats.trace_id << "\n"; + for (const auto& t : program_model.get_trace()) { + ss << "thread " << t->get_executor() << ": " << t->to_string() << "\n"; + } + // See found_abnormal_termination()'s identical comment: `nzec.culprit` may + // no longer have a pending transition in the model's current view if this + // fired while `coordinator::return_to_depth()` was replaying history. + const transition* culprit_transition = + program_model.get_pending_transition_for(nzec.culprit); + if (culprit_transition != nullptr) { + ss << "thread " << culprit_transition->get_executor() << ": " + << culprit_transition->to_string() << "\n"; + } else { + ss << "thread " << nzec.culprit << ": (no longer pending)\n"; + } ss << "\nNEXT THREAD OPERATIONS\n"; for (const auto& tpair : program_model.get_pending_transitions()) { - if (tpair.first == terminator->get_executor()) { + if (culprit_transition != nullptr && + tpair.first == culprit_transition->get_executor()) { ss << "thread " << tpair.first << ": executing" << "\n"; } else { @@ -195,6 +252,7 @@ void do_model_checking(const config& config) { c.deadlock = &found_deadlock; c.undefined_behavior = &found_undefined_behavior; c.abnormal_termination = &found_abnormal_termination; + c.nonzero_exit_code = &found_nonzero_exit_code; classic_dpor_checker.verify_using(coordinator, c); std::cout << "Model checking completed!" << std::endl; } @@ -319,6 +377,7 @@ void do_model_checking_from_dmtcp_ckpt_file(const config& config) { c.undefined_behavior = &found_undefined_behavior; c.deadlock = &found_deadlock; c.abnormal_termination = &found_abnormal_termination; + c.nonzero_exit_code = &found_nonzero_exit_code; classic_dpor_checker.verify_using(coordinator, c); std::cerr << "Deep debugging completed!" << std::endl; } diff --git a/src/mcmini/model/transitions/condition_variables.cpp b/src/mcmini/model/transitions/condition_variables.cpp index c5af6d7a..fdb3db55 100644 --- a/src/mcmini/model/transitions/condition_variables.cpp +++ b/src/mcmini/model/transitions/condition_variables.cpp @@ -15,13 +15,12 @@ model::transition* cond_init_callback(runner_id_t p, // Locate the corresponding model of this object if (!m.contains(remote_cond)) { - // FIXME: Allow dynamic selection of wakeup policies. - // For now, we hard-code it here. Not great, but at least - // we can change it relatively easily still - ConditionVariablePolicy* policy = new ConditionVariableArbitraryPolicy(); - m.observe_object(remote_cond, - new condition_variable( - condition_variable::state::cv_initialized, policy)); + // FIXME: Allow dynamic selection of wakeup policies. For now, the + // constructor's default (a fresh ConditionVariableArbitraryPolicy) is + // hard-coded. Not great, but at least we can change it relatively + // easily still. + m.observe_object(remote_cond, + new condition_variable(condition_variable::state::cv_initialized)); } state::objid_t const cond = m.get_model_of_object(remote_cond); diff --git a/src/mcmini/model/transitions/mutex.cpp b/src/mcmini/model/transitions/mutex.cpp index 1757a7e7..72cd074d 100644 --- a/src/mcmini/model/transitions/mutex.cpp +++ b/src/mcmini/model/transitions/mutex.cpp @@ -14,7 +14,8 @@ model::transition* mutex_init_callback(runner_id_t p, // Locate the corresponding model of this object if (!m.contains(remote_mut)) - m.observe_object(remote_mut, new mutex(mutex::state::uninitialized)); + m.observe_object(remote_mut, + new mutex(mutex::state::uninitialized, remote_mut)); state::objid_t const mut = m.get_model_of_object(remote_mut); return new transitions::mutex_init(p, mut); diff --git a/src/mcmini/model_checking/algorithms/classic_dpor.cpp b/src/mcmini/model_checking/algorithms/classic_dpor.cpp index 2504f213..d9dbde56 100644 --- a/src/mcmini/model_checking/algorithms/classic_dpor.cpp +++ b/src/mcmini/model_checking/algorithms/classic_dpor.cpp @@ -197,7 +197,22 @@ void classic_dpor::verify_using(coordinator &coordinator, // backtracking. log_debug(dpor_logger) << "Backtracking to depth `" << (dpor_stack.size() - 1) << "`"; - coordinator.return_to_depth(dpor_stack.size() - 1); + try { + coordinator.return_to_depth(dpor_stack.size() - 1); + } catch (const real_world::process::termination_error &te) { + // The process spawned to replay history up to this depth (see + // `coordinator::return_to_depth()`) died before the replay + // finished. Report it the same way a termination during forward + // exploration is reported, rather than letting it escape + // unhandled all the way out of `verify_using()`. + if (callbacks.abnormal_termination) + callbacks.abnormal_termination(coordinator, model_checking_stats, te); + return; + } catch (const real_world::process::nonzero_exit_code_error &nzec) { + if (callbacks.nonzero_exit_code) + callbacks.nonzero_exit_code(coordinator, model_checking_stats, nzec); + return; + } log_debug(dpor_logger) << "Finished backtracking to depth `" << (dpor_stack.size() - 1) << "`"; model_checking_stats.trace_id++; diff --git a/src/mcmini/real_world/dmtcp_process_source.cpp b/src/mcmini/real_world/dmtcp_process_source.cpp index 89b79dff..268d2dbe 100644 --- a/src/mcmini/real_world/dmtcp_process_source.cpp +++ b/src/mcmini/real_world/dmtcp_process_source.cpp @@ -66,11 +66,3 @@ std::unique_ptr dmtcp_process_source::make_new_process() { // assert(tstruct->cpid == target_branch_pid); return extensions::make_unique(target_branch_pid); } - -dmtcp_process_source::~dmtcp_process_source() { - target dmtcp_cleanup( - "dmtcp_command", - {"-q", "--port", std::to_string(this->coordinator_target.get_port())}); - dmtcp_cleanup.set_quiet(true); - dmtcp_cleanup.launch_and_wait(); -} diff --git a/src/mcmini/real_world/local_linux_process.cpp b/src/mcmini/real_world/local_linux_process.cpp index 75c46036..c784a7f8 100644 --- a/src/mcmini/real_world/local_linux_process.cpp +++ b/src/mcmini/real_world/local_linux_process.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -52,6 +53,29 @@ local_linux_process::~local_linux_process() { } else { log_error(process_logger) << "Error: " << strerror(errno); } + } else { + // This death is expected and already fully handled (we just reaped + // it above): consume the SIGCHLD it generated, so it doesn't linger in + // signal_tracker's counter. Otherwise the next process this class + // creates (see coordinator::return_to_depth()/assign_new_process_handle(), + // which destroys the old handle immediately before creating a new + // one) would see that leftover count on its own first execute_runner() + // call and wrongly conclude *it* had just died, even though it's + // still alive and simply hasn't responded yet. + signal_tracker::instance().try_consume_signal(SIGCHLD); + + // The process we just killed may itself have left behind other + // now-orphaned descendants (e.g. its own private DMTCP coordinator, + // spawned by `dmtcp_restart --new-coordinator`) that die/reparent to + // us -- as the PR_SET_CHILD_SUBREAPER subreaper -- around the same + // time, generating their own SIGCHLDs. signal_tracker's counter isn't + // tied to a specific pid, so drain every zombie that's already + // reapable right now (non-blocking: this must never wait on a + // descendant that hasn't died yet) and consume one signal per reap, + // so none of them linger to be misattributed later either. + while (waitpid(-1, &status, WNOHANG) > 0) { + signal_tracker::instance().try_consume_signal(SIGCHLD); + } } } } @@ -83,40 +107,60 @@ volatile runner_mailbox *local_linux_process::execute_runner(runner_id_t id) { // TODO: The template process will also send a SIGCHLD if it dies // unexpectedly. Because we don't expect the template process to die, this is // OK for now, but should be handled in the future. - errno = 0; - signal_tracker::sig_semwait((sem_t *)&rmb->model_side_sem); - if (signal_tracker::instance().try_consume_signal(SIGCHLD)) { - // TODO: Get the true failure status from the template process via - // e.g. shared memory. - throw process::termination_error(SIGTERM, id, - "Process terminated abnormally."); - // // TODO: Double check that this - // // is the correct process that sent - // // the SIGCHILD using WNOHANG. - - // // `PR_SET_CHILD_SUBREAPER` enables us to wait on - // // this grandchild. - // int status; - // int rc = waitpid(this->pid, &status, 0); - // if (rc == -1) { - // throw process::execution_error( - // "Error attempting to determine the failure causing the child - // process " "to abnormally exit (or possibly an internal error of - // McMini)." + std::string(strerror(errno))); - // } else { - // if (WIFEXITED(status)) { - // int exit_code = WEXITSTATUS(status); - // throw process::nonzero_exit_code_error( - // exit_code, "Process terminated with a non-zero exit code."); - // } else if (WIFSIGNALED(status)) { - // int signo = WTERMSIG(status); - // throw process::termination_error(signo, - // "Process terminated abnormally."); - // } else { - // throw process::execution_error( - // "SIGSTOP/SIGCONT in branch processes is not yet supported."); - // } - // } + while (true) { + errno = 0; + signal_tracker::sig_semwait((sem_t *)&rmb->model_side_sem); + if (!signal_tracker::instance().try_consume_signal(SIGCHLD)) { + break; + } + // signal_tracker's SIGCHLD count is process-wide, not tied to a pid, so + // a pending count doesn't necessarily mean *this->pid* is the one that + // died -- some other descendant (e.g. a previous branch's own private + // DMTCP coordinator) may have generated it instead. Confirm with a + // non-blocking waitpid() specifically on this->pid (PR_SET_CHILD_SUBREAPER, + // set in target::prepare_mcmini_targets(), lets us wait on it even though + // it isn't a direct child): if it hasn't actually changed state, this + // SIGCHLD wasn't ours, so just resume waiting instead of misreporting a + // termination that didn't happen. + int status; + int rc = waitpid(this->pid, &status, WNOHANG); + if (rc == 0) { + continue; + } + if (rc == -1) { + throw process::execution_error( + "Error attempting to determine the failure causing the child " + "process to abnormally exit (or possibly an internal error of " + "McMini): " + std::string(strerror(errno))); + } else if (WIFEXITED(status)) { + const int exit_code = WEXITSTATUS(status); + if (exit_code != 0) { + throw process::nonzero_exit_code_error( + exit_code, id, "The program exited with code " + std::to_string(exit_code)); + } + // A clean (code 0) exit reaching this SIGCHLD-based detection path is + // not the same situation nonzero_exit_code_error reports: that + // exception means the *target program* exited abnormally, a bug for + // the user to fix. Here, the whole process fully terminated on its + // own while this runner still had a transition pending on it -- i.e. + // it bypassed the model-driven exit protocol (mc_transparent_exit(), + // fixed in commit 6bed56a, deliberately keeps the process alive + // across both of its mailbox rounds before ever calling the real + // exit(2)) rather than checking in normally. That is a McMini-side + // protocol violation to investigate, not a target-program bug. + throw process::execution_error( + "Runner " + std::to_string(id) + + "'s process exited normally (code 0) while a transition was " + "still pending on it, bypassing the model-driven exit protocol."); + } else if (WIFSIGNALED(status)) { + throw process::termination_error( + WTERMSIG(status), id, + "Process terminated abnormally by signal " + + std::to_string(WTERMSIG(status))); + } else { + throw process::execution_error( + "SIGSTOP/SIGCONT in branch processes is not yet supported."); + } } return rmb; } diff --git a/test/tsan_support/test_thread_blocks_signal.c b/test/tsan_support/test_thread_blocks_signal.c new file mode 100644 index 00000000..b9e19adc --- /dev/null +++ b/test/tsan_support/test_thread_blocks_signal.c @@ -0,0 +1,60 @@ +// Standalone host-side unit test for thread_blocks_signal(). Not an mcmini +// model-checking target: compile and run directly (see the command in the +// implementation plan / commit message), not through CMake. +#define _GNU_SOURCE +#include +#include +#include +#include +#include +#include +#include +#include + +#include "mcmini/spy/checkpointing/tsan_support.h" + +static sem_t ready; +static pid_t worker_tid; + +static void *worker(void *arg) { + (void)arg; + worker_tid = (pid_t)syscall(SYS_gettid); + + sigset_t set; + sigemptyset(&set); + sigaddset(&set, SIGUSR1); + pthread_sigmask(SIG_BLOCK, &set, NULL); + + sem_post(&ready); + + for (;;) { + pause(); // cancellation point; SIGUSR1 stays blocked the whole time + } + return NULL; +} + +int main(void) { + int rc = sem_init(&ready, 0, 0); + assert(rc == 0); + + pthread_t t; + rc = pthread_create(&t, NULL, worker, NULL); + assert(rc == 0); + + rc = sem_wait(&ready); + assert(rc == 0); + + pid_t self_tid = (pid_t)syscall(SYS_gettid); + + assert(thread_blocks_signal(worker_tid, SIGUSR1) == 1); + assert(thread_blocks_signal(self_tid, SIGUSR1) == 0); + assert(thread_blocks_signal(999999 /* bogus tid, should not exist */, SIGUSR1) == 0); + + rc = pthread_cancel(t); + assert(rc == 0); + rc = pthread_join(t, NULL); + assert(rc == 0); + + printf("PASS\n"); + return 0; +}