Repository navigation
emrg: a size or an offset in the Win32 ABI is measured from its struct - #2518
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
✅ 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— basef1ed9d59, landing tree56d9bf168141, 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.
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— declaresSidbesideSECURITY_MAX_SID_SIZEand 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 literal68reads as correct and stays correct by accident.ffi.py— declaresAceHeaderandAccessAllowedAcebesideACLStructure. The ACL walk read that layout at+0/+1/+2/+4/+8, which areACE_HEADERandACCESS_ALLOWED_ACEfields checkable against nothing.acl.py— the ACE walk and_inline_sid_bytesnow 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 copied68with a comment now useabi.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_structparses every module ofemrg/sandbox/win32/and refuses two shapes:_SIZE/_LENGTHand whose value is a bare integer;from_address/string_atargument — an offset must come from a field's.offsetor 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_buffercall". That would not have caught its own motivating defect:registry.apply_daclalready passedabi.SECURITY_DESCRIPTOR_MIN_LENGTHby 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 = 20and onbase + 1, and stays quiet onctypes.sizeof(...)andBase.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_readspins 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.DWORDisc_ulong: four bytes on Windows and eight on a 64-bit Linux or macOS interpreter. DeclaringAccessAllowedAcewith it measuredMaskat offset 8 andSidStartat 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-widthctypestypes and measure 4/8 on every leg.A pre-existing instance is left alone and reported rather than changed:
SIDAndAttributes(ffi.py:71) also useswintypes.DWORD. ItsAttributes.offsetandsizeofhappen 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:415allocatescreate_unicode_buffer(512)forFormatMessageW's text. No struct measures that: the API documents no maximum, and512is 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
SECURITY_MAX_SID_SIZE = 68back →tests/test_bash_v2_win32.py::test_a_size_or_an_offset_in_the_win32_abi_is_measured_from_its_structFAILEDacl.pyback toaddress + 1→ same node FAILEDSid.SubAuthoritytruncated to 4 →tests/test_bash_v2_win32.py::test_the_declared_win32_structs_measure_the_offsets_the_acl_walk_readsFAILEDAccessAllowedAce.Maskback towintypes.DWORD→ same node FAILEDuv 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-windowsleg is what confirms the ported walk still reads real ACEs.CI is pending on this head — not green, not passed.