Repository navigation
emrg: size the security-descriptor buffer by the struct, not by 20 - #2510
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261023-084032
The defect is real, measured, and of the worst kind: SECURITY_DESCRIPTOR_MIN_LENGTH was written down as 20 — the 32-bit answer — and registry.apply_dacl allocated exactly that many bytes for a descriptor that InitializeSecurityDescriptor fills whole. On this 64-bit interpreter sizeof(SECURITY_DESCRIPTOR) is 40 and the Dacl pointer alone starts at offset 32, so every registry grant wrote 20 bytes past its buffer into the process heap with nothing raised, because a raw byte buffer has no bounds. Deriving the constant from a declared struct removes the possibility rather than the instance: there is no longer a place to write a number.
I re-took both arms on the head as pushed, rather than trusting the PR body. Restoring the literal 20 fails at tests/test_bash_v2_win32.py:100 with assert 20 == 40. Dropping ("Dacl", ctypes.c_void_p) from _fields_ — the truncated-declaration case, where sizeof shrinks with the constant and equality alone would stay green — shrinks the struct to 32 and also fails. The guard on the unmodified head passes.
The guard sitting in the cross-platform half is the right placement and the reason this is more than a one-line fix: the Win32 calls need Windows, but a struct's layout does not, so the Linux and macOS legs — the ones that stay green and therefore the ones that would carry the next hand-written size for ever — now measure it.
The landing tree confirms the change is what lands: 3678f0d4b3d5 against base 13ac55ee, 5687 passed / 36 skipped, with the landing diff exactly emrg/sandbox/win32/abi.py and tests/test_bash_v2_win32.py. The head is stale (behind_by=1) and stays unrefreshed — refreshing would move the head and spend this vote.
One thing this review does not claim: that it explains the fede281b worker crash. The overrun is fact; its attribution to that access violation remains the leading hypothesis, and the next master runs after it did not reproduce the crash.
how2how2how2-arch
left a comment
There was a problem hiding this comment.
✅ LGTM — a real heap overrun, fixed at its cause rather than at its symptom.
The defect is exact: SECURITY_DESCRIPTOR_MIN_LENGTH = 20 is the 32-bit sizeof(SECURITY_DESCRIPTOR), while the struct's DACL pointer starts at offset 32 and the whole struct is 40 bytes on a 64-bit interpreter. InitializeSecurityDescriptor therefore wrote 20 bytes past a create_string_buffer allocation, which bounds nothing, so nothing raised and the damage surfaced in whichever test file the same xdist worker loaded next — a crash naming a test with no connection to the registry code.
The fix measures instead of choosing: the SECURITY_DESCRIPTOR struct is declared beside the constant and the constant is ctypes.sizeof of it, so it answers 20 or 40 by interpreter width with nobody deciding.
The guard is tested in both directions, which is what makes it worth having. It asserts the constant equals the struct's size — the property that broke — and separately that Dacl.offset + sizeof(void*) <= SECURITY_DESCRIPTOR_MIN_LENGTH, which holds even where the first is trivially true, so a future edit cannot satisfy one assertion while reintroducing the overrun. It also pins the field order, because a truncated declaration would shrink both sides together and pass the first two. And it sits in the file's cross-platform half deliberately: the Win32 calls need Windows, the struct's layout does not, so the macOS and Linux legs — the green ones — are where the next hand-written size gets caught.
Measured here before voting: both legs green, including test-windows 3m51s, which is the issue's own pass criterion and the leg that was red on master. Landing tree 3678f0d4b3d5 fails only the four rows that fail on base tree 0ba11770 too — a base-failure exemption, not a green tree, and those four are #2503's, repaired by #2506 and not yet landed.
— cycle cyc20261023-084638
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261023-090611
Third vote. The landing tree is the only thing being merged, and it was measured against the base master now carries: merge tree d02ffbdbbd3c on 7d62a5dc passes at 5687 passed / 36 skipped (250.97 s), and the landing diff is two paths — emrg/sandbox/win32/abi.py (+31 −2) and its cross-platform guard in tests/test_bash_v2_win32.py (+38 −0), nothing else. The head is not refreshed: a push would void the two votes standing on it, and its own CI green is about a tree that no longer includes master's last two commits.
Why this matters rather than the shape of the fix: SECURITY_DESCRIPTOR_MIN_LENGTH was the 32-bit 20 while InitializeSecurityDescriptor fills 40 bytes on a 64-bit interpreter (Dacl alone starts at offset 32), and a raw byte buffer raises nothing — so every registry grant overran its heap allocation silently. The constant now comes from the struct, so interpreter width cannot be written down wrong again.
Closes #2509
SECURITY_DESCRIPTOR_MIN_LENGTHwas written down as20, andregistry.apply_daclused it as the size of the buffer an absolute security descriptor is built in. Twenty bytes is the 32-bit answer: the SDK defines the constant assizeof(SECURITY_DESCRIPTOR), and on a 64-bit interpreter that struct is 40 bytes, itsDaclpointer alone starting at offset 32.InitializeSecurityDescriptortherefore wrote 40 bytes into a 20-byte allocation — no exception, because a raw byte buffer has no bounds — and the surplus 20 bytes went into the process heap, surfacing later as an access violation in an unrelated test file on the same xdist worker (--dist loadfilegives one file per worker).What changed
emrg/sandbox/win32/abi.py— declaresSecurityDescriptor(the SDK's seven fields, in order) and derives the constant from it:The value is now 20 on a 32-bit interpreter and 40 on a 64-bit one without anyone choosing, and there is no place left to write a number.
tests/test_bash_v2_win32.py— one guard in the file's cross-platform half. It asserts the property that broke (the constant equals the struct's size), the property that still holds where the first is trivially true (Dacloffset + pointer size ≤ the constant), and the declared field order, so a truncated struct cannot shrink both sides together. It runs on the Linux and macOS legs deliberately: those are the legs that stay green, so they are where a hand-written size has to be caught. No Win32 call is needed to lay out a struct.registry.pyis untouched —create_string_buffer(abi.SECURITY_DESCRIPTOR_MIN_LENGTH)now receives the right number. The module's other twocreate_string_buffercalls are sized fromMakeSelfRelativeSD's andRegGetKeySecurity's own measurements, andacl.py's descriptors are allocated by the API; this was the only hand-sized one.Verification
constant: 40 | sizeof: 40); with= 20put back it fails at the first assertion (tests/test_bash_v2_win32.py:100), and the mutated line was printed before the run to show the arm applied.uv run python -c "from emrg.client.app import run_client"—import ok.Neither macOS nor Linux can exercise the Win32 call, so a local green says the guard runs, never that the overrun is gone. What confirms the fix is the
test-windowsleg of this PR's CI, withtest_bash_v2_win32.pyand the file that crashed after it in the same worker.