commit 005e228f80f0bd241d01c106f87e6bfe6dda6127 Author: David Goulet dgoulet@torproject.org Date: Fri Feb 2 08:48:34 2018 -0500

sched: When releasing a channel, do not BUG() if absent from the pending list

The current code flow makes it that we can release a channel in a PENDING state but not in the pending list. This happens while the channel is being processed in the scheduler loop.

Fixes #25125

Signed-off-by: David Goulet dgoulet@torproject.org --- src/or/scheduler.c | 26 +++++++++++++++----------- src/test/test_scheduler.c | 12 ++++++++++++ 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/src/or/scheduler.c b/src/or/scheduler.c index 47141c538..1984084fe 100644 --- a/src/or/scheduler.c +++ b/src/or/scheduler.c @@ -628,17 +628,21 @@ scheduler_release_channel,(channel_t *chan)) return; }

- if (chan->scheduler_state == SCHED_CHAN_PENDING) { - if (SCHED_BUG(smartlist_pos(channels_pending, chan) == -1, chan)) { - log_warn(LD_SCHED, "Scheduler asked to release channel %" PRIu64 " " - "but it wasn't in channels_pending", - chan->global_identifier); - } else { - smartlist_pqueue_remove(channels_pending, - scheduler_compare_channels, - offsetof(channel_t, sched_heap_idx), - chan); - } + /* Try to remove the channel from the pending list regardless of its + * scheduler state. We can release a channel in many places in the tor code + * so we can't rely on the channel state (PENDING) to remove it from the + * list. + * + * For instance, the channel can change state from OPEN to CLOSING while + * being handled in the scheduler loop leading to the channel being in + * PENDING state but not in the pending list. Furthermore, we release the + * channel when it changes state to close and a second time when we free it. + * Not ideal at all but for now that is the way it is. */ + if (chan->sched_heap_idx != -1) { + smartlist_pqueue_remove(channels_pending, + scheduler_compare_channels, + offsetof(channel_t, sched_heap_idx), + chan); }

if (the_scheduler->on_channel_free) { diff --git a/src/test/test_scheduler.c b/src/test/test_scheduler.c index 1f014c4f6..724a6b56b 100644 --- a/src/test/test_scheduler.c +++ b/src/test/test_scheduler.c @@ -502,6 +502,18 @@ perform_channel_state_tests(int KISTSchedRunInterval, int sched_type) scheduler_touch_channel(ch1); tt_assert(scheduler_compare_channels_mock_ctr > old_count);

+ /* Release the ch2 and then do it another time to make sure it doesn't blow + * up and we are still in a quiescent state. */ + scheduler_release_channel(ch2); + tt_int_op(ch2->scheduler_state, OP_EQ, SCHED_CHAN_IDLE); + tt_int_op(smartlist_len(channels_pending), OP_EQ, 1); + /* Cheat a bit so make the release more confused but also will tells us if + * the release did put the channel in the right state. */ + ch2->scheduler_state = SCHED_CHAN_PENDING; + scheduler_release_channel(ch2); + tt_int_op(ch2->scheduler_state, OP_EQ, SCHED_CHAN_IDLE); + tt_int_op(smartlist_len(channels_pending), OP_EQ, 1); + /* Close */ channel_mark_for_close(ch1); tt_int_op(ch1->state, OP_EQ, CHANNEL_STATE_CLOSING);