Fix COW race condition in mem_ptr under concurrent write faults

[evnchn]

Summary

I'm having a hard time to wrap my head around the fix. What I can say is:

1. It definitely does its thing, crashes less
2. We are shuffling more existing code inside the lock (if you use hide whitespace view, it helps a bit, but not fully), so the functionality would be the same unless we deadlock ourselves
3. We did not deadlock ourselves

Hence, I think this is safe to merge.

We have a TOCTOU in mem_ptr() COW path: data and entry pointers were derived under read lock but directly used after a lock upgrade gap. As another thread can also free/remap the page under this condition, it reliably crashes in _platform_memmove on iOS.

• The fix is to re-fetches the page table entry after the lock upgrade (CC: same pattern already used for the growsdown case 10 lines above (line 264))
• If another thread already resolved the COW, the mmap and memcpy are skipped entirely — no wasted allocation. (Human: I forced CC to do this with ultrathink)

The bug

Sorry but I don't understand the following fully. But I think since the code was not in a critical region, we can assume for every line of code, some other thread may be also executing any step of it. Hence why we can assume two threads simultaneously doing step 2 and 3, leading to the issue?

Claude Code's analysis

The COW path in mem_ptr() does a lock upgrade (read → write) to copy a COW page. The old code had this sequence:

// read lock held
void *data = (char *) entry->data->data + entry->offset;  // 1. derive pointer
void *copy = mmap(...);

read_wrunlock(&mem->lock);    // 2. release read lock
write_wrlock(&mem->lock);     // 3. GAP — another thread can COW the same page

memcpy(copy, data, PAGE_SIZE);                        // 4. data may be freed
pt_map(mem, page, 1, copy, 0, entry->flags &~ P_COW); // 4. entry may be freed

Between steps 2 and 3, another thread can acquire the write lock first and:

1. Complete its own COW copy of the same page (freeing the original backing data)
2. Remap or unmap the page entirely (invalidating entry)

Both data and entry become dangling pointers. Classic TOCTOU across the lock upgrade.

Crash signature

From iOS Simulator crash reports:

• Crashing function: _platform_memmove called from mem_ptr+772 (memory.c:286)
• Faulting address: just past the boundary of a VM_ALLOCATE region (backing data freed/unmapped by another thread)
• Exception type: EXC_BAD_ACCESS (SIGSEGV) — KERN_INVALID_ADDRESS

Non-deterministic, depends on thread scheduling, but reproduces within seconds under sufficient thread pressure.

The fix

If I am not mistaken, the fix is to get the read lock, then get the write lock, and only if it all checks out in both stages we do the crucial operations such as data, copy, memcpy, and pt_map?

Move data, copy, memcpy, and pt_map to after the write lock is acquired. Re-fetch entry via mem_pt() and re-check P_COW first. This matches the existing growsdown pattern directly above in the same function. If another thread already handled the COW, we skip straight past — no allocation, no cleanup needed.

ultrathink outcome: why P_COW is checked twice: The outer check (under read lock) is a performance gate — without it, every non-COW write would needlessly do a lock upgrade. The inner check (under write lock) is the correctness re-validation after the lock gap. Both are required; they serve different purposes.

How to reproduce

1. Build iSH for iOS Simulator
2. Install Alpine Linux, install Node.js 20 (apk add nodejs npm)
3. Run any multi-threaded Node.js workload with 10+ worker threads
4. On unpatched iSH, the app crashes to the home screen within seconds

How it was tested

On iOS Simulator (iPhone 16 Pro, iOS 18.4) with Node.js 20:

• Before fix: crashed to home screen within seconds, every attempt
• After fix: ran 7+ minutes under same workload, no crash

Process note

This fix was developed with Claude Code (Anthropic's AI coding agent). The investigation started from an iOS Simulator crash report showing _platform_memmove faulting inside mem_ptr. The lock upgrade gap was identified as a classic TOCTOU pattern on entry and data. The fix follows the existing pattern in the same function.

[emkey1]

I tried to produce an empirical before/after for this one and could not trigger the race, so treating that as a limitation of my harness rather than evidence against the patch — writing up both so the attempt is not repeated blindly.

What I ran: a MAP_PRIVATE file mapping, freshly remapped each round so every round takes real COW faults, with 4 threads each storing to a distinct byte in the same page, released together and checked afterwards. 4000 rounds.

real Linux:      lost writes: 0 over 4000 rounds x 4 threads
upstream master: lost writes: 0 over 4000 rounds x 4 threads   (when it completed)

Two caveats that make this weak evidence:

1. The window is a handful of instructions between read_wrunlock and write_wrlock. Four threads on one page is unlikely to land in it reliably.
2. Two of three runs on master died with "The futex facility returned an unexpected error code" before finishing — that is pthread_create/pthread_join internals, unrelated to this PR, but it makes threaded stress testing on master flaky in general. I had already dropped pthread_barrier for plain atomics for the same reason.

Reading the code, the hazard looks real regardless. The original computes

void *data = (char *) entry->data->data + entry->offset;

before dropping the read lock, then uses that pointer and entry->flags after taking the write lock. If another thread COWs the page in between, the second thread copies from the stale original and pt_maps its own copy over the first thread's — so this is a lost store, not merely a stale read, which matches "crashes less" being hard to pin down.

One data point that may be worth something: our fork hit this area too and ended up eliminating the same window by a different route — taking the write lock up front and re-reading the entry under it, rather than upgrading mid-fault. Different shape, same hazard removed. That is independent support that the window is real even though I could not make it fire on demand.

Happy to re-run the stress with a better repro if anyone has one that lands in the window more reliably.