From 814a500e835a96bad6c620206e2276c4f92eaec7 Mon Sep 17 00:00:00 2001 From: Laytan Date: Tue, 6 May 2025 20:43:02 +0200 Subject: [PATCH 1/4] Windows was defaulting to the std handles of the current process, which is wrong --- core/os/os2/process_windows.odin | 38 +++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/core/os/os2/process_windows.odin b/core/os/os2/process_windows.odin index 536930ee1..6ac51be08 100644 --- a/core/os/os2/process_windows.odin +++ b/core/os/os2/process_windows.odin @@ -431,17 +431,43 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { } environment_block := _build_environment_block(environment, temp_allocator()) environment_block_w := win32_utf8_to_utf16(environment_block, temp_allocator()) or_return - stderr_handle := win32.GetStdHandle(win32.STD_ERROR_HANDLE) - stdout_handle := win32.GetStdHandle(win32.STD_OUTPUT_HANDLE) - stdin_handle := win32.GetStdHandle(win32.STD_INPUT_HANDLE) - if desc.stdout != nil { + stderr_handle: win32.HANDLE + stdout_handle: win32.HANDLE + stdin_handle: win32.HANDLE + + null_handle: win32.HANDLE + if desc.stdout != nil || desc.stderr != nil || desc.stdin != nil { + null_handle := win32.CreateFileW( + win32.L("NUL"), + win32.GENERIC_READ|win32.GENERIC_WRITE, + win32.FILE_SHARE_READ|win32.FILE_SHARE_WRITE, + &win32.SECURITY_ATTRIBUTES{ + nLength = size_of(win32.SECURITY_ATTRIBUTES), + bInheritHandle = true, + }, + win32.OPEN_EXISTING, + win32.FILE_ATTRIBUTE_NORMAL, + nil, + ) + assert(null_handle != nil) + } + + if desc.stdout == nil { + stdout_handle = null_handle + } else { stdout_handle = win32.HANDLE((^File_Impl)(desc.stdout.impl).fd) } - if desc.stderr != nil { + + if desc.stderr == nil { + stderr_handle = null_handle + } else { stderr_handle = win32.HANDLE((^File_Impl)(desc.stderr.impl).fd) } - if desc.stdin != nil { + + if desc.stdin == nil { + stdin_handle = null_handle + } else { stdin_handle = win32.HANDLE((^File_Impl)(desc.stdin.impl).fd) } From 2bce446d0808dbd09f90147408250ca6b35cb8c1 Mon Sep 17 00:00:00 2001 From: Laytan Date: Tue, 6 May 2025 20:47:51 +0200 Subject: [PATCH 2/4] ifs wrong way around --- core/os/os2/process_windows.odin | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/os/os2/process_windows.odin b/core/os/os2/process_windows.odin index 6ac51be08..f7a23eab8 100644 --- a/core/os/os2/process_windows.odin +++ b/core/os/os2/process_windows.odin @@ -437,7 +437,7 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { stdin_handle: win32.HANDLE null_handle: win32.HANDLE - if desc.stdout != nil || desc.stderr != nil || desc.stdin != nil { + if desc.stdout == nil || desc.stderr == nil || desc.stdin == nil { null_handle := win32.CreateFileW( win32.L("NUL"), win32.GENERIC_READ|win32.GENERIC_WRITE, From bf5206968a0c46cf9472eb8fa84b6d69be5ecad0 Mon Sep 17 00:00:00 2001 From: Laytan Date: Tue, 6 May 2025 20:57:26 +0200 Subject: [PATCH 3/4] close null_handle --- core/os/os2/process.odin | 15 +++------------ core/os/os2/process_linux.odin | 6 +++--- core/os/os2/process_posix.odin | 2 +- core/os/os2/process_wasi.odin | 2 +- core/os/os2/process_windows.odin | 23 ++++++++++++++++------- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index 98995981b..6e5a0a679 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -264,8 +264,10 @@ specific process, even after it has died. **Note(linux)**: The `handle` will be referring to pidfd. */ Process :: struct { - pid: int, + pid: int, handle: uintptr, + // Implementation specific state/data. + _impl: _Process, } Process_Open_Flags :: bit_set[Process_Open_Flag] @@ -290,21 +292,10 @@ process_open :: proc(pid: int, flags := Process_Open_Flags {}) -> (Process, Erro return _process_open(pid, flags) } - -/* -OS-specific process attributes. -*/ -Process_Attributes :: struct { - sys_attr: _Sys_Process_Attributes, -} - /* The description of how a process should be created. */ Process_Desc :: struct { - // OS-specific attributes. - sys_attr: Process_Attributes, - // The working directory of the process. If the string has length 0, the // working directory is assumed to be the current working directory of the // current process. diff --git a/core/os/os2/process_linux.odin b/core/os/os2/process_linux.odin index 6d654008b..52a75f548 100644 --- a/core/os/os2/process_linux.odin +++ b/core/os/os2/process_linux.odin @@ -362,6 +362,9 @@ _current_process_info :: proc(selection: Process_Info_Fields, allocator: runtime return _process_info_by_pid(get_pid(), selection, allocator) } +@(private="package") +_Process :: struct {} + @(private="package") _process_open :: proc(pid: int, _: Process_Open_Flags) -> (process: Process, err: Error) { process.pid = pid @@ -378,9 +381,6 @@ _process_open :: proc(pid: int, _: Process_Open_Flags) -> (process: Process, err return } -@(private="package") -_Sys_Process_Attributes :: struct {} - @(private="package") _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { TEMP_ALLOCATOR_GUARD() diff --git a/core/os/os2/process_posix.odin b/core/os/os2/process_posix.odin index cd451781f..ca1a8f211 100644 --- a/core/os/os2/process_posix.odin +++ b/core/os/os2/process_posix.odin @@ -46,7 +46,7 @@ _current_process_info :: proc(selection: Process_Info_Fields, allocator: runtime return _process_info_by_pid(_get_pid(), selection, allocator) } -_Sys_Process_Attributes :: struct {} +_Process :: struct {} _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { if len(desc.command) == 0 { diff --git a/core/os/os2/process_wasi.odin b/core/os/os2/process_wasi.odin index 6ebfe3788..1641e7766 100644 --- a/core/os/os2/process_wasi.odin +++ b/core/os/os2/process_wasi.odin @@ -44,7 +44,7 @@ _current_process_info :: proc(selection: Process_Info_Fields, allocator: runtime return } -_Sys_Process_Attributes :: struct {} +_Process :: struct {} _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { err = .Unsupported diff --git a/core/os/os2/process_windows.odin b/core/os/os2/process_windows.odin index f7a23eab8..3be12c9e9 100644 --- a/core/os/os2/process_windows.odin +++ b/core/os/os2/process_windows.odin @@ -418,7 +418,9 @@ _process_open :: proc(pid: int, flags: Process_Open_Flags) -> (process: Process, } @(private="package") -_Sys_Process_Attributes :: struct {} +_Process :: struct { + null_handle: win32.HANDLE, +} @(private="package") _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { @@ -436,9 +438,8 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { stdout_handle: win32.HANDLE stdin_handle: win32.HANDLE - null_handle: win32.HANDLE if desc.stdout == nil || desc.stderr == nil || desc.stdin == nil { - null_handle := win32.CreateFileW( + process._impl.null_handle = win32.CreateFileW( win32.L("NUL"), win32.GENERIC_READ|win32.GENERIC_WRITE, win32.FILE_SHARE_READ|win32.FILE_SHARE_WRITE, @@ -450,23 +451,23 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { win32.FILE_ATTRIBUTE_NORMAL, nil, ) - assert(null_handle != nil) + assert(process._impl.null_handle != nil) } if desc.stdout == nil { - stdout_handle = null_handle + stdout_handle = process._impl.null_handle } else { stdout_handle = win32.HANDLE((^File_Impl)(desc.stdout.impl).fd) } if desc.stderr == nil { - stderr_handle = null_handle + stderr_handle = process._impl.null_handle } else { stderr_handle = win32.HANDLE((^File_Impl)(desc.stderr.impl).fd) } if desc.stdin == nil { - stdin_handle = null_handle + stdin_handle = process._impl.null_handle } else { stdin_handle = win32.HANDLE((^File_Impl)(desc.stdin.impl).fd) } @@ -506,6 +507,10 @@ _process_wait :: proc(process: Process, timeout: time.Duration) -> (process_stat switch win32.WaitForSingleObject(handle, timeout_ms) { case win32.WAIT_OBJECT_0: + if process._impl.null_handle != nil { + win32.CloseHandle(process._impl.null_handle) + } + exit_code: u32 if !win32.GetExitCodeProcess(handle, &exit_code) { err =_get_platform_error() @@ -532,6 +537,10 @@ _process_wait :: proc(process: Process, timeout: time.Duration) -> (process_stat err = General_Error.Timeout return case: + if process._impl.null_handle != nil { + win32.CloseHandle(process._impl.null_handle) + } + err = _get_platform_error() return } From 9b218a29225cdc1fbe080f2eb900383ca494b1f9 Mon Sep 17 00:00:00 2001 From: laytan Date: Tue, 6 May 2025 19:42:52 +0200 Subject: [PATCH 4/4] don't need to hang on to the null handle --- core/os/os2/process.odin | 2 -- core/os/os2/process_linux.odin | 3 --- core/os/os2/process_posix.odin | 2 -- core/os/os2/process_wasi.odin | 2 -- core/os/os2/process_windows.odin | 30 ++++++++++++------------------ 5 files changed, 12 insertions(+), 27 deletions(-) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index 6e5a0a679..3c84f3539 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -266,8 +266,6 @@ specific process, even after it has died. Process :: struct { pid: int, handle: uintptr, - // Implementation specific state/data. - _impl: _Process, } Process_Open_Flags :: bit_set[Process_Open_Flag] diff --git a/core/os/os2/process_linux.odin b/core/os/os2/process_linux.odin index 52a75f548..8a23f0619 100644 --- a/core/os/os2/process_linux.odin +++ b/core/os/os2/process_linux.odin @@ -362,9 +362,6 @@ _current_process_info :: proc(selection: Process_Info_Fields, allocator: runtime return _process_info_by_pid(get_pid(), selection, allocator) } -@(private="package") -_Process :: struct {} - @(private="package") _process_open :: proc(pid: int, _: Process_Open_Flags) -> (process: Process, err: Error) { process.pid = pid diff --git a/core/os/os2/process_posix.odin b/core/os/os2/process_posix.odin index ca1a8f211..6070b19d6 100644 --- a/core/os/os2/process_posix.odin +++ b/core/os/os2/process_posix.odin @@ -46,8 +46,6 @@ _current_process_info :: proc(selection: Process_Info_Fields, allocator: runtime return _process_info_by_pid(_get_pid(), selection, allocator) } -_Process :: struct {} - _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { if len(desc.command) == 0 { err = .Invalid_Path diff --git a/core/os/os2/process_wasi.odin b/core/os/os2/process_wasi.odin index 1641e7766..9f4d61649 100644 --- a/core/os/os2/process_wasi.odin +++ b/core/os/os2/process_wasi.odin @@ -44,8 +44,6 @@ _current_process_info :: proc(selection: Process_Info_Fields, allocator: runtime return } -_Process :: struct {} - _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { err = .Unsupported return diff --git a/core/os/os2/process_windows.odin b/core/os/os2/process_windows.odin index 3be12c9e9..69764dff7 100644 --- a/core/os/os2/process_windows.odin +++ b/core/os/os2/process_windows.odin @@ -417,11 +417,6 @@ _process_open :: proc(pid: int, flags: Process_Open_Flags) -> (process: Process, return } -@(private="package") -_Process :: struct { - null_handle: win32.HANDLE, -} - @(private="package") _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { TEMP_ALLOCATOR_GUARD() @@ -438,8 +433,9 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { stdout_handle: win32.HANDLE stdin_handle: win32.HANDLE + null_handle: win32.HANDLE if desc.stdout == nil || desc.stderr == nil || desc.stdin == nil { - process._impl.null_handle = win32.CreateFileW( + null_handle = win32.CreateFileW( win32.L("NUL"), win32.GENERIC_READ|win32.GENERIC_WRITE, win32.FILE_SHARE_READ|win32.FILE_SHARE_WRITE, @@ -451,23 +447,29 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { win32.FILE_ATTRIBUTE_NORMAL, nil, ) - assert(process._impl.null_handle != nil) + // Opening NUL should always succeed. + assert(null_handle != nil) + } + // NOTE(laytan): I believe it is fine to close this handle right after CreateProcess, + // and we don't have to hold onto this until the process exits. + defer if null_handle != nil { + win32.CloseHandle(null_handle) } if desc.stdout == nil { - stdout_handle = process._impl.null_handle + stdout_handle = null_handle } else { stdout_handle = win32.HANDLE((^File_Impl)(desc.stdout.impl).fd) } if desc.stderr == nil { - stderr_handle = process._impl.null_handle + stderr_handle = null_handle } else { stderr_handle = win32.HANDLE((^File_Impl)(desc.stderr.impl).fd) } if desc.stdin == nil { - stdin_handle = process._impl.null_handle + stdin_handle = null_handle } else { stdin_handle = win32.HANDLE((^File_Impl)(desc.stdin.impl).fd) } @@ -507,10 +509,6 @@ _process_wait :: proc(process: Process, timeout: time.Duration) -> (process_stat switch win32.WaitForSingleObject(handle, timeout_ms) { case win32.WAIT_OBJECT_0: - if process._impl.null_handle != nil { - win32.CloseHandle(process._impl.null_handle) - } - exit_code: u32 if !win32.GetExitCodeProcess(handle, &exit_code) { err =_get_platform_error() @@ -537,10 +535,6 @@ _process_wait :: proc(process: Process, timeout: time.Duration) -> (process_stat err = General_Error.Timeout return case: - if process._impl.null_handle != nil { - win32.CloseHandle(process._impl.null_handle) - } - err = _get_platform_error() return }