[tor-commits] [tor/maint-0.4.1] guard: Ignore marked for close circuit when changing state to open
commit e3f3478032921a8b6af07a655fd2ea520d3ff463
Author: David Goulet
Date: Tue Jun 18 13:32:45 2019 -0400
guard: Ignore marked for close circuit when changing state to open
When we consider all circuits in "waiting for guard" state to be promoted to
an "open" state, we were considering all circuits, even the one marked for
close.
This ultiamtely triggers a "circuit_has_opened()" called on the circuit that
is marked for close which then leads to possible undesirable behaviors
within
a subsystem.
For instance, the HS subsystem would be unable to find the authentication
key
of the introduction point circuit leading to a BUG() warning and a duplicate
mark for close on the circuit.
This commit also adds a unit test to make sure we never select marked for
close circuits when upgrading its guard state from waiting for guard to
open.
Fixes #30871
Signed-off-by: David Goulet
---
changes/ticket30871 | 6 ++
src/feature/client/entrynodes.c | 4
src/test/test_circuitbuild.c| 47 +
3 files changed, 57 insertions(+)
diff --git a/changes/ticket30871 b/changes/ticket30871
new file mode 100644
index 0..81c076bb0
--- /dev/null
+++ b/changes/ticket30871
@@ -0,0 +1,6 @@
+ o Major bugfixes (circuit build, guard):
+- When considering upgrading circuits from "waiting for guard" to "open",
+ always ignore the ones that are mark for close. Else, we can end up in
+ the situation where a subsystem is notified of that circuit opening but
+ still marked for close leading to undesirable behavior. Fixes bug 30871;
+ bugfix on 0.3.0.1-alpha.
diff --git a/src/feature/client/entrynodes.c b/src/feature/client/entrynodes.c
index e543289ce..d191f5836 100644
--- a/src/feature/client/entrynodes.c
+++ b/src/feature/client/entrynodes.c
@@ -2611,6 +2611,10 @@ entry_guards_upgrade_waiting_circuits(guard_selection_t
*gs,
entry_guard_t *guard = entry_guard_handle_get(state->guard);
if (!guard || guard->in_selection != gs)
continue;
+if (TO_CIRCUIT(circ)->marked_for_close) {
+ /* Don't consider any marked for close circuits. */
+ continue;
+}
smartlist_add(all_circuits, circ);
} SMARTLIST_FOREACH_END(circ);
diff --git a/src/test/test_circuitbuild.c b/src/test/test_circuitbuild.c
index 275f1eced..538f20781 100644
--- a/src/test/test_circuitbuild.c
+++ b/src/test/test_circuitbuild.c
@@ -4,6 +4,8 @@
/* See LICENSE for licensing information */
#define CIRCUITBUILD_PRIVATE
+#define CIRCUITLIST_PRIVATE
+#define ENTRYNODES_PRIVATE
#include "core/or/or.h"
#include "test/test.h"
@@ -13,7 +15,11 @@
#include "core/or/circuitbuild.h"
#include "core/or/circuitlist.h"
+#include "core/or/cpath_build_state_st.h"
#include "core/or/extend_info_st.h"
+#include "core/or/origin_circuit_st.h"
+
+#include "feature/client/entrynodes.h"
/* Dummy nodes smartlist for testing */
static smartlist_t dummy_nodes;
@@ -126,10 +132,51 @@ test_new_route_len_unhandled_exit(void *arg)
UNMOCK(count_acceptable_nodes);
}
+static void
+test_upgrade_from_guard_wait(void *arg)
+{
+ circuit_t *circ = NULL;
+ origin_circuit_t *orig_circ = NULL;
+ entry_guard_t *guard = NULL;
+ smartlist_t *list = NULL;
+
+ (void) arg;
+
+ circ = dummy_origin_circuit_new(0);
+ orig_circ = TO_ORIGIN_CIRCUIT(circ);
+ tt_assert(orig_circ);
+
+ orig_circ->build_state = tor_malloc_zero(sizeof(cpath_build_state_t));
+
+ circuit_set_state(circ, CIRCUIT_STATE_GUARD_WAIT);
+
+ /* Put it in guard wait state. */
+ guard = tor_malloc_zero(sizeof(*guard));
+ guard->in_selection = get_guard_selection_info();
+
+ orig_circ->guard_state =
+circuit_guard_state_new(guard, GUARD_CIRC_STATE_WAITING_FOR_BETTER_GUARD,
+NULL);
+
+ /* Mark the circuit for close. */
+ circuit_mark_for_close(circ, END_CIRC_REASON_TORPROTOCOL);
+ tt_int_op(circ->marked_for_close, OP_NE, 0);
+
+ /* We shouldn't pick the mark for close circuit. */
+ list = circuit_find_circuits_to_upgrade_from_guard_wait();
+ tt_assert(!list);
+
+ done:
+ circuit_free(circ);
+ entry_guard_free_(guard);
+}
+
struct testcase_t circuitbuild_tests[] = {
{ "noexit", test_new_route_len_noexit, 0, NULL, NULL },
{ "safe_exit", test_new_route_len_safe_exit, 0, NULL, NULL },
{ "unsafe_exit", test_new_route_len_unsafe_exit, 0, NULL, NULL },
{ "unhandled_exit", test_new_route_len_unhandled_exit, 0, NULL, NULL },
+ { "upgrade_from_guard_wait", test_upgrade_from_guard_wait, TT_FORK,
+NULL, NULL },
END_OF_TESTCASES
};
___
tor-commits mailing list
tor-commits@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits
[tor-commits] [tor/maint-0.4.1] guard: Ignore marked for close circuit when changing state to open
commit 6a0763cd665e50174df5a7ea6ae0a27db7c9945f
Author: David Goulet
Date: Tue Jun 18 13:32:45 2019 -0400
guard: Ignore marked for close circuit when changing state to open
When we consider all circuits in "waiting for guard" state to be promoted to
an "open" state, we were considering all circuits, even the one marked for
close.
This ultiamtely triggers a "circuit_has_opened()" called on the circuit that
is marked for close which then leads to possible undesirable behaviors
within
a subsystem.
For instance, the HS subsystem would be unable to find the authentication
key
of the introduction point circuit leading to a BUG() warning and a duplicate
mark for close on the circuit.
This commit also adds a unit test to make sure we never select marked for
close circuits when upgrading its guard state from waiting for guard to
open.
Fixes #30871
Signed-off-by: David Goulet
---
changes/ticket30871 | 6 ++
src/feature/client/entrynodes.c | 4
src/test/test_circuitbuild.c| 47 +
3 files changed, 57 insertions(+)
diff --git a/changes/ticket30871 b/changes/ticket30871
new file mode 100644
index 0..81c076bb0
--- /dev/null
+++ b/changes/ticket30871
@@ -0,0 +1,6 @@
+ o Major bugfixes (circuit build, guard):
+- When considering upgrading circuits from "waiting for guard" to "open",
+ always ignore the ones that are mark for close. Else, we can end up in
+ the situation where a subsystem is notified of that circuit opening but
+ still marked for close leading to undesirable behavior. Fixes bug 30871;
+ bugfix on 0.3.0.1-alpha.
diff --git a/src/feature/client/entrynodes.c b/src/feature/client/entrynodes.c
index 4afcee202..54a9238d8 100644
--- a/src/feature/client/entrynodes.c
+++ b/src/feature/client/entrynodes.c
@@ -2611,6 +2611,10 @@ entry_guards_upgrade_waiting_circuits(guard_selection_t
*gs,
entry_guard_t *guard = entry_guard_handle_get(state->guard);
if (!guard || guard->in_selection != gs)
continue;
+if (TO_CIRCUIT(circ)->marked_for_close) {
+ /* Don't consider any marked for close circuits. */
+ continue;
+}
smartlist_add(all_circuits, circ);
} SMARTLIST_FOREACH_END(circ);
diff --git a/src/test/test_circuitbuild.c b/src/test/test_circuitbuild.c
index 47218a559..0c2309159 100644
--- a/src/test/test_circuitbuild.c
+++ b/src/test/test_circuitbuild.c
@@ -4,6 +4,8 @@
/* See LICENSE for licensing information */
#define CIRCUITBUILD_PRIVATE
+#define CIRCUITLIST_PRIVATE
+#define ENTRYNODES_PRIVATE
#include "core/or/or.h"
#include "test/test.h"
@@ -13,7 +15,11 @@
#include "core/or/circuitbuild.h"
#include "core/or/circuitlist.h"
+#include "core/or/cpath_build_state_st.h"
#include "core/or/extend_info_st.h"
+#include "core/or/origin_circuit_st.h"
+
+#include "feature/client/entrynodes.h"
/* Dummy nodes smartlist for testing */
static smartlist_t dummy_nodes;
@@ -126,10 +132,51 @@ test_new_route_len_unhandled_exit(void *arg)
UNMOCK(count_acceptable_nodes);
}
+static void
+test_upgrade_from_guard_wait(void *arg)
+{
+ circuit_t *circ = NULL;
+ origin_circuit_t *orig_circ = NULL;
+ entry_guard_t *guard = NULL;
+ smartlist_t *list = NULL;
+
+ (void) arg;
+
+ circ = dummy_origin_circuit_new(0);
+ orig_circ = TO_ORIGIN_CIRCUIT(circ);
+ tt_assert(orig_circ);
+
+ orig_circ->build_state = tor_malloc_zero(sizeof(cpath_build_state_t));
+
+ circuit_set_state(circ, CIRCUIT_STATE_GUARD_WAIT);
+
+ /* Put it in guard wait state. */
+ guard = tor_malloc_zero(sizeof(*guard));
+ guard->in_selection = get_guard_selection_info();
+
+ orig_circ->guard_state =
+circuit_guard_state_new(guard, GUARD_CIRC_STATE_WAITING_FOR_BETTER_GUARD,
+NULL);
+
+ /* Mark the circuit for close. */
+ circuit_mark_for_close(circ, END_CIRC_REASON_TORPROTOCOL);
+ tt_int_op(circ->marked_for_close, OP_NE, 0);
+
+ /* We shouldn't pick the mark for close circuit. */
+ list = circuit_find_circuits_to_upgrade_from_guard_wait();
+ tt_assert(!list);
+
+ done:
+ circuit_free(circ);
+ entry_guard_free_(guard);
+}
+
struct testcase_t circuitbuild_tests[] = {
{ "noexit", test_new_route_len_noexit, 0, NULL, NULL },
{ "safe_exit", test_new_route_len_safe_exit, 0, NULL, NULL },
{ "unsafe_exit", test_new_route_len_unsafe_exit, 0, NULL, NULL },
{ "unhandled_exit", test_new_route_len_unhandled_exit, 0, NULL, NULL },
+ { "upgrade_from_guard_wait", test_upgrade_from_guard_wait, TT_FORK,
+NULL, NULL },
END_OF_TESTCASES
};
___
tor-commits mailing list
tor-commits@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits
