From 86de065aee642585e092969c69681f7e8847a648 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20F=C3=A6r=C3=B8y?= Date: Wed, 8 Mar 2017 00:12:06 +0100 Subject: [PATCH 1/5] Use read(2) instead of fgets(3) when reading process output. This patch modifies `tor_read_all_handle()` to use read(2) instead of fgets(3) when reading the stdout from the child process. This should eliminate the race condition that can be triggered in the 'slow/util/*' tests on slower machines running OpenBSD, FreeBSD and HardenedBSD. See: https://bugs.torproject.org/21654 --- src/common/util.c | 42 ++++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index a69d887e30..e2ad5ffa48 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -5010,7 +5010,7 @@ tor_read_all_handle(FILE *h, char *buf, size_t count, int *eof) { size_t numread = 0; - char *retval; + ssize_t result; if (eof) *eof = 0; @@ -5019,33 +5019,27 @@ tor_read_all_handle(FILE *h, char *buf, size_t count, return -1; while (numread != count) { - /* Use fgets because that is what we use in log_from_pipe() */ - retval = tor_fgets(buf+numread, (int)(count-numread), h); - if (NULL == retval) { - if (feof(h)) { - log_debug(LD_GENERAL, "fgets() reached end of file"); - if (eof) - *eof = 1; + result = read(fileno(h), buf+numread, count-numread); + + if (result == 0) { + log_debug(LD_GENERAL, "read() reached end of file"); + if (eof) + *eof = 1; + break; + } else if (result < 0 && errno == EAGAIN) { + if (process) + continue; + else break; - } else { - if (EAGAIN == errno) { - if (process) - continue; - else - break; - } else { - log_warn(LD_GENERAL, "fgets() from handle failed: %s", - strerror(errno)); - return -1; - } - } + } else if (result < 0) { + log_warn(LD_GENERAL, "read() failed: %s", strerror(errno)); + return -1; } - tor_assert(retval != NULL); - tor_assert(strlen(retval) + numread <= count); - numread += strlen(retval); + + numread += result; } - log_debug(LD_GENERAL, "fgets() read %d bytes from handle", (int)numread); + log_debug(LD_GENERAL, "read() read %d bytes from handle", (int)numread); return (ssize_t)numread; } #endif From 6e78ede73f190f3aaf91623a131a20cf031aad7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20F=C3=A6r=C3=B8y?= Date: Wed, 8 Mar 2017 01:47:12 +0100 Subject: [PATCH 2/5] 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(-) create mode 100644 changes/bug21654 diff --git a/changes/bug21654 b/changes/bug21654 new file mode 100644 index 0000000000..fd1c650710 --- /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 e2ad5ffa48..1fd3b16d72 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 h into buf, up to count bytes. If +/** Read from a handle fd into buf, up to count bytes. If * process is NULL, the function will return immediately if there is * nothing more to read. Otherwise data will be read until end of file, or * count bytes are read. Returns the number of bytes read, or -1 on * error. Sets eof to true if eof 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 - * handle. Return NULL on error, and set + * fd. Return NULL on error, and set * stream_status_out 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 stream and stores input in buf_out making +/** Reads from fd and stores input in buf_out making * sure it's below count 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 buf_out. */ 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); + ret = read(fd, buf_out, count); - 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; - } + 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; - 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 (buf_out[ret - 1] == '\n') { + /* Remove the trailing newline */ + buf_out[ret - 1] = '\0'; + } else + buf_out[ret] = '\0'; - return IO_STREAM_OKAY; - } - - /* We should never get here */ - return IO_STREAM_TERM; + return IO_STREAM_OKAY; } /** Parse a line 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 13fcc5142d..a8d7978c24 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 f93019f1c4..f50c75ab48 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 1e7160598c..96a2fa756b 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); From 0e5c7dc45b9a70ba44dd9a3dc0c3cb38e05e019d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20F=C3=A6r=C3=B8y?= Date: Wed, 8 Mar 2017 18:08:02 +0100 Subject: [PATCH 3/5] Add test case for get_string_from_pipe(). This patch adds a test case for the get_string_from_pipe() function found in the utility module. See: See: https://bugs.torproject.org/21654 --- src/test/test_util.c | 119 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 118 insertions(+), 1 deletion(-) diff --git a/src/test/test_util.c b/src/test/test_util.c index 3e4d45d35b..640935a662 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -4049,7 +4049,123 @@ test_util_fgets_eagain(void *ptr) if (test_pipe[1] != -1) close(test_pipe[1]); } -#endif + +static void +test_util_string_from_pipe(void *ptr) +{ + int test_pipe[2] = {-1, -1}; + int retval = 0; + enum stream_status status = IO_STREAM_TERM; + ssize_t retlen; + char buf[4] = { 0 }; + + (void)ptr; + + errno = 0; + + /* Set up a pipe to test on */ + retval = pipe(test_pipe); + tt_int_op(retval, OP_EQ, 0); + + /* Send in a string. */ + retlen = write(test_pipe[1], "ABC", 3); + tt_int_op(retlen, OP_EQ, 3); + + status = get_string_from_pipe(test_pipe[0], buf, sizeof(buf)-1); + tt_int_op(errno, OP_EQ, 0); + tt_int_op(status, OP_EQ, IO_STREAM_OKAY); + tt_str_op(buf, OP_EQ, "ABC"); + errno = 0; + + /* Send in a string that contains a nul. */ + retlen = write(test_pipe[1], "AB\0", 3); + tt_int_op(retlen, OP_EQ, 3); + + status = get_string_from_pipe(test_pipe[0], buf, sizeof(buf)-1); + tt_int_op(errno, OP_EQ, 0); + tt_int_op(status, OP_EQ, IO_STREAM_OKAY); + tt_str_op(buf, OP_EQ, "AB"); + errno = 0; + + /* Send in a string that contains a nul only. */ + retlen = write(test_pipe[1], "\0", 1); + tt_int_op(retlen, OP_EQ, 1); + + status = get_string_from_pipe(test_pipe[0], buf, sizeof(buf)-1); + tt_int_op(errno, OP_EQ, 0); + tt_int_op(status, OP_EQ, IO_STREAM_OKAY); + tt_str_op(buf, OP_EQ, ""); + errno = 0; + + /* Send in a string that contains a trailing newline. */ + retlen = write(test_pipe[1], "AB\n", 3); + tt_int_op(retlen, OP_EQ, 3); + + status = get_string_from_pipe(test_pipe[0], buf, sizeof(buf)-1); + tt_int_op(errno, OP_EQ, 0); + tt_int_op(status, OP_EQ, IO_STREAM_OKAY); + tt_str_op(buf, OP_EQ, "AB"); + errno = 0; + + /* Send in a string that contains a newline only. */ + retlen = write(test_pipe[1], "\n", 1); + tt_int_op(retlen, OP_EQ, 1); + + status = get_string_from_pipe(test_pipe[0], buf, sizeof(buf)-1); + tt_int_op(errno, OP_EQ, 0); + tt_int_op(status, OP_EQ, IO_STREAM_OKAY); + tt_str_op(buf, OP_EQ, ""); + errno = 0; + + /* Send in a string and check that we nul terminate return values. */ + retlen = write(test_pipe[1], "AAA", 3); + tt_int_op(retlen, OP_EQ, 3); + + status = get_string_from_pipe(test_pipe[0], buf, sizeof(buf)-1); + tt_int_op(errno, OP_EQ, 0); + tt_int_op(status, OP_EQ, IO_STREAM_OKAY); + tt_str_op(buf, OP_EQ, "AAA"); + tt_mem_op(buf, OP_EQ, "AAA\0", sizeof(buf)); + errno = 0; + + retlen = write(test_pipe[1], "B", 1); + tt_int_op(retlen, OP_EQ, 1); + + memset(buf, '\xff', sizeof(buf)); + status = get_string_from_pipe(test_pipe[0], buf, sizeof(buf)-1); + tt_int_op(errno, OP_EQ, 0); + tt_int_op(status, OP_EQ, IO_STREAM_OKAY); + tt_str_op(buf, OP_EQ, "B"); + tt_mem_op(buf, OP_EQ, "B\0\xff\xff", sizeof(buf)); + errno = 0; + + /* Send in a line and close */ + retlen = write(test_pipe[1], "AB", 2); + tt_int_op(retlen, OP_EQ, 2); + retval = close(test_pipe[1]); + tt_int_op(retval, OP_EQ, 0); + test_pipe[1] = -1; + + status = get_string_from_pipe(test_pipe[0], buf, sizeof(buf)-1); + tt_int_op(errno, OP_EQ, 0); + tt_int_op(status, OP_EQ, IO_STREAM_OKAY); + tt_str_op(buf, OP_EQ, "AB"); + errno = 0; + + /* Check for EOF */ + status = get_string_from_pipe(test_pipe[0], buf, sizeof(buf)-1); + tt_int_op(errno, OP_EQ, 0); + tt_int_op(status, OP_EQ, IO_STREAM_CLOSED); + errno = 0; + + done: + if (test_pipe[0] != -1) + close(test_pipe[0]); + if (test_pipe[1] != -1) + close(test_pipe[1]); +} + +#endif // _WIN32 /** * Test for format_hex_number_sigsafe() @@ -5724,6 +5840,7 @@ struct testcase_t util_tests[] = { UTIL_TEST_WIN_ONLY(load_win_lib, 0), UTIL_TEST_NO_WIN(exit_status, 0), UTIL_TEST_NO_WIN(fgets_eagain, 0), + UTIL_TEST_NO_WIN(string_from_pipe, 0), UTIL_TEST(format_hex_number, 0), UTIL_TEST(format_dec_number, 0), UTIL_TEST(join_win_cmdline, 0), From 02ef06516e64c1559b24123d7c7d164b76110c9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20F=C3=A6r=C3=B8y?= Date: Wed, 8 Mar 2017 23:02:34 +0100 Subject: [PATCH 4/5] Use less-than instead of not-equal-to for comparison in read loops. This patch changes a number of read loops in the util module to use less-than comparison instead of not-equal-to comparison. We do this in the case that we have a bug elsewhere that might cause `numread` to become larger than `count` and thus become an infinite loop. --- src/common/util.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 1fd3b16d72..a8b44cbda7 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2118,7 +2118,7 @@ read_all(tor_socket_t fd, char *buf, size_t count, int isSocket) return -1; } - while (numread != count) { + while (numread < count) { if (isSocket) result = tor_socket_recv(fd, buf+numread, count-numread, 0); else @@ -4943,7 +4943,7 @@ tor_read_all_handle(HANDLE h, char *buf, size_t count, if (count > SIZE_T_CEILING || count > SSIZE_MAX) return -1; - while (numread != count) { + while (numread < count) { /* Check if there is anything to read */ retval = PeekNamedPipe(h, NULL, 0, NULL, &byte_count, NULL); if (!retval) { @@ -5009,7 +5009,7 @@ tor_read_all_handle(int fd, char *buf, size_t count, if (count > SIZE_T_CEILING || count > SSIZE_MAX) return -1; - while (numread != count) { + while (numread < count) { result = read(fd, buf+numread, count-numread); if (result == 0) { From 02fc0a5ecfc917e9261596816e926468db976453 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20F=C3=A6r=C3=B8y?= Date: Wed, 8 Mar 2017 23:11:42 +0100 Subject: [PATCH 5/5] Remove fgets() compatbility function and related tests. This patch removes the `tor_fgets()` wrapper around `fgets(3)` since it is no longer needed. The function was created due to inconsistency between the returned values of `fgets(3)` on different versions of Unix when using `fgets(3)` on non-blocking file descriptors, but with the recent changes in bug #21654 we switch from unbuffered to direct I/O on non-blocking file descriptors in our utility module. We continue to use `fgets(3)` directly in the geoip and dirserv module since this usage is considered safe. This patch also removes the test-case that was created to detect differences in the implementation of `fgets(3)` as well as the changes file since these changes was not included in any releases yet. See: https://bugs.torproject.org/21654 --- changes/bug20988 | 4 -- src/common/compat.c | 39 --------------- src/common/compat.h | 2 - src/or/dirserv.c | 4 +- src/or/geoip.c | 2 +- src/test/test_util.c | 111 ------------------------------------------- 6 files changed, 3 insertions(+), 159 deletions(-) delete mode 100644 changes/bug20988 diff --git a/changes/bug20988 b/changes/bug20988 deleted file mode 100644 index b1d73f08e7..0000000000 --- a/changes/bug20988 +++ /dev/null @@ -1,4 +0,0 @@ - o Minor bugfixes (portability): - - Add Tor compatibility function for fgets(3) due to inconsistency of - returned values in different supported C libraries. This fixes unit test - failures reported on FreeBSD. Fixes bug 20988. diff --git a/src/common/compat.c b/src/common/compat.c index da4283fbaa..0dbede6bed 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -3476,45 +3476,6 @@ tor_getpass(const char *prompt, char *output, size_t buflen) #endif } -/** Read at most one less than the number of characters specified by - * size from the given stream and store it in str. - * - * Reading stops when a newline character is found or at EOF or error. If any - * characters are read and there's no error, a trailing NUL byte is appended to - * the end of str. - * - * Upon successful completion, this function returns a pointer to the string - * str. If EOF occurs before any characters are read the function will - * return NULL and the content of str is unchanged. Upon error, the - * function returns NULL and the caller must check for error using foef(3) and - * ferror(3). - */ -char * -tor_fgets(char *str, int size, FILE *stream) -{ - char *ret; - - /* Reset errno before our call to fgets(3) to avoid a situation where the - * caller is calling us again because we just returned NULL and errno == - * EAGAIN, but when they call us again we will always return NULL because the - * error flag on the file handler remains set and errno is set to EAGAIN. - */ - errno = 0; - - ret = fgets(str, size, stream); - - /* FreeBSD, OpenBSD, Linux (glibc), and Linux (musl) seem to disagree about - * what to do in the given situation. We check if the stream has been flagged - * with an error-bit and return NULL in that situation if errno is also set - * to EAGAIN. - */ - if (ferror(stream) && errno == EAGAIN) { - return NULL; - } - - return ret; -} - /** Return the amount of free disk space we have permission to use, in * bytes. Return -1 if the amount of free space can't be determined. */ int64_t diff --git a/src/common/compat.h b/src/common/compat.h index 1f51ece61f..ee1c9454de 100644 --- a/src/common/compat.h +++ b/src/common/compat.h @@ -740,8 +740,6 @@ STATIC int tor_ersatz_socketpair(int family, int type, int protocol, ssize_t tor_getpass(const char *prompt, char *output, size_t buflen); -char *tor_fgets(char *str, int size, FILE *stream); - /* This needs some of the declarations above so we include it here. */ #include "compat_threads.h" diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 2c3ce100df..f01668adcb 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -2701,7 +2701,7 @@ dirserv_read_measured_bandwidths(const char *from_file, return -1; } - if (!tor_fgets(line, sizeof(line), fp) + if (!fgets(line, sizeof(line), fp) || !strlen(line) || line[strlen(line)-1] != '\n') { log_warn(LD_DIRSERV, "Long or truncated time in bandwidth file: %s", escaped(line)); @@ -2731,7 +2731,7 @@ dirserv_read_measured_bandwidths(const char *from_file, while (!feof(fp)) { measured_bw_line_t parsed_line; - if (tor_fgets(line, sizeof(line), fp) && strlen(line)) { + if (fgets(line, sizeof(line), fp) && strlen(line)) { if (measured_bw_line_parse(&parsed_line, line) != -1) { /* Also cache the line for dirserv_get_bandwidth_for_router() */ dirserv_cache_measured_bw(&parsed_line, file_time); diff --git a/src/or/geoip.c b/src/or/geoip.c index a8dc807c19..74811ea643 100644 --- a/src/or/geoip.c +++ b/src/or/geoip.c @@ -346,7 +346,7 @@ geoip_load_file(sa_family_t family, const char *filename) (family == AF_INET) ? "IPv4" : "IPv6", filename); while (!feof(f)) { char buf[512]; - if (tor_fgets(buf, (int)sizeof(buf), f) == NULL) + if (fgets(buf, (int)sizeof(buf), f) == NULL) break; crypto_digest_add_bytes(geoip_digest_env, buf, strlen(buf)); /* FFFF track full country name. */ diff --git a/src/test/test_util.c b/src/test/test_util.c index 640935a662..130a019d5c 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -3940,116 +3940,6 @@ test_util_exit_status(void *ptr) #endif #ifndef _WIN32 -/* Check that fgets with a non-blocking pipe returns partial lines and sets - * EAGAIN, returns full lines and sets no error, and returns NULL on EOF and - * sets no error */ -static void -test_util_fgets_eagain(void *ptr) -{ - int test_pipe[2] = {-1, -1}; - int retval; - ssize_t retlen; - char *retptr; - FILE *test_stream = NULL; - char buf[4] = { 0 }; - - (void)ptr; - - errno = 0; - - /* Set up a pipe to test on */ - retval = pipe(test_pipe); - tt_int_op(retval, OP_EQ, 0); - - /* Set up the read-end to be non-blocking */ - retval = fcntl(test_pipe[0], F_SETFL, O_NONBLOCK); - tt_int_op(retval, OP_EQ, 0); - - /* Open it as a stdio stream */ - test_stream = fdopen(test_pipe[0], "r"); - tt_ptr_op(test_stream, OP_NE, NULL); - - /* Send in a partial line */ - retlen = write(test_pipe[1], "A", 1); - tt_int_op(retlen, OP_EQ, 1); - retptr = tor_fgets(buf, sizeof(buf), test_stream); - tt_int_op(errno, OP_EQ, EAGAIN); - tt_ptr_op(retptr, OP_EQ, NULL); - tt_str_op(buf, OP_EQ, "A"); - errno = 0; - - /* Send in the rest */ - retlen = write(test_pipe[1], "B\n", 2); - tt_int_op(retlen, OP_EQ, 2); - retptr = tor_fgets(buf, sizeof(buf), test_stream); - tt_int_op(errno, OP_EQ, 0); - tt_ptr_op(retptr, OP_EQ, buf); - tt_str_op(buf, OP_EQ, "B\n"); - errno = 0; - memset(buf, '\0', sizeof(buf)); - - /* Send in a full line */ - retlen = write(test_pipe[1], "CD\n", 3); - tt_int_op(retlen, OP_EQ, 3); - retptr = tor_fgets(buf, sizeof(buf), test_stream); - tt_int_op(errno, OP_EQ, 0); - tt_ptr_op(retptr, OP_EQ, buf); - tt_str_op(buf, OP_EQ, "CD\n"); - errno = 0; - memset(buf, '\0', sizeof(buf)); - - /* Send in a partial line */ - retlen = write(test_pipe[1], "E", 1); - tt_int_op(retlen, OP_EQ, 1); - retptr = tor_fgets(buf, sizeof(buf), test_stream); - tt_int_op(errno, OP_EQ, EAGAIN); - tt_ptr_op(retptr, OP_EQ, NULL); - tt_str_op(buf, OP_EQ, "E"); - errno = 0; - - /* Send in the rest */ - retlen = write(test_pipe[1], "F\n", 2); - tt_int_op(retlen, OP_EQ, 2); - retptr = tor_fgets(buf, sizeof(buf), test_stream); - tt_int_op(errno, OP_EQ, 0); - tt_ptr_op(retptr, OP_EQ, buf); - tt_str_op(buf, OP_EQ, "F\n"); - errno = 0; - memset(buf, '\0', sizeof(buf)); - - /* Send in a full line and close */ - retlen = write(test_pipe[1], "GH", 2); - tt_int_op(retlen, OP_EQ, 2); - retval = close(test_pipe[1]); - tt_int_op(retval, OP_EQ, 0); - test_pipe[1] = -1; - retptr = tor_fgets(buf, sizeof(buf), test_stream); - tt_int_op(errno, OP_EQ, 0); - tt_ptr_op(retptr, OP_EQ, buf); - tt_str_op(buf, OP_EQ, "GH"); - errno = 0; - - /* Check for EOF */ - retptr = tor_fgets(buf, sizeof(buf), test_stream); - tt_int_op(errno, OP_EQ, 0); - tt_ptr_op(retptr, OP_EQ, NULL); - retval = feof(test_stream); - tt_int_op(retval, OP_NE, 0); - errno = 0; - - /* Check that buf is unchanged according to C99 and C11 */ - tt_str_op(buf, OP_EQ, "GH"); - memset(buf, '\0', sizeof(buf)); - - done: - if (test_stream != NULL) - fclose(test_stream); - if (test_pipe[0] != -1) - close(test_pipe[0]); - if (test_pipe[1] != -1) - close(test_pipe[1]); -} - static void test_util_string_from_pipe(void *ptr) { @@ -5839,7 +5729,6 @@ struct testcase_t util_tests[] = { UTIL_TEST(num_cpus, 0), UTIL_TEST_WIN_ONLY(load_win_lib, 0), UTIL_TEST_NO_WIN(exit_status, 0), - UTIL_TEST_NO_WIN(fgets_eagain, 0), UTIL_TEST_NO_WIN(string_from_pipe, 0), UTIL_TEST(format_hex_number, 0), UTIL_TEST(format_dec_number, 0),