Merge pull request #4136 from flysand7/process-info-fix

[os2/process]: Don't free process info fields in partial success scenarios

core/os/os2/process.odin

@@ -167,14 +167,14 @@ Process_Info :: struct {
 	This procedure obtains an information, specified by `selection` parameter of
 	a process given by `pid`.
 
-	Use `free_process_info` to free the memory allocated by this procedure. In
+	Use `free_process_info` to free the memory allocated by this procedure. The
-	case the function returns an error it may only have been an error for one part
+	`free_process_info` procedure needs to be called, even if this procedure
-	of the information and you would still need to call it to free the other parts.
+	returned an error, as some of the fields may have been allocated.
 
 	**Note**: The resulting information may or may contain the fields specified
 	by the `selection` parameter. Always check whether the returned
 	`Process_Info` struct has the required fields before checking the error code
-	returned by this function.
+	returned by this procedure.
 */
 @(require_results)
 process_info_by_pid :: proc(pid: int, selection: Process_Info_Fields, allocator: runtime.Allocator) -> (Process_Info, Error) {
@@ -188,14 +188,14 @@ process_info_by_pid :: proc(pid: int, selection: Process_Info_Fields, allocator:
 	about a process that has been opened by the application, specified in
 	the `process` parameter.
 
-	Use `free_process_info` to free the memory allocated by this procedure. In
+	Use `free_process_info` to free the memory allocated by this procedure. The
-	case the function returns an error it may only have been an error for one part
+	`free_process_info` procedure needs to be called, even if this procedure
-	of the information and you would still need to call it to free the other parts.
+	returned an error, as some of the fields may have been allocated.
 
 	**Note**: The resulting information may or may contain the fields specified
 	by the `selection` parameter. Always check whether the returned
 	`Process_Info` struct has the required fields before checking the error code
-	returned by this function.
+	returned by this procedure.
 */
 @(require_results)
 process_info_by_handle :: proc(process: Process, selection: Process_Info_Fields, allocator: runtime.Allocator) -> (Process_Info, Error) {
@@ -208,14 +208,14 @@ process_info_by_handle :: proc(process: Process, selection: Process_Info_Fields,
 	This procedure obtains the information, specified by `selection` parameter
 	about the currently running process.
 
-	Use `free_process_info` to free the memory allocated by this procedure. In
+	Use `free_process_info` to free the memory allocated by this procedure. The
-	case the function returns an error it may only have been an error for one part
+	`free_process_info` procedure needs to be called, even if this procedure
-	of the information and you would still need to call it to free the other parts.
+	returned an error, as some of the fields may have been allocated.
 
 	**Note**: The resulting information may or may contain the fields specified
 	by the `selection` parameter. Always check whether the returned
 	`Process_Info` struct has the required fields before checking the error code
-	returned by this function.
+	returned by this procedure.
 */
 @(require_results)
 current_process_info :: proc(selection: Process_Info_Fields, allocator: runtime.Allocator) -> (Process_Info, Error) {

core/os/os2/process_windows.odin

@@ -93,34 +93,11 @@ read_memory_as_slice :: proc(h: win32.HANDLE, addr: rawptr, dest: []$T) -> (byte
 @(private="package")
 _process_info_by_pid :: proc(pid: int, selection: Process_Info_Fields, allocator: runtime.Allocator) -> (info: Process_Info, err: Error) {
 	info.pid = pid
-	defer if err != nil {
+	// Note(flysand): Open the process handle right away to prevent some race
-		free_process_info(info, allocator)
+	// conditions. Once the handle is open, the process will be kept alive by
-	}
+	// the OS.
-
-	// Data obtained from process snapshots
-	if selection >= {.PPid, .Priority} {
-		entry, entry_err := _process_entry_by_pid(info.pid)
-		if entry_err != nil {
-			err = General_Error.Not_Exist
-			return
-		}
-		if .PPid in selection {
-			info.fields += {.PPid}
-			info.ppid = int(entry.th32ParentProcessID)
-		}
-		if .Priority in selection {
-			info.fields += {.Priority}
-			info.priority = int(entry.pcPriClassBase)
-		}
-	}
-	if .Executable_Path in selection { // snap module
-		info.executable_path = _process_exe_by_pid(pid, allocator) or_return
-		info.fields += {.Executable_Path}
-	}
-
 	ph := win32.INVALID_HANDLE_VALUE
-
+	if selection >= {.Command_Line, .Environment, .Working_Dir, .Username} {
-	if selection >= {.Command_Line, .Environment, .Working_Dir, .Username} { // need process handle
 		ph = win32.OpenProcess(
 			win32.PROCESS_QUERY_LIMITED_INFORMATION | win32.PROCESS_VM_READ,
 			false,
@@ -134,84 +111,15 @@ _process_info_by_pid :: proc(pid: int, selection: Process_Info_Fields, allocator
 	defer if ph != win32.INVALID_HANDLE_VALUE {
 		win32.CloseHandle(ph)
 	}
-
+	snapshot_process: if selection >= {.PPid, .Priority} {
-	if selection >= {.Command_Line, .Environment, .Working_Dir} { // need peb
-		process_info_size: u32
-		process_info: win32.PROCESS_BASIC_INFORMATION
-		status := win32.NtQueryInformationProcess(ph, .ProcessBasicInformation, &process_info, size_of(process_info), &process_info_size)
-		if status != 0 {
-			// TODO(flysand): There's probably a mismatch between NTSTATUS and
-			// windows userland error codes, I haven't checked.
-			err = Platform_Error(status)
-			return
-		}
-		if process_info.PebBaseAddress == nil {
-			// Not sure what the error is
-			err = General_Error.Unsupported
-			return
-		}
-		process_peb: win32.PEB
-
-		_ = read_memory_as_struct(ph, process_info.PebBaseAddress, &process_peb) or_return
-
-		process_params: win32.RTL_USER_PROCESS_PARAMETERS
-		_ = read_memory_as_struct(ph, process_peb.ProcessParameters, &process_params) or_return
-
-		if selection >= {.Command_Line, .Command_Args} {
-			TEMP_ALLOCATOR_GUARD()
-			cmdline_w := make([]u16, process_params.CommandLine.Length, temp_allocator()) or_return
-			_ = read_memory_as_slice(ph, process_params.CommandLine.Buffer, cmdline_w) or_return
-
-			if .Command_Line in selection {
-				info.command_line = win32_utf16_to_utf8(cmdline_w, allocator) or_return
-				info.fields += {.Command_Line}
-			}
-			if .Command_Args in selection {
-				info.command_args = _parse_command_line(raw_data(cmdline_w), allocator) or_return
-				info.fields += {.Command_Args}
-			}
-		}
-		if .Environment in selection {
-			TEMP_ALLOCATOR_GUARD()
-			env_len := process_params.EnvironmentSize / 2
-			envs_w := make([]u16, env_len, temp_allocator()) or_return
-			_ = read_memory_as_slice(ph, process_params.Environment, envs_w) or_return
-
-			info.environment = _parse_environment_block(raw_data(envs_w), allocator) or_return
-			info.fields += {.Environment}
-		}
-		if .Working_Dir in selection {
-			TEMP_ALLOCATOR_GUARD()
-			cwd_w := make([]u16, process_params.CurrentDirectoryPath.Length, temp_allocator()) or_return
-			_ = read_memory_as_slice(ph, process_params.CurrentDirectoryPath.Buffer, cwd_w) or_return
-
-			info.working_dir = win32_utf16_to_utf8(cwd_w, allocator) or_return
-			info.fields += {.Working_Dir}
-		}
-	}
-
-	if .Username in selection {
-		info.username = _get_process_user(ph, allocator) or_return
-		info.fields += {.Username}
-	}
-	err = nil
-	return
-}
-
-@(private="package")
-_process_info_by_handle :: proc(process: Process, selection: Process_Info_Fields, allocator: runtime.Allocator) -> (info: Process_Info, err: Error) {
-	pid := process.pid
-	info.pid = pid
-	defer if err != nil {
-		free_process_info(info, allocator)
-	}
-
-	// Data obtained from process snapshots
-	if selection >= {.PPid, .Priority} { // snap process
 		entry, entry_err := _process_entry_by_pid(info.pid)
 		if entry_err != nil {
-			err = General_Error.Not_Exist
+			err = entry_err
-			return
+			if entry_err == General_Error.Not_Exist {
+				return
+			} else {
+				break snapshot_process
+			}
 		}
 		if .PPid in selection {
 			info.fields += {.PPid}
@@ -222,12 +130,18 @@ _process_info_by_handle :: proc(process: Process, selection: Process_Info_Fields
 			info.priority = int(entry.pcPriClassBase)
 		}
 	}
-	if .Executable_Path in selection { // snap module
+	snapshot_modules: if .Executable_Path in selection {
-		info.executable_path = _process_exe_by_pid(pid, allocator) or_return
+		exe_path: string
+		exe_path, err = _process_exe_by_pid(pid, allocator)
+		if _, ok := err.(runtime.Allocator_Error); ok {
+			return
+		} else if err != nil {
+			break snapshot_modules
+		}
+		info.executable_path = exe_path
 		info.fields += {.Executable_Path}
 	}
-	ph := win32.HANDLE(process.handle)
+	read_peb: if selection >= {.Command_Line, .Environment, .Working_Dir} {
-	if selection >= {.Command_Line, .Environment, .Working_Dir} { // need peb
 		process_info_size: u32
 		process_info: win32.PROCESS_BASIC_INFORMATION
 		status := win32.NtQueryInformationProcess(ph, .ProcessBasicInformation, &process_info, size_of(process_info), &process_info_size)
@@ -235,25 +149,26 @@ _process_info_by_handle :: proc(process: Process, selection: Process_Info_Fields
 			// TODO(flysand): There's probably a mismatch between NTSTATUS and
 			// windows userland error codes, I haven't checked.
 			err = Platform_Error(status)
-			return
+			break read_peb
 		}
-		if process_info.PebBaseAddress == nil {
+		assert(process_info.PebBaseAddress != nil)
-			// Not sure what the error is
-			err = General_Error.Unsupported
-			return
-		}
-
 		process_peb: win32.PEB
-		_ = read_memory_as_struct(ph, process_info.PebBaseAddress, &process_peb) or_return
+		_, err = read_memory_as_struct(ph, process_info.PebBaseAddress, &process_peb)
-
+		if err != nil {
+			break read_peb
+		}
 		process_params: win32.RTL_USER_PROCESS_PARAMETERS
-		_ = read_memory_as_struct(ph, process_peb.ProcessParameters, &process_params) or_return
+		_, err = read_memory_as_struct(ph, process_peb.ProcessParameters, &process_params)
-
+		if err != nil {
+			break read_peb
+		}
 		if selection >= {.Command_Line, .Command_Args} {
 			TEMP_ALLOCATOR_GUARD()
 			cmdline_w := make([]u16, process_params.CommandLine.Length, temp_allocator()) or_return
-			_ = read_memory_as_slice(ph, process_params.CommandLine.Buffer, cmdline_w) or_return
+			_, err = read_memory_as_slice(ph, process_params.CommandLine.Buffer, cmdline_w)
-
+			if err != nil {
+				break read_peb
+			}
 			if .Command_Line in selection {
 				info.command_line = win32_utf16_to_utf8(cmdline_w, allocator) or_return
 				info.fields += {.Command_Line}
@@ -263,28 +178,147 @@ _process_info_by_handle :: proc(process: Process, selection: Process_Info_Fields
 				info.fields += {.Command_Args}
 			}
 		}
-
 		if .Environment in selection {
 			TEMP_ALLOCATOR_GUARD()
 			env_len := process_params.EnvironmentSize / 2
 			envs_w := make([]u16, env_len, temp_allocator()) or_return
-			_ = read_memory_as_slice(ph, process_params.Environment, envs_w) or_return
+			_, err = read_memory_as_slice(ph, process_params.Environment, envs_w)
-
+			if err != nil {
-			info.environment =  _parse_environment_block(raw_data(envs_w), allocator) or_return
+				break read_peb
+			}
+			info.environment = _parse_environment_block(raw_data(envs_w), allocator) or_return
 			info.fields += {.Environment}
 		}
-
 		if .Working_Dir in selection {
 			TEMP_ALLOCATOR_GUARD()
 			cwd_w := make([]u16, process_params.CurrentDirectoryPath.Length, temp_allocator()) or_return
-			_ = read_memory_as_slice(ph, process_params.CurrentDirectoryPath.Buffer, cwd_w) or_return
+			_, err = read_memory_as_slice(ph, process_params.CurrentDirectoryPath.Buffer, cwd_w)
-
+			if err != nil {
+				break read_peb
+			}
 			info.working_dir = win32_utf16_to_utf8(cwd_w, allocator) or_return
 			info.fields += {.Working_Dir}
 		}
 	}
-	if .Username in selection {
+	read_username: if .Username in selection {
-		info.username = _get_process_user(ph, allocator) or_return
+		username: string
+		username, err = _get_process_user(ph, allocator)
+		if _, ok := err.(runtime.Allocator_Error); ok {
+			return
+		} else if err != nil {
+			break read_username
+		}
+		info.username = username
+		info.fields += {.Username}
+	}
+	err = nil
+	return
+}
+
+@(private="package")
+_process_info_by_handle :: proc(process: Process, selection: Process_Info_Fields, allocator: runtime.Allocator) -> (info: Process_Info, err: Error) {
+	pid := process.pid
+	info.pid = pid
+	// Data obtained from process snapshots
+	snapshot_process: if selection >= {.PPid, .Priority} {
+		entry, entry_err := _process_entry_by_pid(info.pid)
+		if entry_err != nil {
+			err = entry_err
+			if entry_err == General_Error.Not_Exist {
+				return
+			} else {
+				break snapshot_process
+			}
+		}
+		if .PPid in selection {
+			info.fields += {.PPid}
+			info.ppid = int(entry.th32ParentProcessID)
+		}
+		if .Priority in selection {
+			info.fields += {.Priority}
+			info.priority = int(entry.pcPriClassBase)
+		}
+	}
+	snapshot_module: if .Executable_Path in selection {
+		exe_path: string
+		exe_path, err = _process_exe_by_pid(pid, allocator)
+		if _, ok := err.(runtime.Allocator_Error); ok {
+			return
+		} else if err != nil {
+			break snapshot_module
+		}
+		info.executable_path = exe_path
+		info.fields += {.Executable_Path}
+	}
+	ph := win32.HANDLE(process.handle)
+	read_peb: if selection >= {.Command_Line, .Environment, .Working_Dir} {
+		process_info_size: u32
+		process_info: win32.PROCESS_BASIC_INFORMATION
+		status := win32.NtQueryInformationProcess(ph, .ProcessBasicInformation, &process_info, size_of(process_info), &process_info_size)
+		if status != 0 {
+			// TODO(flysand): There's probably a mismatch between NTSTATUS and
+			// windows userland error codes, I haven't checked.
+			err = Platform_Error(status)
+			return
+		}
+		assert(process_info.PebBaseAddress != nil)
+		process_peb: win32.PEB
+		_, err = read_memory_as_struct(ph, process_info.PebBaseAddress, &process_peb)
+		if err != nil {
+			break read_peb
+		}
+		process_params: win32.RTL_USER_PROCESS_PARAMETERS
+		_, err = read_memory_as_struct(ph, process_peb.ProcessParameters, &process_params)
+		if err != nil {
+			break read_peb
+		}
+		if selection >= {.Command_Line, .Command_Args} {
+			TEMP_ALLOCATOR_GUARD()
+			cmdline_w := make([]u16, process_params.CommandLine.Length, temp_allocator()) or_return
+			_, err = read_memory_as_slice(ph, process_params.CommandLine.Buffer, cmdline_w)
+			if err != nil {
+				break read_peb
+			}
+			if .Command_Line in selection {
+				info.command_line = win32_utf16_to_utf8(cmdline_w, allocator) or_return
+				info.fields += {.Command_Line}
+			}
+			if .Command_Args in selection {
+				info.command_args = _parse_command_line(raw_data(cmdline_w), allocator) or_return
+				info.fields += {.Command_Args}
+			}
+		}
+		if .Environment in selection {
+			TEMP_ALLOCATOR_GUARD()
+			env_len := process_params.EnvironmentSize / 2
+			envs_w := make([]u16, env_len, temp_allocator()) or_return
+			_, err = read_memory_as_slice(ph, process_params.Environment, envs_w)
+			if err != nil {
+				break read_peb
+			}
+			info.environment =  _parse_environment_block(raw_data(envs_w), allocator) or_return
+			info.fields += {.Environment}
+		}
+		if .Working_Dir in selection {
+			TEMP_ALLOCATOR_GUARD()
+			cwd_w := make([]u16, process_params.CurrentDirectoryPath.Length, temp_allocator()) or_return
+			_, err = read_memory_as_slice(ph, process_params.CurrentDirectoryPath.Buffer, cwd_w)
+			if err != nil {
+				break read_peb
+			}
+			info.working_dir = win32_utf16_to_utf8(cwd_w, allocator) or_return
+			info.fields += {.Working_Dir}
+		}
+	}
+	read_username: if .Username in selection {
+		username: string
+		username, err = _get_process_user(ph, allocator)
+		if _, ok := err.(runtime.Allocator_Error); ok {
+			return
+		} else if err != nil {
+			break read_username
+		}
+		info.username = username
 		info.fields += {.Username}
 	}
 	err = nil
@@ -294,15 +328,15 @@ _process_info_by_handle :: proc(process: Process, selection: Process_Info_Fields
 @(private="package")
 _current_process_info :: proc(selection: Process_Info_Fields, allocator: runtime.Allocator) -> (info: Process_Info, err: Error) {
 	info.pid = get_pid()
-	defer if err != nil {
+	snapshot_process: if selection >= {.PPid, .Priority} {
-		free_process_info(info, allocator)
-	}
-
-	if selection >= {.PPid, .Priority} { // snap process
 		entry, entry_err := _process_entry_by_pid(info.pid)
 		if entry_err != nil {
-			err = General_Error.Not_Exist
+			err = entry_err
-			return
+			if entry_err == General_Error.Not_Exist {
+				return
+			} else {
+				break snapshot_process
+			}
 		}
 		if .PPid in selection {
 			info.fields += {.PPid}
@@ -313,14 +347,16 @@ _current_process_info :: proc(selection: Process_Info_Fields, allocator: runtime
 			info.priority = int(entry.pcPriClassBase)
 		}
 	}
-	if .Executable_Path in selection {
+	module_filename: if .Executable_Path in selection {
 		exe_filename_w: [256]u16
 		path_len := win32.GetModuleFileNameW(nil, raw_data(exe_filename_w[:]), len(exe_filename_w))
+		assert(path_len > 0)
 		info.executable_path = win32_utf16_to_utf8(exe_filename_w[:path_len], allocator) or_return
 		info.fields += {.Executable_Path}
 	}
-	if selection >= {.Command_Line,  .Command_Args} {
+	command_line: if selection >= {.Command_Line,  .Command_Args} {
 		command_line_w := win32.GetCommandLineW()
+		assert(command_line_w != nil)
 		if .Command_Line in selection {
 			info.command_line = win32_wstring_to_utf8(command_line_w, allocator) or_return
 			info.fields += {.Command_Line}
@@ -330,14 +366,22 @@ _current_process_info :: proc(selection: Process_Info_Fields, allocator: runtime
 			info.fields += {.Command_Args}
 		}
 	}
-	if .Environment in selection {
+	read_environment: if .Environment in selection {
 		env_block := win32.GetEnvironmentStringsW()
+		assert(env_block != nil)
 		info.environment = _parse_environment_block(env_block, allocator) or_return
 		info.fields += {.Environment}
 	}
-	if .Username in selection {
+	read_username: if .Username in selection {
 		process_handle := win32.GetCurrentProcess()
-		info.username = _get_process_user(process_handle, allocator) or_return
+		username: string
+		username, err = _get_process_user(process_handle, allocator)
+		if _, ok := err.(runtime.Allocator_Error); ok {
+			return
+		} else if err != nil {
+			break read_username
+		}
+		info.username = username
 		info.fields += {.Username}
 	}
 	if .Working_Dir in selection {