[tor-bugs] #16695 [Tor]: Decouple generating controller events from sending them to controllers

[Tor Bug Tracker & Wiki]

#16695: Decouple generating controller events  from sending them to controllers
------------------------+---------------------------------------------
     Reporter:  nickm   |      Owner:
         Type:  defect  |     Status:  needs_revision
     Priority:  normal  |  Milestone:  Tor: 0.2.7.x-final
    Component:  Tor     |    Version:
   Resolution:          |   Keywords:  SponsorS TorCoreTeam201508 blob
Actual Points:          |  Parent ID:  #16764
       Points:          |
------------------------+---------------------------------------------
Changes (by teor):

 * status:  needs_review => needs_revision


Comment:

 ''Multithreading''

 If this code could be accessed by multiple threads simultaneously, two
 threads could both proceed past `if (block_event_queue)`, then both
 increment using `++block_event_queue`.
 {{{
 static int block_event_queue = 0;
 }}}
 {{{
 if (block_event_queue) {
   tor_free(msg);
   return;
 }

 /* No queueing an event while queueing an event */
 ++block_event_queue;
 }}}

 I don't think that can happen if these statements are swapped around like
 this, but this code does allow for two simultaneous events to mutually
 block each other (and therefore both would be skipped).

 {{{
 static int block_event_queue = -1;
 }}}
 {{{
 /* No queueing an event while queueing an event */
 ++block_event_queue;

 if (block_event_queue) {
   tor_free(msg);
   goto done;
 }
 }}}

 I think we need atomics or locks to write code that avoids both these
 issues.

 Similarly, what happens if we are in the middle of queueing an event, then
 get a call to flush events?
 I can imagine that, depending on the exact order of
 connection_write_to_buf in queued_events_flush_all, and smartlist_add in
 queue_control_event_string, we might miss Also, does SMARTLIST_FOREACH_*
 cope with its list size changing (from another thread) while
 iterating/deleting?

 ''Error Handling''

 If `struct event_base *b = tor_libevent_get_base();` returns NULL in
 queue_control_event_string, we will continue to add events to the queue,
 and never clear them.
 Should we clear the queue if we can't register a callback?
 Do we need to check the return values of tor_event_new and event_active?
 (That said, if Libevent is failing, we probably have bigger issues.)

 Can we keep `tor_assert(event >= EVENT_MIN_ && event <= EVENT_MAX_);` in
 send_control_event_string?

 Typos:
 queue_control_event_string:
 "attempt to avoid queueing something we shouldn't have tot queue"

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16695#comment:4>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online