commit 6e78ede73f190f3aaf91623a131a20cf031aad7e Author: Alexander Færøy <ahf@torproject.org> Date: Wed Mar 8 01:47:12 2017 +0100 Remove buffered I/O stream usage in process_handle_t. This patch removes the buffered I/O stream usage in process_handle_t and its related utility functions. This simplifies the code and avoids racy code where we used buffered I/O on non-blocking file descriptors. See: https://bugs.torproject.org/21654 --- changes/bug21654 | 4 ++ src/common/util.c | 100 +++++++++++++++------------------------------- src/common/util.h | 11 ++--- src/test/test_pt.c | 6 +-- src/test/test_util_slow.c | 4 +- 5 files changed, 46 insertions(+), 79 deletions(-) diff --git a/changes/bug21654 b/changes/bug21654 new file mode 100644 index 0000000..fd1c650 --- /dev/null +++ b/changes/bug21654 @@ -0,0 +1,4 @@ + o Code simplifications and refactoring + - Use unbuffered I/O for utility functions around the process_handle_t + type. This fixes unit test failures reported on OpenBSD and FreeBSD. + Fixes bug 21654. diff --git a/src/common/util.c b/src/common/util.c index e2ad5ff..1fd3b16 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -4175,10 +4175,10 @@ tor_process_get_stdout_pipe(process_handle_t *process_handle) } #else /* DOCDOC tor_process_get_stdout_pipe */ -FILE * +int tor_process_get_stdout_pipe(process_handle_t *process_handle) { - return process_handle->stdout_handle; + return process_handle->stdout_pipe; } #endif @@ -4609,10 +4609,6 @@ tor_spawn_background(const char *const filename, const char **argv, log_warn(LD_GENERAL, "Failed to set stderror/stdout/stdin pipes " "nonblocking in parent process: %s", strerror(errno)); } - /* Open the buffered IO streams */ - process_handle->stdout_handle = fdopen(process_handle->stdout_pipe, "r"); - process_handle->stderr_handle = fdopen(process_handle->stderr_pipe, "r"); - process_handle->stdin_handle = fdopen(process_handle->stdin_pipe, "r"); *process_handle_out = process_handle; return process_handle->status; @@ -4659,14 +4655,9 @@ tor_process_handle_destroy,(process_handle_t *process_handle, if (process_handle->stdin_pipe) CloseHandle(process_handle->stdin_pipe); #else - if (process_handle->stdout_handle) - fclose(process_handle->stdout_handle); - - if (process_handle->stderr_handle) - fclose(process_handle->stderr_handle); - - if (process_handle->stdin_handle) - fclose(process_handle->stdin_handle); + close(process_handle->stdout_pipe); + close(process_handle->stderr_pipe); + close(process_handle->stdin_pipe); clear_waitpid_callback(process_handle->waitpid_cb); #endif @@ -4998,14 +4989,14 @@ tor_read_all_handle(HANDLE h, char *buf, size_t count, return (ssize_t)numread; } #else -/** Read from a handle <b>h</b> into <b>buf</b>, up to <b>count</b> bytes. If +/** Read from a handle <b>fd</b> into <b>buf</b>, up to <b>count</b> bytes. If * <b>process</b> is NULL, the function will return immediately if there is * nothing more to read. Otherwise data will be read until end of file, or * <b>count</b> bytes are read. Returns the number of bytes read, or -1 on * error. Sets <b>eof</b> to true if <b>eof</b> is not NULL and the end of the * file has been reached. */ ssize_t -tor_read_all_handle(FILE *h, char *buf, size_t count, +tor_read_all_handle(int fd, char *buf, size_t count, const process_handle_t *process, int *eof) { @@ -5019,7 +5010,7 @@ tor_read_all_handle(FILE *h, char *buf, size_t count, return -1; while (numread != count) { - result = read(fileno(h), buf+numread, count-numread); + result = read(fd, buf+numread, count-numread); if (result == 0) { log_debug(LD_GENERAL, "read() reached end of file"); @@ -5053,7 +5044,7 @@ tor_read_all_from_process_stdout(const process_handle_t *process_handle, return tor_read_all_handle(process_handle->stdout_pipe, buf, count, process_handle); #else - return tor_read_all_handle(process_handle->stdout_handle, buf, count, + return tor_read_all_handle(process_handle->stdout_pipe, buf, count, process_handle, NULL); #endif } @@ -5067,7 +5058,7 @@ tor_read_all_from_process_stderr(const process_handle_t *process_handle, return tor_read_all_handle(process_handle->stderr_pipe, buf, count, process_handle); #else - return tor_read_all_handle(process_handle->stderr_handle, buf, count, + return tor_read_all_handle(process_handle->stderr_pipe, buf, count, process_handle, NULL); #endif } @@ -5261,11 +5252,10 @@ log_from_handle(HANDLE *pipe, int severity) #else /** Return a smartlist containing lines outputted from - * <b>handle</b>. Return NULL on error, and set + * <b>fd</b>. Return NULL on error, and set * <b>stream_status_out</b> appropriately. */ MOCK_IMPL(smartlist_t *, -tor_get_lines_from_handle, (FILE *handle, - enum stream_status *stream_status_out)) +tor_get_lines_from_handle, (int fd, enum stream_status *stream_status_out)) { enum stream_status stream_status; char stdout_buf[400]; @@ -5274,7 +5264,7 @@ tor_get_lines_from_handle, (FILE *handle, while (1) { memset(stdout_buf, 0, sizeof(stdout_buf)); - stream_status = get_string_from_pipe(handle, + stream_status = get_string_from_pipe(fd, stdout_buf, sizeof(stdout_buf) - 1); if (stream_status != IO_STREAM_OKAY) goto done; @@ -5288,20 +5278,20 @@ tor_get_lines_from_handle, (FILE *handle, return lines; } -/** Read from stream, and send lines to log at the specified log level. +/** Read from fd, and send lines to log at the specified log level. * Returns 1 if stream is closed normally, -1 if there is a error reading, and * 0 otherwise. Handles lines from tor-fw-helper and * tor_spawn_background() specially. */ static int -log_from_pipe(FILE *stream, int severity, const char *executable, +log_from_pipe(int fd, int severity, const char *executable, int *child_status) { char buf[256]; enum stream_status r; for (;;) { - r = get_string_from_pipe(stream, buf, sizeof(buf) - 1); + r = get_string_from_pipe(fd, buf, sizeof(buf) - 1); if (r == IO_STREAM_CLOSED) { return 1; @@ -5326,7 +5316,7 @@ log_from_pipe(FILE *stream, int severity, const char *executable, } #endif -/** Reads from <b>stream</b> and stores input in <b>buf_out</b> making +/** Reads from <b>fd</b> and stores input in <b>buf_out</b> making * sure it's below <b>count</b> bytes. * If the string has a trailing newline, we strip it off. * @@ -5342,52 +5332,28 @@ log_from_pipe(FILE *stream, int severity, const char *executable, * IO_STREAM_OKAY: If everything went okay and we got a string * in <b>buf_out</b>. */ enum stream_status -get_string_from_pipe(FILE *stream, char *buf_out, size_t count) +get_string_from_pipe(int fd, char *buf_out, size_t count) { - char *retval; - size_t len; + ssize_t ret; tor_assert(count <= INT_MAX); - retval = tor_fgets(buf_out, (int)count, stream); - - if (!retval) { - if (feof(stream)) { - /* Program has closed stream (probably it exited) */ - /* TODO: check error */ - return IO_STREAM_CLOSED; - } else { - if (EAGAIN == errno) { - /* Nothing more to read, try again next time */ - return IO_STREAM_EAGAIN; - } else { - /* There was a problem, abandon this child process */ - return IO_STREAM_TERM; - } - } - } else { - len = strlen(buf_out); - if (len == 0) { - /* this probably means we got a NUL at the start of the string. */ - return IO_STREAM_EAGAIN; - } + ret = read(fd, buf_out, count); - if (buf_out[len - 1] == '\n') { - /* Remove the trailing newline */ - buf_out[len - 1] = '\0'; - } else { - /* No newline; check whether we overflowed the buffer */ - if (!feof(stream)) - log_info(LD_GENERAL, - "Line from stream was truncated: %s", buf_out); - /* TODO: What to do with this error? */ - } + if (ret == 0) + return IO_STREAM_CLOSED; + else if (ret < 0 && errno == EAGAIN) + return IO_STREAM_EAGAIN; + else if (ret < 0) + return IO_STREAM_TERM; - return IO_STREAM_OKAY; - } + if (buf_out[ret - 1] == '\n') { + /* Remove the trailing newline */ + buf_out[ret - 1] = '\0'; + } else + buf_out[ret] = '\0'; - /* We should never get here */ - return IO_STREAM_TERM; + return IO_STREAM_OKAY; } /** Parse a <b>line</b> from tor-fw-helper and issue an appropriate @@ -5624,7 +5590,7 @@ tor_check_port_forwarding(const char *filename, #ifdef _WIN32 stderr_status = log_from_handle(child_handle->stderr_pipe, LOG_INFO); #else - stderr_status = log_from_pipe(child_handle->stderr_handle, + stderr_status = log_from_pipe(child_handle->stderr_pipe, LOG_INFO, filename, &retval); #endif if (handle_fw_helper_output(filename, child_handle) < 0) { diff --git a/src/common/util.h b/src/common/util.h index 13fcc51..a8d7978 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -322,7 +322,7 @@ enum stream_status { const char *stream_status_to_string(enum stream_status stream_status); -enum stream_status get_string_from_pipe(FILE *stream, char *buf, size_t count); +enum stream_status get_string_from_pipe(int fd, char *buf, size_t count); MOCK_DECL(int,tor_unlink,(const char *pathname)); @@ -463,9 +463,6 @@ struct process_handle_t { int stdin_pipe; int stdout_pipe; int stderr_pipe; - FILE *stdin_handle; - FILE *stdout_handle; - FILE *stderr_handle; pid_t pid; /** If the process has not given us a SIGCHLD yet, this has the * waitpid_callback_t that gets invoked once it has. Otherwise this @@ -488,7 +485,7 @@ int tor_split_lines(struct smartlist_t *sl, char *buf, int len); ssize_t tor_read_all_handle(HANDLE h, char *buf, size_t count, const process_handle_t *process); #else -ssize_t tor_read_all_handle(FILE *h, char *buf, size_t count, +ssize_t tor_read_all_handle(int fd, char *buf, size_t count, const process_handle_t *process, int *eof); #endif @@ -502,7 +499,7 @@ int tor_process_get_pid(process_handle_t *process_handle); #ifdef _WIN32 HANDLE tor_process_get_stdout_pipe(process_handle_t *process_handle); #else -FILE *tor_process_get_stdout_pipe(process_handle_t *process_handle); +int tor_process_get_stdout_pipe(process_handle_t *process_handle); #endif #ifdef _WIN32 @@ -511,7 +508,7 @@ tor_get_lines_from_handle,(HANDLE *handle, enum stream_status *stream_status)); #else MOCK_DECL(struct smartlist_t *, -tor_get_lines_from_handle,(FILE *handle, +tor_get_lines_from_handle,(int fd, enum stream_status *stream_status)); #endif diff --git a/src/test/test_pt.c b/src/test/test_pt.c index f93019f..f50c75a 100644 --- a/src/test/test_pt.c +++ b/src/test/test_pt.c @@ -284,13 +284,13 @@ test_pt_get_extrainfo_string(void *arg) } #ifdef _WIN32 -#define STDIN_HANDLE HANDLE +#define STDIN_HANDLE HANDLE* #else -#define STDIN_HANDLE FILE +#define STDIN_HANDLE int #endif static smartlist_t * -tor_get_lines_from_handle_replacement(STDIN_HANDLE *handle, +tor_get_lines_from_handle_replacement(STDIN_HANDLE handle, enum stream_status *stream_status_out) { static int times_called = 0; diff --git a/src/test/test_util_slow.c b/src/test/test_util_slow.c index 1e71605..96a2fa7 100644 --- a/src/test/test_util_slow.c +++ b/src/test/test_util_slow.c @@ -242,7 +242,7 @@ test_util_spawn_background_partial_read_impl(int exit_early) #else /* Check that we didn't read the end of file last time */ tt_assert(!eof); - pos = tor_read_all_handle(process_handle->stdout_handle, stdout_buf, + pos = tor_read_all_handle(process_handle->stdout_pipe, stdout_buf, sizeof(stdout_buf) - 1, NULL, &eof); #endif log_info(LD_GENERAL, "tor_read_all_handle() returned %d", (int)pos); @@ -273,7 +273,7 @@ test_util_spawn_background_partial_read_impl(int exit_early) #else if (!eof) { /* We should have got all the data, but maybe not the EOF flag */ - pos = tor_read_all_handle(process_handle->stdout_handle, stdout_buf, + pos = tor_read_all_handle(process_handle->stdout_pipe, stdout_buf, sizeof(stdout_buf) - 1, process_handle, &eof); tt_int_op(0,OP_EQ, pos);