#28179 win32 robustness #608
Conversation
This prevents us from leaking the HANDLE for stdout, stderr, and stdin. See: https://bugs.torproject.org/28179
…re closed. This patch makes us delay checking for whether we have an exit code value (via GetExitCodeProcess()) until both stdout and stderr have been closed by the operating system either by the process itself or by process cleanup after termination. See: https://bugs.torproject.org/28179
This patch adds some additional error checking after calls to ReadFileEx() and WriteFileEx(). I have not managed to get this code to reach the branch where `error_code` is NOT `ERROR_SUCCESS`, but MSDN says one should check for this condition so we do so just to be safe. See: https://bugs.torproject.org/28179
This patch adds some missing calls to set `reached_eof` of our handles when various error conditions happens or when we close our handle (which happens at `process_terminate()`. See: https://bugs.torproject.org/28179
Handle `ERROR_BROKEN_PIPE` from ReadFileEx() and WriteFileEx() in process_win32_stdin_write_done() and process_win32_handle_read_completion() instead of in the early handler. This most importantmly makes sure that `reached_eof` is set to true when these errors appears. See: https://bugs.torproject.org/28179
This patch changes the CancelIoEx() example code to use CancelIo(), which is available for older versions of Windows too. I still think the kernel handles this nicely by sending broken pipes if either side closes the pipe while I/O operations are pending. See: https://bugs.torproject.org/28179
Let's not use log_warn() when a pipe is closed under what should be considered normal conditions. See: https://bugs.torproject.org/28179
Pull Request Test Coverage Report for Build 3351
|
Branch looks okay -- just added one question.
| * something to write and is thus usually not awaiting to finish any | ||
| * operations. If we WriteFileEx() to a file that has terminated we'll simply | ||
| * get an error from ReadFileEx() or its completion routine and move on with | ||
| * life. */ |
Is there any possibility that these checks might prevent a dead process from being freed forever? I'm guessing no, but I feel like I should ask.
I haven't been able to see that happen: even with processes that don't initialize properly (missing a .DLL for example) it seems like the console handles gets closed properly upon termination. I wanted to document this though such that it makes more sense for people looking at the code in the future.
| * should check GetLastError() after a call to WriteFileEx() even though the | ||
| * `ret` return value was successful. If everything is good, GetLastError() | ||
| * returns `ERROR_SUCCESS` and nothing happens. | ||
| * |
Wow, what a horrible API they have! But I guess we can't do anything about that. (I wonder if it is intentional, or whether they had a bug and just documented it as the official behavior.)
I agree. In the documentation they mostly talk about buffer size issues, but I think that is only happening if you do I/O where you have multiple async I/O calls out at the same time that works on the same overlapped structure.
I tried to run the test cases for process_t related code in a VM with limited amount of memory and where I also ran a "load heavy" task (compiled Tor with -jN in a while true loop) and made some discoveries:
ReadFileEx(). We now wait until we have received EOF state for both standard out and error before we check whether the process have terminated.reached_eofto true under certain pathological conditions. Together with (1) this could sometimes lead to an error in the slow tests where the process was terminated before all output from the child process had been received by Tor's event loop.To run a test in PowerShell until it fails one can do:
while ($true) { .\test-slow.exe slow/process/... ; if ($LastExitCode -ne 0) { break : } }The text was updated successfully, but these errors were encountered: