Skip to content

emrg: a size or an offset in the Win32 ABI is measured from its struct - #2518

Merged
argszero merged 1 commit into
masterfrom
fix/win32-literal-beside-struct
Oct 23, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/win32-literal-beside-struct

Conversation

@argszero

Copy link
Copy Markdown
Owner

Closes #2517

A Win32 structure is declared, and a size or an offset beside it is written down as a number. The number is whatever the width the author ran on answered, and no host where the call never executes can contradict it. #2509 was one instance of this class; the class itself was unguarded.

What changed

abi.py — declares Sid beside SECURITY_MAX_SID_SIZE and derives the constant from it (ctypes.sizeof(Sid)), so the number has a measurable source. The struct holds no pointer, so its layout is the same at either interpreter width — which is exactly why a literal 68 reads as correct and stays correct by accident.

ffi.py — declares AceHeader and AccessAllowedAce beside ACLStructure. The ACL walk read that layout at +0/+1/+2/+4/+8, which are ACE_HEADER and ACCESS_ALLOWED_ACE fields checkable against nothing.

acl.py — the ACE walk and _inline_sid_bytes now take every offset and length from those declarations (AceHeader.AceFlags.offset, AccessAllowedAce.SidStart.offset, Sid.SubAuthority.offset, sizeof(ACLStructure)) instead of integers. No behaviour change: the same bytes are read at the same addresses, verified by the file's existing tests.

tests/test_bash_v2_win32.py — two guards in the file's cross-platform half, plus the two buffer sites that had copied 68 with a comment now use abi.SECURITY_MAX_SID_SIZE (a comment is not a measurement).

The guard, and what it actually catches

test_a_size_or_an_offset_in_the_win32_abi_is_measured_from_its_struct parses every module of emrg/sandbox/win32/ and refuses two shapes:

  1. a module-level constant whose name ends in _SIZE/_LENGTH and whose value is a bare integer;
  2. an integer literal anywhere in a from_address/string_at argument — an offset must come from a field's .offset or a struct's size.

Rule 1 is not the rule the issue proposed, and this matters. #2517 suggested flagging "an integer literal in a create_string_buffer call". That would not have caught its own motivating defect: registry.apply_dacl already passed abi.SECURITY_DESCRIPTOR_MIN_LENGTH by name — the literal was one level up, in the constant's definition. Refusing size-like constants that are bare integers catches #2509 at its definition; an inline-literal rule would have stayed green through it.

The matcher is proven on snippets rather than on today's tree, so it discriminates independently of the state it reads: it fires on SECURITY_DESCRIPTOR_MIN_LENGTH = 20 and on base + 1, and stays quiet on ctypes.sizeof(...) and Base.Field.offset. It also asserts the package it scanned is non-empty, so it cannot pass by reading nothing.

test_the_declared_win32_structs_measure_the_offsets_the_acl_walk_reads pins both directions — each value equals the struct's own measurement, and the struct has the shape the platform defines (sizeof(Sid) == 68, SubAuthority.offset == 8, Mask.offset == 4, SidStart.offset == 8, sizeof(ACLStructure) == 8), because a truncated declaration shrinks both sides together.

A finding this change had to fix to be honest

ctypes.wintypes.DWORD is c_ulong: four bytes on Windows and eight on a 64-bit Linux or macOS interpreter. Declaring AccessAllowedAce with it measured Mask at offset 8 and SidStart at 16 on this host — a different struct from the one Windows builds — so a guard written that way would have asserted the layout of the machine running it rather than the platform's. Measured on this checkout: sizeof(wintypes.DWORD) == 8. The new declarations use fixed-width ctypes types and measure 4/8 on every leg.

A pre-existing instance is left alone and reported rather than changed: SIDAndAttributes (ffi.py:71) also uses wintypes.DWORD. Its Attributes.offset and sizeof happen to agree on 32- and 64-bit, so there is no defect to fix here — but a struct that describes a Win32 layout should be declared in fixed-width types.

Scope decision, stated rather than hidden

ffi.py:415 allocates create_unicode_buffer(512) for FormatMessageW's text. No struct measures that: the API documents no maximum, and 512 is a chosen capacity, not a claim about a declaration. Flagging it would demand a name for a choice and dress the choice as a measurement, so the guard's subject is the ABI package's size-like constants and read offsets — the two places where a declaration exists to be asked. The same reasoning keeps the guard off the test file: create_string_buffer(16) there is a foreign size class, deliberately not a measurement of anything.

Verification

  • Four mutation arms, each red, each mutated line printed before the run so a no-op could not read as a survivor:
    • SECURITY_MAX_SID_SIZE = 68 back → tests/test_bash_v2_win32.py::test_a_size_or_an_offset_in_the_win32_abi_is_measured_from_its_struct FAILED
    • acl.py back to address + 1 → same node FAILED
    • Sid.SubAuthority truncated to 4 → tests/test_bash_v2_win32.py::test_the_declared_win32_structs_measure_the_offsets_the_acl_walk_reads FAILED
    • AccessAllowedAce.Mask back to wintypes.DWORD → same node FAILED
  • Local suite on this checkout: 5708 passed, 35 skipped in 254 s.
  • uv run python -c "from emrg.client.app import run_client" — import ok. uv run python -m emrg --help — rc 0. check-doc-count.py --measure — rc 0, 5743 collected.

Neither macOS nor Linux runs the Win32 calls, so a local green says the guards run, never that a DACL application is right. What this PR does establish on a non-Windows host is the thing #2509 needed: the declaration is pure Python, so the size and every offset are checked on the legs that stay green. The test-windows leg is what confirms the ported walk still reads real ACEs.

CI is pending on this head — not green, not passed.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261023-120159

This head was pushed by an earlier run of this loop, so this cycle may vote on it: the abstention
covers only the cycle that pushed the head, not the one after it. First vote, 0/3 → 1/3; it needs two
more cycles and this cycle does not merge it.

Read on 3a0306b0, both legs pass (test 3m25s, test-windows 4m18s), MERGEABLE/CLEAN.

What it does. SECURITY_MAX_SID_SIZE = 68 becomes ctypes.sizeof(Sid) against a declared Sid;
AceHeader and AccessAllowedAce are declared so acl.py's ACE walk reads .offset instead of
+1/+2/+4/+8; two create_string_buffer(68) copies in the tests take the constant instead. Two guards
in the cross-platform half of the test file.

Armed here, by me, before voting: SECURITY_MAX_SID_SIZE = 68 put back reddens
tests/test_bash_v2_win32.py::test_a_size_or_an_offset_in_the_win32_abi_is_measured_from_its_struct
(rc 1, 1 failed in 0.42s), mutated line printed first. Restored, tree clean.

The judgement I want on the record, because it is the part a later reader cannot re-derive: the AST
rule this PR installs is not the rule its issue proposed. The issue asked for "no integer literal in
a create_string_buffer call" — which would have stayed green through its own motivating defect, because
registry.apply_dacl already passed abi.SECURITY_DESCRIPTOR_MIN_LENGTH by name and the literal was
one level up, in the constant's definition. Flagging size-like constants that are bare integers is what
catches that class; flagging call arguments does not. A reviewer should check that the guard's stated
subject matches the rule it enforces.

And the finding that made the declarations correct: ctypes.wintypes.DWORD is c_ulong — 4 bytes on
Windows, 8 under LP64 — so a Win32 struct declared with it measures a different struct on the very
legs that run the guard (measured: Mask at 8, SidStart at 16, against the platform's 4 and 8). The
PR uses fixed-width types and says so. That is the defect worth catching in review here; everything else
is mechanical.

What it cannot do: neither macOS nor Linux executes the Win32 calls, so a local green says the
declarations are right, never that a DACL application is. The test-windows leg is the only confirmation
that the ported walk still reads real ACEs, and it passed.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261023-122839

Reviewed the landing tree, not the diff-to-master: this head sits one commit behind master, and on it diff(base, head) shows open_source_prompt.md and test_scheduler.py as reversals this PR never makes. check-merge-landing-diff.py names the landing as 4 paths (abi.py +29-2, acl.py +20-11, ffi.py +36-0, test_bash_v2_win32.py +113-2) and reports the backwards reading as the hazard it is, not as a defect.

The claims checked out against the PR's own tree rather than its body — sizeof(Sid) 68, SubAuthority.offset 8, SubAuthorityCount.offset 1, AceFlags.offset 1, Mask.offset 4, SidStart.offset 8, sizeof(ACLStructure) 8. The finding the body reports is real on this host: sizeof(ctypes.wintypes.DWORD) is 8 here, and declaring AccessAllowedAce with it moves Mask to offset 8 and SidStart to 16 — a different struct from the one Windows builds, on the legs that run the guard.

The guard was taken in both directions on this head, mutated line printed before each run, baseline green and restored green afterwards: SECURITY_MAX_SID_SIZE = 68 → FAILED, and ace_flags read at a literal +1 → FAILED. Worth noting what the rule catches that the issue proposed: refusing size-like constants that are bare integers finds #2509 at its definition, where an inline-literal rule would have stayed green.

check-merge-plan-suite.py 2518 → landing tree 56d9bf168141, suite OK on base f1ed9d59: 5708 passed, 36 skipped.

Neither macOS nor Linux executes the Win32 calls, so this vote is about the declarations and the walk that reads them, not about a DACL applied on Windows. That half belongs to test-windows, which passed on this head.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261023-123837

This head sits one commit behind master, so this review is of the tree the merge would produce, not of
diff(base, head). Measured on this cycle's own run:

  • check-merge-plan-suite.py 2518 — base f1ed9d59, landing tree 56d9bf168141, suite 5708 passed,
    36 skipped
    in 240.83s, rc=0.
  • check-merge-landing-diff.py 2518 — the landing changes 4 paths on the base: emrg/sandbox/win32/abi.py
    (+29 -2), acl.py (+20 -11), ffi.py (+36 -0), tests/test_bash_v2_win32.py (+113 -2). The two paths
    it also reports — open_source_prompt.md, test_scheduler.py — are the base's own later commit shown as
    reversals this PR never makes; rc=1 there is the reading hazard its own exit table names, not a defect.

The head does not move for this vote, so the votes already standing on 3a0306b0 remain valid. Both CI legs
passed on this head (test 3m25s, test-windows 4m18s).

What this vote is about, and what it is not: neither macOS nor Linux executes the Win32 calls, so a local
green says the declarations measure the structs Windows actually builds — not that a DACL is applied. That
half belongs to test-windows, the only leg that runs the ported ACE walk against real instances, and it
passed.

@argszero
argszero merged commit 5098e73 into master Oct 23, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

emrg: a size or an offset written as a literal, beside the struct that could measure it

1 participant