From 73a37d1e547d996515e2a0ffea8331bb535d2841 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 12 Sep 2018 08:57:18 -0400 Subject: [PATCH 1/2] Check waitpid return value and exit status in tinytest.c It's possible for a unit test to report success via its pipe, but to fail as it tries to clean up and exit. Notably, this happens on a leak sanitizer failure. Fixes bug 27658; bugfix on 0.2.2.4-alpha when tinytest was introduced. --- changes/bug27658 | 6 ++++++ src/ext/tinytest.c | 12 ++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 changes/bug27658 diff --git a/changes/bug27658 b/changes/bug27658 new file mode 100644 index 0000000000..8cc0aa4714 --- /dev/null +++ b/changes/bug27658 @@ -0,0 +1,6 @@ + o Minor bugfixes (testing): + - If a unit test running in a subprocess exits abnormally or with a + nonzero status code, treat the test as having failed, even if + the test reported success. Without this fix, memory leaks don't cause + cause the tests to fail, even with LeakSanitizer. Fixes bug 27658; + bugfix on 0.2.2.4-alpha. diff --git a/src/ext/tinytest.c b/src/ext/tinytest.c index 3fb1b39c71..a51cd6011a 100644 --- a/src/ext/tinytest.c +++ b/src/ext/tinytest.c @@ -207,12 +207,20 @@ testcase_run_forked_(const struct testgroup_t *group, r = (int)read(outcome_pipe[0], b, 1); if (r == 0) { printf("[Lost connection!] "); - return 0; + return FAIL; } else if (r != 1) { perror("read outcome from pipe"); } - waitpid(pid, &status, 0); + r = waitpid(pid, &status, 0); close(outcome_pipe[0]); + if (r == -1) { + perror("waitpid"); + return FAIL; + } + if (! WIFEXITED(status) || WEXITSTATUS(status) != 0) { + printf("[did not exit cleanly.]"); + return FAIL; + } return b[0]=='Y' ? OK : (b[0]=='S' ? SKIP : FAIL); } #endif From 0971b3ce4d03929ef09838209ab87de4f9dfcf1f Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 14 Sep 2018 10:56:34 -0400 Subject: [PATCH 2/2] hs-v3: Don't BUG() on directory permission check failure In hs_config.c, we do validate the permission of the hidden service directory but we do not try to create it. So, in the event that the directory doesn't exists, we end up in the loading key code path which checks for the permission and possibly creates the directory. On failure, don't BUG() since there is a perfectly valid use case for that function to fail. Fixes #27335 Signed-off-by: David Goulet --- changes/bug27335 | 4 ++++ src/or/hs_service.c | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) create mode 100644 changes/bug27335 diff --git a/changes/bug27335 b/changes/bug27335 new file mode 100644 index 0000000000..dcc55a945a --- /dev/null +++ b/changes/bug27335 @@ -0,0 +1,4 @@ + o Minor bugfixes (hidden service v3): + - In case the hidden service directory can't be created or has wrong + permissions, do not BUG() on it which lead to a non fatal stacktrace. + Fixes bug 27335; bugfix on 0.3.2.1. diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 33088480d1..408625c3ac 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -974,9 +974,9 @@ load_service_keys(hs_service_t *service) * files to that directory so make sure it exists and has the right * permissions. We do this here because at this stage we know that Tor is * actually running and the service we have has been validated. */ - if (BUG(hs_check_service_private_dir(get_options()->User, - config->directory_path, - config->dir_group_readable, 1) < 0)) { + if (hs_check_service_private_dir(get_options()->User, + config->directory_path, + config->dir_group_readable, 1) < 0) { goto end; }