Skip to content

Restrict Windows subprocess handle inheritance - #116

Merged
sheredom merged 5 commits into
mainfrom
fix/windows-handle-inheritance
Aug 14, 2026
Merged

Restrict Windows subprocess handle inheritance#116
sheredom merged 5 commits into
mainfrom
fix/windows-handle-inheritance

Conversation

@sheredom

Copy link
Copy Markdown
Owner

Summary

  • launch Windows children with STARTUPINFOEX and PROC_THREAD_ATTRIBUTE_HANDLE_LIST
  • whitelist only the stdin, stdout, and stderr handles intended for the new child
  • prevent concurrent subprocess_create calls from inheriting each other's temporarily-inheritable pipe handles
  • add a Windows regression test proving unrelated inheritable handles are excluded
  • release the process-thread attribute list on both success and failure

This is the Windows counterpart to the POSIX descriptor inheritance fix in #115, kept as a separate change.

Testing

  • ctest --test-dir build --output-on-failure
  • 4/4 test configurations passed on macOS; the Windows-specific regression test is skipped there

🧙 Conjured by AI via pi.dev using gpt-5.6-sol

Comment thread test/test.c Outdated
EXPECT_EQ_MSG(wait_timeout, wait_result,
"subprocess inherited a handle outside its standard streams");
#else
UTEST_SKIP("Windows handle-inheritance test");

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.

Could you not just skip the entire test rather than use an explicit skip on linux/macos?

🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol
🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol
🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol
🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol
🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol
@sheredom
sheredom force-pushed the fix/windows-handle-inheritance branch from 22c39cc to 3f8d2d8 Compare August 14, 2026 14:05
@sheredom
sheredom merged commit 0d76f78 into main Aug 14, 2026
24 checks passed
@sheredom
sheredom deleted the fix/windows-handle-inheritance branch August 14, 2026 15:28
sheredom pushed a commit that referenced this pull request Aug 18, 2026
* Declare the ProcThreadAttributeList APIs with pointer-width sizes

#116 added declarations for InitializeProcThreadAttributeList and
UpdateProcThreadAttribute using subprocess_size_t where the SDK has SIZE_T,
PSIZE_T and DWORD_PTR. Those are ULONG_PTR, which is unsigned long on Win32
while subprocess_size_t is unsigned int there. Same width, distinct types, so
the redeclaration itself is rejected before any call is reached:

    subprocess.h(461,1): error C2733: 'InitializeProcThreadAttributeList':
        you cannot overload a function with 'extern "C"' linkage
    subprocess.h(1157,3): error C2664: cannot convert argument 4 from
        'subprocess_size_t *' to 'PSIZE_T'

Win64 is unaffected because ULONG_PTR and subprocess_size_t agree there, which
is why cmake.yml does not see it — it builds Windows x64 only.

Added subprocess_ulongptr_t and used it for those parameters and for
attribute_list_size. It sits outside the _MSC_VER < 1920 split, or newer MSVC
would keep taking size_t from <inttypes.h> and stay broken.

Reproduced with MSVC 19.44 and SDK 10.0.26100 before the change and verified
after: -A Win32 goes from four errors to a clean build with both test binaries
passing, -A x64 still passes, and Linux glibc is unchanged at 442/442 in both
implementations.

* Derive the pointer-width type instead of naming __int64 again

clang-cl rejects the new typedef under -Weverything:

    subprocess.h(348,18): error : extension used
        [-Werror,-Wlanguage-extension-token]

The identical typedef a few lines above does not trip it because it sits
behind _MSC_VER < 1920, which clang-cl never compiles. Mine does not.

On _WIN64 subprocess_size_t is already 64 bits wide and so matches ULONG_PTR,
so reusing it avoids naming the extension token at all. Win32 keeps
unsigned long, which is what SIZE_T is there.

MSVC 19.44 builds and passes on both -A Win32 and -A x64. clang-cl is not
installed here, which is why CI found this and the local run did not.
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.

1 participant