Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std.child_process: opt-in to explicit annotate of all to be inherited handles to prevent leaks on Windows #14251

Open
matu3ba opened this issue Jan 9, 2023 · 0 comments
Labels
bug Observed behavior contradicts documented or intended behavior os-windows proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@matu3ba
Copy link
Contributor

matu3ba commented Jan 9, 2023

std Proposal and Context

This blog post explains how to do it https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873
The basic principle is to provide the CreateProcess method with a list of all handles: They must be regular ones and they must be set to inheritable.

In comparison Unixes like Linux leak the inheritable descriptors (those not marked with CLOEXEC), if another thread spawns processes at the same time. Another problem exists, if one does use CLOEXEC and one/external_code does not use exec*.

This would improve perf of multi-threaded code depending on no leaks, since there is no explicit mutex necessary.
Rust does not use this and uses a full process mutex rust-lang/rust#38227 to this day and has several other open handle-related problems related to it: rust-lang/rust#54760 rust-lang/rust#70719.

Note that trying out multiple executions along PATH and PATHEXT can increase the potential leaking time on Windows.

I started this, but got stuck on UpdateProcThreadAttribute failing with a bogous error message in https://github.com/matu3ba/zig/tree/extra_streams4.

The idea is to keep a buffer of 3 handles for stdin, stdout and stderr, but otherwise let the user provide one. The main caveats are that Windows may change the underlying attribute structure, as it requires dynamic probing of the attribute list size and usage of the setup function:

 LPPROC_THREAD_ATTRIBUTE_LIST lpAttributeList = NULL;
 fSuccess = cHandlesToInherit < 0xFFFFFFFF / sizeof(HANDLE) &&
            lpStartupInfo->cb == sizeof(*lpStartupInfo);
 if (!fSuccess) {
  SetLastError(ERROR_INVALID_PARAMETER);
 }
 if (fSuccess) {
  fSuccess = InitializeProcThreadAttributeList(NULL, 1, 0, &size) ||
             GetLastError() == ERROR_INSUFFICIENT_BUFFER;
 }
 if (fSuccess) {
  lpAttributeList = reinterpret_cast<LPPROC_THREAD_ATTRIBUTE_LIST>
                                (HeapAlloc(GetProcessHeap(), 0, size));
  fSuccess = lpAttributeList != NULL;
 }
 if (fSuccess) {
  fSuccess = InitializeProcThreadAttributeList(lpAttributeList,
                    1, 0, &size);
 }
 if (fSuccess) {
  fInitialized = TRUE;
  fSuccess = UpdateProcThreadAttribute(lpAttributeList,
                    0, PROC_THREAD_ATTRIBUTE_HANDLE_LIST,
                    rgHandlesToInherit,
                    cHandlesToInherit * sizeof(HANDLE), NULL, NULL);
 }

Expected Behavior

Default to no possible leaks on Windows, even if the user forgets to disable inheritance on handles.
UPDATE: Due to backwards incompatibility, since a user can not "overwrite enabling file handle inheritance as list for only this one to be spawned process" an opt-in sounds better.

Possible drawback: External code may use the non-explicit code, which can leak the handles if they are set to inheritable.
Related footgun: Handles must be set to inheritable, even for the explicit list method, to be inherited.

@matu3ba matu3ba added the bug Observed behavior contradicts documented or intended behavior label Jan 9, 2023
@matu3ba matu3ba changed the title std.childprocess: default to explicit annotation of all to be inherited handles to prevent leaks on Windows std.child_process: default to explicit annotation of all to be inherited handles to prevent leaks on Windows Jan 9, 2023
matu3ba added a commit to matu3ba/zig that referenced this issue Jan 17, 2023
This PR simplifies the setup code and handling of non-standard file
descriptors and handles for Windows and Posix with main focus on pipes.

Portable pipes default to pipe2 with CLOEXEC on Posix and disabled handle
inhertance on Windows except shortly before and after the creation of
the "child process".

Leaking of file desriptors on Posix
1. CLOEXEC does not take immediately effect with clone(), but after the
setup code for the child process was run and a exec* function is executed.
External code may at worst be long living and never do exec*.
2. File descriptors without CLOEXEC are designed to "leak to the child",
but every spawned process at the same time gets them as well.

Leaking of handles on Windows
1. File leaking on Windows can be fixed with an explicit list approach
as suggested in ziglang#14251, which might require runtime probing and
allocation of the necessary process setup list. Otherwise, it might
break on Kernel updates.
2. The potential time for leaking can be long due trying to spawn on
multiple PATH and PATHEXT entries on Windows.
@andrewrk andrewrk added standard library This issue involves writing Zig code for the standard library. os-windows proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. labels Jan 23, 2023
@andrewrk andrewrk added this to the 0.13.0 milestone Jan 23, 2023
andrewrk pushed a commit to matu3ba/zig that referenced this issue Jan 27, 2023
This PR simplifies the setup code and handling of non-standard file
descriptors and handles for Windows and Posix with main focus on pipes.

Portable pipes default to pipe2 with CLOEXEC on Posix and disabled handle
inhertance on Windows except shortly before and after the creation of
the "child process".

Leaking of file desriptors on Posix
1. CLOEXEC does not take immediately effect with clone(), but after the
setup code for the child process was run and a exec* function is executed.
External code may at worst be long living and never do exec*.
2. File descriptors without CLOEXEC are designed to "leak to the child",
but every spawned process at the same time gets them as well.

Leaking of handles on Windows
1. File leaking on Windows can be fixed with an explicit list approach
as suggested in ziglang#14251, which might require runtime probing and
allocation of the necessary process setup list. Otherwise, it might
break on Kernel updates.
2. The potential time for leaking can be long due trying to spawn on
multiple PATH and PATHEXT entries on Windows.
matu3ba added a commit to matu3ba/zig that referenced this issue Feb 4, 2023
This PR simplifies the setup code and handling of non-standard file
descriptors and handles for Windows and Posix with main focus on pipes.

Portable pipes default to pipe2 with CLOEXEC on Posix and disabled handle
inhertance on Windows except shortly before and after the creation of
the "child process".

Leaking of file desriptors on Posix
1. CLOEXEC does not take immediately effect with clone(), but after the
setup code for the child process was run and a exec* function is executed.
External code may at worst be long living and never do exec*.
2. File descriptors without CLOEXEC are designed to "leak to the child",
but every spawned process at the same time gets them as well.

Leaking of handles on Windows
1. File leaking on Windows can be fixed with an explicit list approach
as suggested in ziglang#14251, which might require runtime probing and
allocation of the necessary process setup list. Otherwise, it might
break on Kernel updates.
2. The potential time for leaking can be long due trying to spawn on
multiple PATH and PATHEXT entries on Windows.
matu3ba added a commit to matu3ba/zig that referenced this issue Feb 14, 2023
This PR simplifies the setup code and handling of non-standard file
descriptors and handles for Windows and Posix with main focus on pipes.

Portable pipes default to pipe2 with CLOEXEC on Posix and disabled handle
inhertance on Windows except shortly before and after the creation of
the "child process".

Leaking of file desriptors on Posix
1. CLOEXEC does not take immediately effect with clone(), but after the
setup code for the child process was run and a exec* function is executed.
External code may at worst be long living and never do exec*.
2. File descriptors without CLOEXEC are designed to "leak to the child",
but every spawned process at the same time gets them as well.

Leaking of handles on Windows
1. File leaking on Windows can be fixed with an explicit list approach
as suggested in ziglang#14251, which might require runtime probing and
allocation of the necessary process setup list. Otherwise, it might
break on Kernel updates.
2. The potential time for leaking can be long due trying to spawn on
multiple PATH and PATHEXT entries on Windows.
matu3ba added a commit to matu3ba/zig that referenced this issue Feb 21, 2023
This PR simplifies the setup code and handling of non-standard file
descriptors and handles for Windows and Posix with main focus on pipes.

Portable pipes default to pipe2 with CLOEXEC on Posix and disabled handle
inhertance on Windows except shortly before and after the creation of
the "child process".

Leaking of file desriptors on Posix
1. CLOEXEC does not take immediately effect with clone(), but after the
setup code for the child process was run and a exec* function is executed.
External code may at worst be long living and never do exec*.
2. File descriptors without CLOEXEC are designed to "leak to the child",
but every spawned process at the same time gets them as well.

Leaking of handles on Windows
1. File leaking on Windows can be fixed with an explicit list approach
as suggested in ziglang#14251, which might require runtime probing and
allocation of the necessary process setup list. Otherwise, it might
break on Kernel updates.
2. The potential time for leaking can be long due trying to spawn on
multiple PATH and PATHEXT entries on Windows.
matu3ba added a commit to matu3ba/zig that referenced this issue Apr 25, 2023
This PR simplifies the setup code and handling of non-standard file
descriptors and handles for Windows and Posix with main focus on pipes.

Portable pipes default to pipe2 with CLOEXEC on Posix and disabled handle
inhertance on Windows except shortly before and after the creation of
the "child process".

Leaking of file desriptors on Posix
1. CLOEXEC does not take immediately effect with clone(), but after the
setup code for the child process was run and a exec* function is executed.
External code may at worst be long living and never do exec*.
2. File descriptors without CLOEXEC are designed to "leak to the child",
but every spawned process at the same time gets them as well.

Leaking of handles on Windows
1. File leaking on Windows can be fixed with an explicit list approach
as suggested in ziglang#14251, which might require runtime probing and
allocation of the necessary process setup list. Otherwise, it might
break on Kernel updates.
2. The potential time for leaking can be long due trying to spawn on
multiple PATH and PATHEXT entries on Windows.
matu3ba added a commit to matu3ba/zig that referenced this issue Apr 28, 2023
This PR simplifies the setup code and handling of non-standard file
descriptors and handles for Windows and Posix with main focus on pipes.

Portable pipes default to pipe2 with CLOEXEC on Posix and disabled handle
inhertance on Windows except shortly before and after the creation of
the "child process".

Leaking of file desriptors on Posix
1. CLOEXEC does not take immediately effect with clone(), but after the
setup code for the child process was run and a exec* function is executed.
External code may at worst be long living and never do exec*.
2. File descriptors without CLOEXEC are designed to "leak to the child",
but every spawned process at the same time gets them as well.

Leaking of handles on Windows
1. File leaking on Windows can be fixed with an explicit list approach
as suggested in ziglang#14251, which might require runtime probing and
allocation of the necessary process setup list. Otherwise, it might
break on Kernel updates.
2. The potential time for leaking can be long due trying to spawn on
multiple PATH and PATHEXT entries on Windows.
@matu3ba matu3ba changed the title std.child_process: default to explicit annotation of all to be inherited handles to prevent leaks on Windows std.child_process: opt-in to explicit annotate of all to be inherited handles to prevent leaks on Windows Aug 21, 2023
matu3ba added a commit to matu3ba/zig that referenced this issue Jan 10, 2024
…LoadLibraryW

These are the non-controversial parts of implementing ziglang#14251 from
https://github.com/matu3ba/win32k-mitigation.

NOACCESS can occur, if the process is not permitted to access memory
provided to UpdateProcThreadAttribute.

DLL_INIT_FAILED occurs on first failure to load restricted libraries
with win32k mitigation with followup failing loads returning nonsensical
NOT_ENOUGH_MEMORY.
matu3ba added a commit to matu3ba/zig that referenced this issue Jan 10, 2024
…essW, LoadLibraryW

These are the non-controversial parts of implementing ziglang#14251 from
https://github.com/matu3ba/win32k-mitigation.

NOACCESS can occur, if the process is not permitted to access memory
provided to UpdateProcThreadAttribute.

DLL_INIT_FAILED occurs on first failure to load restricted libraries
with win32k mitigation with followup failing loads returning nonsensical
NOT_ENOUGH_MEMORY.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior os-windows proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

No branches or pull requests

2 participants