From 97702c69b03b19a8a6f867e56f716ce984550fa0 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 4 Dec 2017 14:48:15 -0500 Subject: [PATCH 1/2] sched: Set channel scheduler state to IDLE when not opened In the KIST main loop, if the channel happens to be not opened, set its state to IDLE so we can release it properly later on. Prior to this fix, the channel was in PENDING state, removed from the channel pending list and then kept in that state because it is not opened. This bug was introduced in commit dcabf801e52a83e2c3cc23ccc1fa906582a927d6 for which we made the scheduler loop not consider unopened channel. This has no consequences on tor except for an annoying but harmless BUG() warning. Fixes #24502 Signed-off-by: David Goulet --- changes/bug24502 | 4 ++++ src/or/scheduler_kist.c | 5 ++++- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 changes/bug24502 diff --git a/changes/bug24502 b/changes/bug24502 new file mode 100644 index 000000000..3fa6fb58d --- /dev/null +++ b/changes/bug24502 @@ -0,0 +1,4 @@ + o Minor bugfixes (scheduler): + - Properly set the scheduler state of an unopened channel in the KIST + scheduler main loop. This prevents a harmless but annoying log warning. + Fixes bug 24502; bugfix on 0.3.2.4-alpha. diff --git a/src/or/scheduler_kist.c b/src/or/scheduler_kist.c index fea92705d..3d8f553ac 100644 --- a/src/or/scheduler_kist.c +++ b/src/or/scheduler_kist.c @@ -606,9 +606,12 @@ kist_scheduler_run(void) * fails leading to the channel to be closed which triggers a release * and free its entry in the socket table. And because of a engineering * design issue, the error is not propagated back so we don't get an - * error at this poin. So before we continue, make sure the channel is + * error at this point. So before we continue, make sure the channel is * open and if not just ignore it. See #23751. */ if (!CHANNEL_IS_OPEN(chan)) { + /* Channel isn't open so we put it back in IDLE mode. It is either + * renegotiating its TLS session or about to be released. */ + chan->scheduler_state = SCHED_CHAN_IDLE; continue; } /* flush_result has the # cells flushed */ From 1a55a5ff06f9022a115943248b0eeb9d3c67c6c8 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 6 Dec 2017 11:33:01 -0500 Subject: [PATCH 2/2] test: Add a KIST test for a non opened channel This makes sure that a non opened channel is never put back in the channel pending list and that its state is consistent with what we expect that is IDLE. Test the fixes in #24502. Signed-off-by: David Goulet --- src/test/test_scheduler.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/src/test/test_scheduler.c b/src/test/test_scheduler.c index d679d7cfe..63add2f38 100644 --- a/src/test/test_scheduler.c +++ b/src/test/test_scheduler.c @@ -807,6 +807,7 @@ test_scheduler_loop_kist(void *arg) #endif channel_t *ch1 = new_fake_channel(), *ch2 = new_fake_channel(); + channel_t *ch3 = new_fake_channel(); /* setup options so we're sure about what sched we are running */ MOCK(get_options, mock_get_options); @@ -857,14 +858,35 @@ test_scheduler_loop_kist(void *arg) the_scheduler->run(); channel_flush_some_cells_mock_free_all(); - tt_int_op(1,==,1); + + /* We'll try to run this closed channel threw the scheduler loop and make + * sure it ends up in the right state. */ + tt_assert(ch3); + ch3->magic = TLS_CHAN_MAGIC; + ch3->state = CHANNEL_STATE_OPEN; + ch3->cmux = circuitmux_alloc(); + channel_register(ch3); + tt_assert(ch3->registered); + + ch3->scheduler_state = SCHED_CHAN_WAITING_FOR_CELLS; + scheduler_channel_has_waiting_cells(ch3); + /* Should be in the pending list now waiting to be handled. */ + tt_int_op(ch3->scheduler_state, OP_EQ, SCHED_CHAN_PENDING); + tt_int_op(smartlist_len(get_channels_pending()), OP_EQ, 1); + /* By running the scheduler on a closed channel, it should end up in the + * IDLE state and not in the pending channel list. */ + ch3->state = CHANNEL_STATE_CLOSED; + the_scheduler->run(); + tt_int_op(ch3->scheduler_state, OP_EQ, SCHED_CHAN_IDLE); + tt_int_op(smartlist_len(get_channels_pending()), OP_EQ, 0); done: /* Prep the channel so the free() function doesn't explode. */ - ch1->state = ch2->state = CHANNEL_STATE_CLOSED; - ch1->registered = ch2->registered = 0; + ch1->state = ch2->state = ch3->state = CHANNEL_STATE_CLOSED; + ch1->registered = ch2->registered = ch3->registered = 0; channel_free(ch1); channel_free(ch2); + channel_free(ch3); UNMOCK(update_socket_info_impl); UNMOCK(channel_should_write_to_kernel); UNMOCK(channel_write_to_kernel);