commit c6e041e3d8dcc6f887014f9dd8887faebf5f4a49 Author: Alexander Færøy ahf@torproject.org Date: Thu Dec 20 12:57:20 2018 +0100

Handle errors even after success from ReadFileEx() and WriteFileEx().

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 --- src/lib/process/process_win32.c | 50 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)

diff --git a/src/lib/process/process_win32.c b/src/lib/process/process_win32.c index 52acf4937..71dd4001c 100644 --- a/src/lib/process/process_win32.c +++ b/src/lib/process/process_win32.c @@ -329,6 +329,7 @@ process_win32_write(struct process_t *process, buf_t *buffer)

process_win32_t *win32_process = process_get_win32_process(process); BOOL ret = FALSE; + DWORD error_code = 0; const size_t buffer_size = buf_datalen(buffer);

/* Windows is still writing our buffer. */ @@ -350,6 +351,12 @@ process_win32_write(struct process_t *process, buf_t *buffer) /* Read data from the process_t buffer into our intermediate buffer. */ buf_get_bytes(buffer, win32_process->stdin_handle.buffer, write_size);

+ /* Because of the slightly weird API for WriteFileEx() we must set this to 0 + * before we call WriteFileEx() because WriteFileEx() does not reset the last + * error itself when it's succesful. See comment below after the call to + * GetLastError(). */ + SetLastError(0); + /* Schedule our write. */ ret = WriteFileEx(win32_process->stdin_handle.pipe, win32_process->stdin_handle.buffer, @@ -363,6 +370,24 @@ process_win32_write(struct process_t *process, buf_t *buffer) return 0; }

+ /* Here be dragons: According to MSDN's documentation for WriteFileEx() we + * 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. + * + * XXX(ahf): I have not managed to trigger this code while stress-testing + * this code. */ + error_code = GetLastError(); + + if (error_code != ERROR_SUCCESS) { + /* LCOV_EXCL_START */ + log_warn(LD_PROCESS, "WriteFileEx() failed after returning success: %s", + format_win32_error(error_code)); + win32_process->stdin_handle.reached_eof = true; + return 0; + /* LCOV_EXCL_STOP */ + } + /* This cast should be safe since our buffer can maximum be BUFFER_SIZE * large. */ return (int)write_size; @@ -864,6 +889,7 @@ process_win32_read_from_handle(process_win32_handle_t *handle,

BOOL ret = FALSE; int bytes_available = 0; + DWORD error_code = 0;

/* We already have a request to read data that isn't complete yet. */ if (BUG(handle->busy)) @@ -887,6 +913,12 @@ process_win32_read_from_handle(process_win32_handle_t *handle, memset(handle->buffer, 0, sizeof(handle->buffer)); }

+ /* Because of the slightly weird API for ReadFileEx() we must set this to 0 + * before we call ReadFileEx() because ReadFileEx() does not reset the last + * error itself when it's succesful. See comment below after the call to + * GetLastError(). */ + SetLastError(0); + /* Ask the Windows kernel to read data from our pipe into our buffer and call * the callback function when it is done. */ ret = ReadFileEx(handle->pipe, @@ -901,6 +933,24 @@ process_win32_read_from_handle(process_win32_handle_t *handle, return bytes_available; }

+ /* Here be dragons: According to MSDN's documentation for ReadFileEx() we + * should check GetLastError() after a call to ReadFileEx() even though the + * `ret` return value was successful. If everything is good, GetLastError() + * returns `ERROR_SUCCESS` and nothing happens. + * + * XXX(ahf): I have not managed to trigger this code while stress-testing + * this code. */ + error_code = GetLastError(); + + if (error_code != ERROR_SUCCESS) { + /* LCOV_EXCL_START */ + log_warn(LD_PROCESS, "ReadFileEx() failed after returning success: %s", + format_win32_error(error_code)); + handle->reached_eof = true; + return bytes_available; + /* LCOV_EXCL_STOP */ + } + /* We mark our handle as having a pending I/O request. */ handle->busy = true;