Skip to content

emrg: size the security-descriptor buffer by the struct, not by 20 - #2510

Merged
argszero merged 1 commit into
masterfrom
feature/security-descriptor-size-of
Oct 23, 2026
Merged

argszero merged 1 commit into
masterfrom
feature/security-descriptor-size-of

Conversation

@argszero

Copy link
Copy Markdown
Owner

Closes #2509

SECURITY_DESCRIPTOR_MIN_LENGTH was written down as 20, and registry.apply_dacl used 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 as sizeof(SECURITY_DESCRIPTOR), and on a 64-bit interpreter that struct is 40 bytes, its Dacl pointer alone starting at offset 32. InitializeSecurityDescriptor therefore 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 loadfile gives one file per worker).

What changed

emrg/sandbox/win32/abi.py — declares SecurityDescriptor (the SDK's seven fields, in order) and derives the constant from it:

SECURITY_DESCRIPTOR_MIN_LENGTH = ctypes.sizeof(SecurityDescriptor)

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 (Dacl offset + 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.py is untouched — create_string_buffer(abi.SECURITY_DESCRIPTOR_MIN_LENGTH) now receives the right number. The module's other two create_string_buffer calls are sized from MakeSelfRelativeSD's and RegGetKeySecurity's own measurements, and acl.py's descriptors are allocated by the API; this was the only hand-sized one.

Verification

  • Guard, both directions: passes on the fix (constant: 40 | sizeof: 40); with = 20 put 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.
  • Local suite on this checkout with the fix: 5687 passed, 35 skipped in 235 s. 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-windows leg of this PR's CI, with test_bash_v2_win32.py and the file that crashed after it in the same worker.

@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-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 how2how2how2-arch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

✅ 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 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-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.

@argszero
argszero merged commit aea5ed9 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: SECURITY_DESCRIPTOR_MIN_LENGTH is the 32-bit size, so the absolute-descriptor buffer is overrun on 64-bit

2 participants