From 6e408060251e5a8136bb06353fcd8083ff3e28fa Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Thu, 18 Jul 2013 16:01:49 +0300 Subject: [PATCH 1/6] Fix invalid-read when a managed proxy configuration fails. --- changes/bug9288 | 4 ++++ src/or/transports.c | 24 +++++++++++++++--------- 2 files changed, 19 insertions(+), 9 deletions(-) create mode 100644 changes/bug9288 diff --git a/changes/bug9288 b/changes/bug9288 new file mode 100644 index 0000000000..59bf414ea1 --- /dev/null +++ b/changes/bug9288 @@ -0,0 +1,4 @@ + o Minor bugfixes: + - Fix an invalid memory read that occured when a pluggable + transport proxy failed its configuration protocol. + Fixes bug 9288. diff --git a/src/or/transports.c b/src/or/transports.c index cfec70340c..604d9fce82 100644 --- a/src/or/transports.c +++ b/src/or/transports.c @@ -106,7 +106,7 @@ static void managed_proxy_destroy(managed_proxy_t *mp, int also_terminate_process); static void handle_finished_proxy(managed_proxy_t *mp); -static void configure_proxy(managed_proxy_t *mp); +static int configure_proxy(managed_proxy_t *mp); static void parse_method_error(const char *line, int is_server_method); #define parse_server_method_error(l) parse_method_error(l, 1) @@ -573,10 +573,8 @@ pt_configure_remaining_proxies(void) /* If the proxy is not fully configured, try to configure it futher. */ if (!proxy_configuration_finished(mp)) - configure_proxy(mp); - - if (proxy_configuration_finished(mp)) - at_least_a_proxy_config_finished = 1; + if (configure_proxy(mp) == 1) + at_least_a_proxy_config_finished = 1; } SMARTLIST_FOREACH_END(mp); @@ -588,10 +586,14 @@ pt_configure_remaining_proxies(void) mark_my_descriptor_dirty("configured managed proxies"); } -/** Attempt to continue configuring managed proxy mp. */ -static void +/** Attempt to continue configuring managed proxy mp. + * Return 1 if the transport configuration finished, and return 0 + * otherwise (if we still have more configuring to do for this + * proxy). */ +static int configure_proxy(managed_proxy_t *mp) { + int configuration_finished = 0; smartlist_t *proxy_output = NULL; enum stream_status stream_status = 0; @@ -601,7 +603,7 @@ configure_proxy(managed_proxy_t *mp) mp->conf_state = PT_PROTO_FAILED_LAUNCH; handle_finished_proxy(mp); } - return; + return 0; } tor_assert(mp->conf_state != PT_PROTO_INFANT); @@ -633,13 +635,17 @@ configure_proxy(managed_proxy_t *mp) done: /* if the proxy finished configuring, exit the loop. */ - if (proxy_configuration_finished(mp)) + if (proxy_configuration_finished(mp)) { handle_finished_proxy(mp); + configuration_finished = 1; + } if (proxy_output) { SMARTLIST_FOREACH(proxy_output, char *, cp, tor_free(cp)); smartlist_free(proxy_output); } + + return configuration_finished; } /** Register server managed proxy mp transports to state */ From 2e7c531fdc63ed3d96180a10ea5d7d27989092a9 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Mon, 29 Jul 2013 15:46:57 +0200 Subject: [PATCH 2/6] Prepare some mock functions to test #9288. --- src/common/util.c | 10 +++++----- src/common/util.h | 21 ++++++++++++--------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index d9913dda45..a8867b691d 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3969,9 +3969,9 @@ tor_spawn_background(const char *const filename, const char **argv, * process_handle. * If also_terminate_process is true, also terminate the * process of the process handle. */ -void -tor_process_handle_destroy(process_handle_t *process_handle, - int also_terminate_process) +MOCK_IMPL(void, +tor_process_handle_destroy,(process_handle_t *process_handle, + int also_terminate_process)) { if (!process_handle) return; @@ -4570,8 +4570,8 @@ log_from_handle(HANDLE *pipe, int severity) /** Return a smartlist containing lines outputted from * handle. Return NULL on error, and set * stream_status_out appropriately. */ -smartlist_t * -tor_get_lines_from_handle(FILE *handle, enum stream_status *stream_status_out) +MOCK_IMPL(smartlist_t *, +tor_get_lines_from_handle,(FILE *handle, enum stream_status *stream_status_out)) { enum stream_status stream_status; char stdout_buf[400]; diff --git a/src/common/util.h b/src/common/util.h index ad75266587..505ef23bd2 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -491,18 +491,21 @@ FILE *tor_process_get_stdout_pipe(process_handle_t *process_handle); #endif #ifdef _WIN32 -struct smartlist_t * -tor_get_lines_from_handle(HANDLE *handle, - enum stream_status *stream_status); +MOCK_DECL(struct smartlist_t *, +tor_get_lines_from_handle,(HANDLE *handle, + enum stream_status *stream_status)); #else -struct smartlist_t * -tor_get_lines_from_handle(FILE *handle, - enum stream_status *stream_status); +MOCK_DECL(struct smartlist_t *, +tor_get_lines_from_handle,(FILE *handle, + enum stream_status *stream_status)); #endif -int tor_terminate_process(process_handle_t *process_handle); -void tor_process_handle_destroy(process_handle_t *process_handle, - int also_terminate_process); +int +tor_terminate_process(process_handle_t *process_handle); + +MOCK_DECL(void, +tor_process_handle_destroy,(process_handle_t *process_handle, + int also_terminate_process)); /* ===== Insecure rng */ typedef struct tor_weak_rng_t { From aaf79eb4d334fb5e1f98d0a68b3a30dde983325a Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Mon, 29 Jul 2013 15:59:59 +0200 Subject: [PATCH 3/6] Write unit tests for configure_proxy(). --- src/or/transports.c | 4 +-- src/or/transports.h | 2 ++ src/test/test_pt.c | 71 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/src/or/transports.c b/src/or/transports.c index 604d9fce82..3aced21b3d 100644 --- a/src/or/transports.c +++ b/src/or/transports.c @@ -106,8 +106,6 @@ static void managed_proxy_destroy(managed_proxy_t *mp, int also_terminate_process); static void handle_finished_proxy(managed_proxy_t *mp); -static int configure_proxy(managed_proxy_t *mp); - static void parse_method_error(const char *line, int is_server_method); #define parse_server_method_error(l) parse_method_error(l, 1) #define parse_client_method_error(l) parse_method_error(l, 0) @@ -590,7 +588,7 @@ pt_configure_remaining_proxies(void) * Return 1 if the transport configuration finished, and return 0 * otherwise (if we still have more configuring to do for this * proxy). */ -static int +STATIC int configure_proxy(managed_proxy_t *mp) { int configuration_finished = 0; diff --git a/src/or/transports.h b/src/or/transports.h index cc3e018d6d..7f180a67b9 100644 --- a/src/or/transports.h +++ b/src/or/transports.h @@ -111,6 +111,8 @@ STATIC int parse_version(const char *line, managed_proxy_t *mp); STATIC void parse_env_error(const char *line); STATIC void handle_proxy_line(const char *line, managed_proxy_t *mp); +STATIC int configure_proxy(managed_proxy_t *mp); + #endif #endif diff --git a/src/test/test_pt.c b/src/test/test_pt.c index 16daa2836a..6648326e3f 100644 --- a/src/test/test_pt.c +++ b/src/test/test_pt.c @@ -5,9 +5,11 @@ #include "orconfig.h" #define PT_PRIVATE +#define UTIL_PRIVATE #include "or.h" #include "transports.h" #include "circuitbuild.h" +#include "util.h" #include "test.h" static void @@ -153,12 +155,81 @@ test_pt_protocol(void) tor_free(mp); } +#ifdef _WIN32 +static smartlist_t * +tor_get_lines_from_handle_replacement(HANDLE *handle, + enum stream_status *stream_status_out) +#else +static smartlist_t * +tor_get_lines_from_handle_replacement(FILE *handle, + enum stream_status *stream_status_out) +#endif +{ + (void) handle; + (void) stream_status_out; + static int times_called = 0; + + smartlist_t *retval_sl = smartlist_new(); + + /* Generate some dummy CMETHOD lines the first 5 times. The 6th + time, send 'CMETHODS DONE' to finish configuring the proxy. */ + if (times_called++ != 5) { + smartlist_add_asprintf(retval_sl, "CMETHOD mock%d socks5 127.0.0.1:555%d", + times_called, times_called); + } else { + smartlist_add(retval_sl, tor_strdup("CMETHODS DONE")); + } + + return retval_sl; +} + +/* NOP mock */ +static void +tor_process_handle_destroy_replacement(process_handle_t *process_handle, + int also_terminate_process) +{ + return; +} + +/* Test the configure_proxy() function. */ +static void +test_pt_configure_proxy(void *arg) +{ + (void) arg; + int i; + managed_proxy_t *mp = NULL; + + MOCK(tor_get_lines_from_handle, + tor_get_lines_from_handle_replacement); + MOCK(tor_process_handle_destroy, + tor_process_handle_destroy_replacement); + + mp = tor_malloc(sizeof(managed_proxy_t)); + mp->conf_state = PT_PROTO_ACCEPTING_METHODS; + mp->transports = smartlist_new(); + mp->transports_to_launch = smartlist_new(); + mp->process_handle = tor_malloc_zero(sizeof(process_handle_t)); + + /* Test the return value of configure_proxy() by calling it some + times while it is uninitialized and then finally finalizing its + configuration. */ + for (i = 0 ; i < 5 ; i++) { + test_assert(configure_proxy(mp) == 0); + } + test_assert(configure_proxy(mp) == 1); + + done: + UNMOCK(tor_get_lines_from_handle); + UNMOCK(tor_process_handle_destroy); +} #define PT_LEGACY(name) \ { #name, legacy_test_helper, 0, &legacy_setup, test_pt_ ## name } struct testcase_t pt_tests[] = { PT_LEGACY(parsing), PT_LEGACY(protocol), + { "configure_proxy",test_pt_configure_proxy, TT_FORK, + NULL, NULL }, END_OF_TESTCASES }; From 99bb6d2937a76caf7d6085fb063d89c1c8b3d9b6 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Mon, 29 Jul 2013 16:01:10 +0200 Subject: [PATCH 4/6] Modifications to transports.c for the unit tests to work. Both 'managed_proxy_list' and 'unconfigured_proxies_n' are global src/or/transports.c variables that are not initialized properly when unit tests are run. --- src/or/transports.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/transports.c b/src/or/transports.c index 3aced21b3d..0bd024fab7 100644 --- a/src/or/transports.c +++ b/src/or/transports.c @@ -712,7 +712,8 @@ managed_proxy_destroy(managed_proxy_t *mp, smartlist_free(mp->transports_to_launch); /* remove it from the list of managed proxies */ - smartlist_remove(managed_proxy_list, mp); + if (managed_proxy_list) + smartlist_remove(managed_proxy_list, mp); /* free the argv */ free_execve_args(mp->argv); @@ -749,7 +750,6 @@ handle_finished_proxy(managed_proxy_t *mp) } unconfigured_proxies_n--; - tor_assert(unconfigured_proxies_n >= 0); } /** Return true if the configuration of the managed proxy mp is From 22a074caa71723b35c3a59620f8a2ebdbf290388 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 29 Jul 2013 11:37:39 -0400 Subject: [PATCH 5/6] Update pt/configure_proxy until it stops segfaulting --- src/or/statefile.c | 4 ++-- src/or/statefile.h | 2 +- src/test/test_pt.c | 32 +++++++++++++++++++++++++------- 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/src/or/statefile.c b/src/or/statefile.c index bcb7b07417..aac8850b16 100644 --- a/src/or/statefile.c +++ b/src/or/statefile.c @@ -117,8 +117,8 @@ static const config_format_t state_format = { static or_state_t *global_state = NULL; /** Return the persistent state struct for this Tor. */ -or_state_t * -get_or_state(void) +MOCK_IMPL(or_state_t *, +get_or_state, (void)) { tor_assert(global_state); return global_state; diff --git a/src/or/statefile.h b/src/or/statefile.h index dcdee6c604..762b0f5ee1 100644 --- a/src/or/statefile.h +++ b/src/or/statefile.h @@ -7,7 +7,7 @@ #ifndef TOR_STATEFILE_H #define TOR_STATEFILE_H -or_state_t *get_or_state(void); +MOCK_DECL(or_state_t *,get_or_state,(void)); int did_last_state_file_write_fail(void); int or_state_save(time_t now); diff --git a/src/test/test_pt.c b/src/test/test_pt.c index 6648326e3f..162d9e3da0 100644 --- a/src/test/test_pt.c +++ b/src/test/test_pt.c @@ -10,6 +10,7 @@ #include "transports.h" #include "circuitbuild.h" #include "util.h" +#include "statefile.h" #include "test.h" static void @@ -156,14 +157,14 @@ test_pt_protocol(void) } #ifdef _WIN32 -static smartlist_t * -tor_get_lines_from_handle_replacement(HANDLE *handle, - enum stream_status *stream_status_out) +#define STDIN_HANDLE HANDLE #else -static smartlist_t * -tor_get_lines_from_handle_replacement(FILE *handle, - enum stream_status *stream_status_out) +#define STDIN_HANDLE FILE #endif + +static smartlist_t * +tor_get_lines_from_handle_replacement(STDIN_HANDLE *handle, + enum stream_status *stream_status_out) { (void) handle; (void) stream_status_out; @@ -188,7 +189,16 @@ static void tor_process_handle_destroy_replacement(process_handle_t *process_handle, int also_terminate_process) { - return; + (void) process_handle; + (void) also_terminate_process; +} + +static or_state_t *dummy_state = NULL; + +static or_state_t * +get_or_state_replacement(void) +{ + return dummy_state; } /* Test the configure_proxy() function. */ @@ -199,16 +209,22 @@ test_pt_configure_proxy(void *arg) int i; managed_proxy_t *mp = NULL; + dummy_state = tor_malloc_zero(sizeof(or_state_t)); + MOCK(tor_get_lines_from_handle, tor_get_lines_from_handle_replacement); MOCK(tor_process_handle_destroy, tor_process_handle_destroy_replacement); + MOCK(get_or_state, + get_or_state_replacement); mp = tor_malloc(sizeof(managed_proxy_t)); mp->conf_state = PT_PROTO_ACCEPTING_METHODS; mp->transports = smartlist_new(); mp->transports_to_launch = smartlist_new(); mp->process_handle = tor_malloc_zero(sizeof(process_handle_t)); + mp->argv = tor_malloc_zero(sizeof(char*)*2); + mp->argv[0] = tor_strdup(""); /* Test the return value of configure_proxy() by calling it some times while it is uninitialized and then finally finalizing its @@ -219,9 +235,11 @@ test_pt_configure_proxy(void *arg) test_assert(configure_proxy(mp) == 1); done: + tor_free(dummy_state); UNMOCK(tor_get_lines_from_handle); UNMOCK(tor_process_handle_destroy); } + #define PT_LEGACY(name) \ { #name, legacy_test_helper, 0, &legacy_setup, test_pt_ ## name } From 8a0eedbbb08199818cd7c2c6998f062c03e33122 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 31 Jul 2013 13:36:17 -0400 Subject: [PATCH 6/6] Fix mixed declaration/statement warning --- src/test/test_pt.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/test_pt.c b/src/test/test_pt.c index 162d9e3da0..afa4917da7 100644 --- a/src/test/test_pt.c +++ b/src/test/test_pt.c @@ -166,11 +166,11 @@ static smartlist_t * tor_get_lines_from_handle_replacement(STDIN_HANDLE *handle, enum stream_status *stream_status_out) { + static int times_called = 0; + smartlist_t *retval_sl = smartlist_new(); + (void) handle; (void) stream_status_out; - static int times_called = 0; - - smartlist_t *retval_sl = smartlist_new(); /* Generate some dummy CMETHOD lines the first 5 times. The 6th time, send 'CMETHODS DONE' to finish configuring the proxy. */ @@ -205,9 +205,9 @@ get_or_state_replacement(void) static void test_pt_configure_proxy(void *arg) { - (void) arg; int i; managed_proxy_t *mp = NULL; + (void) arg; dummy_state = tor_malloc_zero(sizeof(or_state_t));