Background modex fence callback can write to a dead stack frame when MPI/Session init fails mid-stream

[jsquyres]

Summary

When the background (async) modex fence is started during MPI/Session initialization, the PMIx completion callback holds a pointer to a stack variable of the initializing function. If any of the init steps between starting that fence and waiting on it fails, the function returns with the fence still pending — and when the PMIx progress thread later delivers the completion callback, it writes into a dead stack frame. This is a latent stack-corruption bug on an error path.

Found during code review of the fence-status hardening work related to #14125 (the reviewer flagged it while examining the fence callbacks). It is pre-existing — the same shape, with a bare volatile bool, goes back years and exists on v5.0.x and v6.0.x as well.

Affected code (line numbers at main commit 8d9e1ec)

Two parallel copies of the same pattern:

1. ompi/instance/instance.c (ompi_mpi_instance_init_common) — the main window

• fence_release() callback: line 382 — writes *active = false (cbdata points into the caller's frame).
• volatile bool active declared on the stack: line 423.
• Background fence started: line 654 (PMIx_Fence_nb(..., fence_release, (void*)&active)), taken when !opal_process_info.is_singleton && opal_pmix_base_async_modex && opal_pmix_collect_all_data.
• Wait point: line 849–851 (OMPI_LAZY_WAIT_FOR_COMPLETION(active)).
• Between lines ~690 and ~849 there are 20+ error returns that exit with the fence pending, including failures of mca_pml_base_bsend_init, mca_coll_base_find_available, ompi_request_init, ompi_message_init, ompi_group_init, ompi_comm_init, ompi_attr_create_predefined_keyvals, ompi_file_init, ompi_win_init, mca_part_base_select, ompi_dpm_init, ompi_proc_complete_init, "PML control failed", "PML add procs failed", and more.

2. ompi/runtime/ompi_mpi_init.c (classic world-model path) — a narrower window

• Its own copy of fence_release(): line 395; stack active: line 411.
• Background fence started: line 546; wait point: line 630.
• Between them, the error returns are the FT-MPI init calls (ompi_comm_rbcast_init, ompi_comm_revoke_init, ompi_comm_failure_propagator_init, ompi_comm_failure_detector_init, lines ~595–602) — reachable only when FT is enabled (ompi_ftmpi_enabled).

Not affected: the blocking (non-async) modex fence and the init barrier wait inline immediately after starting, with no intervening error returns; ompi_mpi_finalize's fence likewise waits immediately. Singletons never start the fence.

Note: PR (forthcoming, from branch sessions-fence-hardening) reworks these callbacks to write a small struct (status + flag) instead of a bare bool. That work deliberately keeps the stack-allocation shape, so this issue applies before and after it — after it, the callback writes two fields into the dead frame instead of one. Whoever fixes this should build on top of that PR if it has landed (the callback/waiter sites are the same, just renamed to fence_sync).

Trigger conditions and blast radius

All of the following must hold:

1. Multi-process job (not a singleton).
2. Async modex enabled: --mca pmix_base_async_modex 1 (not the default), with data collection on (pmix_base_collect_data, default true). This is why the bug has been latent for years.
3. An init step between fence-start and fence-wait fails.

Consequence differs by path:

• Sessions path (the dangerous one): MPI_Session_init errors are returned to the application, which typically keeps running. The process stays alive with PMIx still connected, so when the other ranks enter the fence (or the server prunes/completes it), the progress thread fires fence_release and scribbles on a stack address that has since been reused by whatever the calling thread is now doing. Silent, delayed, hard-to-attribute corruption in a live application.
• Classic MPI_Init path: init errors are effectively fatal (the error handler aborts), so in practice the process usually dies before the callback fires — but the progress thread races the abort, so corruption of the aborting process is possible (mostly a nuisance for debuggers/core files).

Suggested fix

There is no PMIx fence-cancel API, so the pending callback cannot be revoked. Two workable designs; (a) is recommended:

(a) Heap-allocate the sync object with an atomic ownership handoff (recommended).

typedef struct {
    opal_atomic_int32_t state;   /* PENDING -> COMPLETE (callback) or ABANDONED (error path) */
    pmix_status_t status;
} ompi_fence_sync_t;

• Allocate with malloc before PMIx_Fence_nb; free on the normal path after the wait completes.
• Callback: write status, write barrier, then opal_atomic_swap_32(&sync->state, COMPLETE); if the previous value was ABANDONED, the callback frees the object.
• Error path (any return between start and wait): opal_atomic_swap_32(&sync->state, ABANDONED); if the previous value was COMPLETE (callback already ran), the error path frees it; otherwise ownership has passed to the callback.
• The atomic swap makes the free exactly-once regardless of which side loses the race. If PMIx never delivers the callback at all (e.g., server died), the abandoned object leaks — a bounded, error-path-only leak; note it in a comment.
• Wrap the abandon logic in a small helper and call it from the error returns between start and wait (in instance.c that's simplest as a goto-style cleanup or a macro, since there are 20+ sites; alternatively only the background-fence case needs the heap object — the inline-wait cases can keep using the stack).

(b) Drain the fence before any error return. Wait for active/fence_sync.active to clear before returning an error. Rejected as the primary approach: the fence may be wedged (that's often why init is failing — see #14125), and OMPI_LAZY_WAIT_FOR_COMPLETION has no timeout, so this can convert "return an error" into "hang forever."

(c) Make the sync object static file-scope. Rejected: a stale callback from a previous failed init attempt could fire during a subsequent init (Sessions apps can call MPI_Session_init again after a failure) and clobber the new attempt's state; fixing that needs generation counters, which is messier than (a).

Fix both copies (instance.c and ompi_mpi_init.c). Add your copyright line to both files.

How to reproduce / validate

Fault injection (deterministic): apply a temporary test patch in ompi_mpi_instance_init_common immediately after the background fence is started (after line ~665 on current main), e.g.:

if (NULL != getenv("OMPI_TEST_FAIL_AFTER_BG_FENCE")) {
    return ompi_instance_print_error ("injected test failure", OMPI_ERROR);
}

Reproducer app (Sessions, so the process outlives the error):

#include <mpi.h>
#include <unistd.h>
#include <stdio.h>
int main(void) {
    MPI_Session s;
    int rc = MPI_Session_init(MPI_INFO_NULL, MPI_ERRORS_RETURN, &s);
    fprintf(stderr, "Session_init rc=%d; sleeping so the pending fence callback can fire\n", rc);
    /* Burn stack so the dead frame gets reused, then give the PMIx
       progress thread time to deliver the fence callback. */
    sleep(10);
    return 0;
}

Run: mpirun --np 2 --mca pmix_base_async_modex 1 ./repro with OMPI_TEST_FAIL_AFTER_BG_FENCE=1 set on one rank only (e.g. via mpirun --np 1 -x OMPI_TEST_FAIL_AFTER_BG_FENCE=1 ./repro : --np 1 ./repro), so the other rank still enters the fence and drives it to completion on the server.

Detection: build Open MPI (at least libmpi/libopen-pal) and the reproducer with AddressSanitizer (CFLAGS="-fsanitize=address -g", link flags likewise) and run with ASAN_OPTIONS=detect_stack_use_after_return=1. On unfixed code the callback's store trips a stack-use-after-return report pointing at fence_release. A cruder alternative without ASan: add a temporary fprintf in fence_release and observe it firing after ompi_mpi_instance_init_common has already returned (the corruption itself is silent without a sanitizer).

Validation of the fix:

1. The ASan reproducer above runs clean (no stack-use-after-return, no double-free). Exercise both race outcomes: run several times, and also add a sleep(1) before the injected return so the callback sometimes completes before the error path abandons (exercising the "error path frees" arm) and sometimes after (the "callback frees" arm).
2. The injected-failure run still exits promptly with an error — no hang (this is the trap of fix option (b)).
3. No regressions on the normal path: mpirun --np 2 of examples/hello_c, ring_c, and hello_sessions_c (with and without --mca pmix_base_async_modex 1), and make check.
4. Remove the injection patch before committing.

Related

• MPI Sessions: repeated MPI_Session_init after full finalize deadlocks on macOS (shmem backing-file ENOENT + PMIx fence hang) #14125 — the Sessions fence desync/hang investigation that led to reviewing this code.
• The fence-status hardening PR from branch sessions-fence-hardening (forthcoming) — fixes silent ignoring of fence completion status; this issue is the separate lifetime problem in the same callbacks, kept out of that PR because the fix involves the ownership-handoff design above.

[rhc54]

Ouch - yes, your fix (a) is definitely the way to go.