commit 02fc0a5ecfc917e9261596816e926468db976453 Author: Alexander Færøy <ahf@torproject.org> Date: Wed Mar 8 23:11:42 2017 +0100 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(-) diff --git a/changes/bug20988 b/changes/bug20988 deleted file mode 100644 index b1d73f0..0000000 --- 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 da4283f..0dbede6 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 - * <b>size</b> from the given <b>stream</b> and store it in <b>str</b>. - * - * 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 <b>str</b>. - * - * Upon successful completion, this function returns a pointer to the string - * <b>str</b>. If EOF occurs before any characters are read the function will - * return NULL and the content of <b>str</b> 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 1f51ece..ee1c945 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 2c3ce10..f01668a 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 a8dc807..74811ea 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 640935a..130a019 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),