Fix mprotect address for the W^X recent-allocation flip

[dspearson]

Summary

Under WRITE_XOR_EXECUTE_CODE, when a code allocation region is closed off while a code-write bracket is open, close_off_segment records it so that the end of the bracket can restore PROT_EXEC. It records the length measured from the region's base and the address taken from the garbage collector's sweep cursor. Those are the same value until a collection with target generation 0 moves the cursor, and different afterwards.

Once they differ, the mprotect at c/segment.c:687 is made at an object boundary. mprotect requires a page-aligned address, so the call fails with EINVAL and the process aborts in S_error_abort("failed to protect recent allocation segments"). On the runs where that object boundary happens to be page aligned the call succeeds instead, and protects a length measured from one place starting at another, running past the end of the region.

Where

c/segment.c:682-690, the sweep_next drain loop at the tail of enable_code_write:

    if (!on) {
      while ((sip = tgc->sweep_next[0][space_code]) != NULL) {
        tgc->sweep_next[0][space_code] = sip->sweep_next;
        addr = TO_VOIDP(sip->sweep_start);      /* :685 — the defect */
        bytes = sip->sweep_bytes;               /* :686 */
        if (mprotect(addr, bytes, flags) != 0) {
          S_error_abort("failed to protect recent allocation segments");
        }
      }
    }

The two fields are set together in close_off_segment, c/alloc.c:227-247:

    uptr bytes = (uptr)old - (uptr)base_loc;    /* :227 — from the region's base */
    ...
    si = SegInfo(addr_get_segment(base_loc));   /* :244 */
    si->sweep_start = sweep_loc;                /* :245 — from the sweep cursor */
#if defined(WRITE_XOR_EXECUTE_CODE)
    si->sweep_bytes = bytes;                    /* :247 */
#endif

Mechanism

S_find_more_gc_room takes a fresh region and sets both pointers to its base (c/alloc.c:271-276):

  new = build_ptr(seg, 0);
  ...
  tgc->base_loc[g][s] = new;
  tgc->sweep_loc[g][s] = new;

So a region always begins where a segment begins. While the two pointers are equal, sweep_start is base_loc, the window is exactly the region, and the code is correct. That is why this has gone unnoticed: in the ordinary case the wrong expression and the right one evaluate to the same address.

Allocation and sweeping then advance independently. A collection whose target generation is 0 sweeps generation 0 in place and raises the cursor to the allocation pointer (c/gc.c:971):

          t_tgc->sweep_loc[MAX_TG][s] = t_tgc->next_loc[MAX_TG][s];

Nothing lowers it again while the region stays in use, so from that point sweep_loc sits above base_loc by however much had been allocated. The next region closed off inside a bracket — which happens as soon as a code allocation exhausts the current one, and a fasl read will do it — is recorded at that raised cursor.

An allocation cursor stops at object boundaries. It has no reason to be page aligned, and generally is not.

Why this has never fired upstream

WRITE_XOR_EXECUTE_CODE is defined at exactly one place in the tree, c/version.h:346, inside the TARGET_OS_IPHONE arm. No other supported configuration compiles enable_code_write at all, so no other configuration reaches the drain loop.

That is worth stating explicitly, because it defeats the obvious shortcut: an argument of the form "Chez would fail on Linux too if the address were unaligned" proves nothing here, since a default Linux build does not compile this function. Define the macro explicitly on an ordinary host and the path compiles and the failure reproduces.

Reaching the divergence needs, in addition, a collection with target generation 0 — (collect 0 0) requests one — followed by enough code allocation to close a region off while a bracket is open.

How we hit it

We hit it porting Chez to an operating system of our own, with a kernel, a libc and a userland all written from scratch. Chez runs there natively as a threaded x86-64 build with 4K pages. Our kernel refuses to map any page both writable and executable, so the default x86-64 S_PROT_CODE of PROT_READ|PROT_WRITE|PROT_EXEC is not available to us; we define WRITE_XOR_EXECUTE_CODE for that target so that code is written RW and then flipped to RX. That is why we compile this function at all. The arithmetic reported here has nothing to do with our kernel — it is the same expression on every platform that compiles this loop.

Chez aborted at startup on failed to protect recent allocation segments, which is what sent us into this function. Two separate faults were waiting there. Our own kernel had a conformance bug of its own in the same call — it rejected an mprotect length that was not a page multiple, where POSIX permits one, and sweep_bytes is an allocation distance that is essentially never a page multiple — and fixing that cleared the abort we had captured. We have not established which of the two produced that particular capture, and I would not want the report to rest on it.

What the report rests on instead is the code and the reproduction below, which needs none of our platform.

The abort message is a useful discriminator when reading captures. c/segment.c:676 is "failed to protect current allocation segments" — the whole-region flip, whose address is base_loc and which is therefore always correct. This is :688, "recent". The two messages differ by one word and by which branch they prove.

Reproduction on stock x86-64 Linux

Built from main at 659c263e, Chez 10.5.0-pre-release.1, machine type ta6le, ./configure --threads --disable-x11 --disable-curses CFLAGS+=-DWRITE_XOR_EXECUTE_CODE, GCC 16.1.0, 4K pages. The two arms differ by exactly one line of c/segment.c; mats/7.ms, c/types.h and everything else are identical between them, and scheme was confirmed relinked (its sha256 changed) before each run.

c/segment.c:685	zuo . 7.mo o=3	mats started	Finished loading mat	failed to protect recent allocation segments
addr = TO_VOIDP(sip->sweep_start) (current)	exit 1, "mat did not finish"	30	0	1
addr = TO_VOIDP(build_ptr(sip->number, 0)) (fix)	exit 0	49	1	0

The pre-fix run stops inside code-write-flip. 7.out ends:

compiling testfile-code-write-flip.ss with output to testfile-code-write-flip.so
failed to protect recent allocation segments

Instrumenting the drain loop to print both candidate addresses gives one line per arm, and they differ only in which one reaches mprotect:

pre-fix   sweep_start=0x42a96880  seg_base=0x42a88000  cursor_above_base=59520
          used=0x42a96880  used&0xfff=0x880  bytes=262080  -> mprotect errno=22 (EINVAL) -> abort

post-fix  sweep_start=0x42cb6880  seg_base=0x42ca8000  cursor_above_base=59520
          used=0x42ca8000  used&0xfff=0x000  bytes=262080  -> ok

The cursor sits 59520 bytes above the region base in both arms, so the passing run is not a case of the path going unexercised — the divergence happens either way. The only difference is that sweep_start is 0x880 into a page and the segment base is not. bytes is identical in both, which is the point: it was always measured from the base.

The fix

Take the address from the segment rather than from the cursor:

-        addr = TO_VOIDP(sip->sweep_start);
+        /* sweep_start is the collector's cursor, not the region's base */
+        addr = TO_VOIDP(build_ptr(sip->number, 0));

This is base_loc by construction, and therefore the address the length was measured from. The argument is in two steps, both from the code quoted above:

1. A region always begins where a segment begins, because S_find_more_gc_room sets base_loc to build_ptr(seg, 0).
2. The seginfo carrying the record is the one for the segment containing base_loc — close_off_segment obtains it as SegInfo(addr_get_segment(base_loc)).

So build_ptr(sip->number, 0) is the base of the segment the region starts in, which is the region's base. It is also how the rest of enable_code_write forms addresses: the hint path at :658 is build_ptr(seg, 0), and the current-region flip at :669 uses base_loc directly.

The patch also corrects the field's comment in c/types.h:162, which described sweep_bytes as "total number of bytes starting at sweep_start". It never was; it is measured from the region's base. The comment is arguably the root of the defect, since it documents the pairing the code then implements.

Why the ordinary case was always correct

Where no target-generation-0 collection has intervened, sweep_loc == base_loc == build_ptr(sip->number, 0), and the old and new expressions are the same address. The change is a no-op on any run that could not reach the bug.

The repair that does not work

Making the length agree with the cursor — sweep_bytes = old - sweep_loc — is the natural-looking alternative and it fixes nothing. The address is still an object boundary, and mprotect rejects an unaligned address whatever length accompanies it. Recorded because it is the first thing one tries.

Testing

mats/7.ms gains (mat code-write-flip ...), which drives the sequence from Scheme: generation-0 code kept live, a (collect 0 0) to raise the cursor off the region base, enough further code allocation to close a region off, and then a fasl read to drain the sweep_next chain. It checks that the live procedures still run afterwards and that the loaded definition works.

It can only fail where WRITE_XOR_EXECUTE_CODE is defined; on every other configuration it runs the same Scheme and passes, in about a quarter of a second. That is the most a mat can do here, since the defect is in a function those configurations do not compile, but it does mean the sequence is exercised on every platform rather than bit-rotting.

Measured both ways in the reproduction above: it aborts the run on the current code and passes on the fix, and 7.ms otherwise reports 49 mats with no Bug, no unexpected Error and no invalid memory.

Release notes

release_notes/release_notes.stex gains a Bug Fixes subsection, "Code-write protection after a generation-0 collection (10.5.0)".

Scope

• Independent of the companion PR, Fix mprotect addresses in the W^X chunk walk #1061. That one is the threaded chunk walk and needs a page size finer than bytes_per_segment; this one is the sweep_next drain and needs a target-generation-0 collection. They touch different lines, either can be taken without the other, and they merge cleanly together.
• Deliberately not reported here: threaded mats under WRITE_XOR_EXECUTE_CODE. Those do fail, but the comment above S_thread_start_code_write already documents the mechanism as best-effort and notes that a process-wide W^X disposition "seems incompatible" with foreign-thread callbacks. That is stated behaviour, not a defect. These two address bugs are a different category: arithmetic that is wrong on its own terms.
• I found no existing issue or PR covering this.

[mflatt]

This looks like right to me, and I'm planning to merge it.