From 6e408060251e5a8136bb06353fcd8083ff3e28fa Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Thu, 18 Jul 2013 16:01:49 +0300 Subject: Fix invalid-read when a managed proxy configuration fails. --- src/or/transports.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) (limited to 'src/or/transports.c') 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 */ -- cgit v1.2.3-54-g00ecf From aaf79eb4d334fb5e1f98d0a68b3a30dde983325a Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Mon, 29 Jul 2013 15:59:59 +0200 Subject: 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(-) (limited to 'src/or/transports.c') 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 }; -- cgit v1.2.3-54-g00ecf From 99bb6d2937a76caf7d6085fb063d89c1c8b3d9b6 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Mon, 29 Jul 2013 16:01:10 +0200 Subject: 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(-) (limited to 'src/or/transports.c') 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 -- cgit v1.2.3-54-g00ecf