[issues-find] [S3] Random32 updates RNG with interrupts enabled (src/random.c)

[evilchinesefood]

[issues-find] filed on 2026-09-02, lens Concurrency, at commit 545f501.

Severity: S3 (minor)

What is wrong
Random32 load-modify-stores gRngValue with interrupts enabled. VBlankIntr in src/main.c calls AdvanceRandom (line 383), which calls Random32 whenever sRngLoopUnlocked is TRUE; that lock is only taken around LOOP_RANDOM macros, not around Random32 itself.

Where
src/random.c:45 in Random32

u32 NAKED Random32(void)
{
    asm(".thumb\n\
    push {r4, r5, r6}\n\
    mov r6, #11\n\
    ldr r5, =gRngValue\n\
    ldmia r5!, {r1, r2, r3, r4}\n\
    @ result = a + b + (d+=STREAM1)\n\
    add r1, r1, r2\n\
    add r0, r1, r4\n\
    add r4, r4, #" STR(STREAM1) "\n\

Why it matters
A VBlank nested inside Random32 tears gRngValue, so later Random/Random32 results are wrong (and can desync non-link systems that consume the same stream).

How to observe it
Static finding; not executed.

Recommended fix
In src/random.c, make Random32 (and Random2_32) reuse sRngLoopUnlocked around the state update, matching LOOP_RANDOM_START/END; keep AdvanceRandom skipping when the lock is held.

Done when
A test in test/random.c that calls Random32 in a tight loop while VBlank AdvanceRandom is enabled never observes a gRngValue that is not a valid successor of the previous state.

Lead check: snippet present at src/random.c:45; claim confirmed.

[evilchinesefood]

[issues-review] Verdict: CONFIRMED (reviewed 2026-09-02)

Evidence
Random32 reads all four state words at src/random.c:51 and writes them back only at src/random.c:66, without changing sRngLoopUnlocked; AdvanceRandom checks that flag and calls Random32 at src/random.c:100-103.
VBlankIntr invokes AdvanceRandom at src/main.c:357-383; the header explicitly says AdvanceRandom exists for VBlank burning at include/random.h:15-18.

Corrections to this issue
The shown interleaving loses or duplicates an RNG advance rather than necessarily producing a torn hybrid state: a nested call between the load and store computes and stores the same successor that the interrupted call later overwrites. The lock is also used by SeedRng at src/random.c:83-85, and locking Random2_32 is not justified by this VBlank race because it updates separate gRng2Value at src/random.c:73-75.

Still needed before this can be fixed
Define whether the required guarantee is one advance per call or merely a valid state. The proposed test needs an interrupt-injection hook because normal test runs suppress VBlank AdvanceRandom when gTestRunnerEnabled is true at src/main.c:382-383; checking only for a valid successor would not catch the lost-advance interleaving.

Severity: S3 (minor)

Left open. Confirmed against the code; see Evidence. Ready for issues-fix.

[evilchinesefood]

[issues-fix] Working on this (run 20260902-181612-91395, started 2026-09-02).

[evilchinesefood]

[issues-fix] Fixed in PR #281 (commit c174209); the issue closes when the PR merges.