[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
[tor-commits] [tor/master] Remove buffered I/O stream usage in process_handle_t.
commit 6e78ede73f190f3aaf91623a131a20cf031aad7e
Author: Alexander Færøy <ahf@xxxxxxxxxxxxxx>
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);
_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits