[BUG] Module GIL does not protect the event loop: RM_CreateTimer from a module thread races the timer-list walk in aeProcessEvents #15582

Describe the bug

RedisModule_CreateTimer mutates the event loop's time-event list. The main thread walks that same list in usUntilEarliestTimer() after beforeSleep() has already released the module GIL. A module thread that holds the GIL is therefore not serialized against that walk, and can be observed mid-insertion.

This shows up as a SIGSEGV in the main thread. We have seen it twice in production on aarch64, ~3 months apart, on two different Redis versions (7.4.3 and 8.6.2). In both cases the faulting PC was the load of te->when inside the inlined usUntilEarliestTimer() loop, and in both cases the pointer being dereferenced held printable ASCII text rather than an address — i.e. the next link of the freshly inserted head node was not yet visible to the reading core, so the walker followed whatever the previous user of that heap block had left in the slot.

Details

In aeProcessEvents(), src/ae.c:

if (eventLoop->beforesleep != NULL && (flags & AE_CALL_BEFORE_SLEEP))
    eventLoop->beforesleep(eventLoop);            /* ae.c:383 */
...
usUntilTimer = usUntilEarliestTimer(eventLoop);   /* ae.c:394 — walks timeEventHead */

beforeSleep() ends with, src/server.c:2153:

if (moduleCount()) moduleReleaseGIL();
/********************* WARNING ********************
 * Do NOT add anything below moduleReleaseGIL !!! *
 ***************************** ********************/

That warning is respected inside beforeSleep() itself, but its caller keeps reading shared event-loop state after it returns. usUntilEarliestTimer() (ae.c:263) walks timeEventHead and every ->next with the GIL released.

Meanwhile RM_CreateTimer() (src/module.c:10331), which a module may call from a background thread while holding the GIL, does:

aeDeleteTimeEvent(server.el, aeTimer);                                      /* module.c:10363 */
aeTimer = aeCreateTimeEvent(server.el, period, moduleTimerHandler, NULL, NULL); /* module.c:10372 */

and aeCreateTimeEvent() publishes the new head with plain stores and no release barrier:

te->next = eventLoop->timeEventHead;
if (te->next) te->next->prev = te;
eventLoop->timeEventHead = te;

On a weakly ordered architecture the walking thread can observe eventLoop->timeEventHead = te before te->next = old_head becomes visible, read the stale contents of the freshly allocated block as a pointer, and fault. On x86 the store-store ordering makes this practically unreachable, which is consistent with both observed crashes being aarch64.

Core already documents this exact hazard

RM_Yield() (src/module.c:2505) has the same problem and already solves it by bouncing the work to the main thread:

if (!pthread_equal(server.main_thread_id, pthread_self())) {
    /* If we are not in the main thread, we defer event loop processing to the main thread
     * after the main thread enters acquiring GIL state in order to protect the event
     * loop (ae.c) and avoid potential race conditions. */

So the principle — holding the GIL is not sufficient to touch ae.c — is already established in-tree. RM_CreateTimer simply never got the same treatment.

Same class, likely also affected

RM_EventLoopAdd() (module.c:10507) calls aeCreateFileEvent(server.el, ...) (module.c:10541), which mutates eventLoop->events[] and maxfd. aeApiPoll() reads events[fd].mask and writes fired[] inside the same GIL-released window. epoll_ctl concurrent with epoll_wait is safe, but the array accesses are not.

Possible fixes, in increasing order of blast radius

1. Defer only the ae mutation in RM_CreateTimer to the main thread when !pthread_equal(server.main_thread_id, pthread_self()), reusing the existing one-shot / server.module_pipe machinery. The rax insert can stay synchronous, since the returned RedisModuleTimerID is the rax key and Timers is already GIL-protected. The deferred part is only "re-arm aeTimer to match the earliest rax key", which is idempotent and whose logic already exists in moduleTimerHandler. Contained in module.c, no change to the hot path, and it fixes every module at once.
2. The same treatment for RM_EventLoopAdd.
3. Narrow the GIL release so that it wraps only aeApiPoll() instead of ending beforeSleep(). This closes the whole class in one go, but needs new hooks across the ae/module boundary in the hottest loop in the server.

One non-fix worth recording: moving the usUntilEarliestTimer() call to before beforesleep() does not work. beforeSleep() calls aeSetDontWait() and can itself create time events, and ae.c already notes that eventLoop->flags must be re-read after beforesleep returns. Computing the timeout earlier would use stale state and can delay module timers by up to a full sleep interval.

To reproduce

No deterministic reproducer. Conditions under which we saw it:

• aarch64 host.
• A module re-arming a timer via RedisModule_CreateTimer from a background thread (holding the GIL) roughly every 30–60 s, i.e. on the order of tens of thousands of arm operations over a multi-week uptime.
• Heavy small-allocation churn, so the size class aeTimeEvent is allocated from keeps getting recycled with unrelated data.

Note that AddressSanitizer will not find this — there is no out-of-bounds write anywhere; the bytes in the stale slot are simply the previous contents of a legitimately allocated block. ThreadSanitizer on server.el->timeEventHead, or a targeted harness that hammers RedisModule_CreateTimer from a background thread while the main loop idles on ARM, is the appropriate instrument.

Additional information

Found while root-causing shard crashes in RediSearch, which was calling RedisModule_CreateTimer from its GC thread pool under the GIL. That module has since been changed to defer the call to the main thread via RedisModule_EventLoopAddOneShot, which resolves it for that module — but the underlying core gap affects any module that arms a timer off the main thread, and the module API does not currently document RM_CreateTimer as main-thread-only.

Line numbers above are against unstable as of filing.