Re: [tor-bugs] #32576 [Circumvention/Snowflake]: Fix race condition in snowflake broker

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#32576: Fix race condition in snowflake broker
-------------------------------------+------------------------------
 Reporter:  cohosh                   |          Owner:  cohosh
     Type:  defect                   |         Status:  needs_review
 Priority:  Very High                |      Milestone:
Component:  Circumvention/Snowflake  |        Version:
 Severity:  Normal                   |     Resolution:
 Keywords:  metrics                  |  Actual Points:
Parent ID:                           |         Points:
 Reviewer:                           |        Sponsor:  Sponsor28
-------------------------------------+------------------------------
Changes (by cohosh):

 * status:  assigned => needs_review


Comment:

 This race condition occurs because both the proxy process and the client
 process try to `Pop`/`Remove` the same snowflake from the heap.

 When a proxy polls the broker, it starts a go routine that waits for an
 offer through the snowflake's offer channel, or waits for a timeout:
 {{{
         go func(request *ProxyPoll) {
             select {
             case offer := <-snowflake.offerChannel:
                 request.offerChannel <- offer
             case <-time.After(time.Second * ProxyTimeout):
                 // This snowflake is no longer available to serve clients.
                 // TODO: Fix race using a delete channel
                 heap.Remove(ctx.snowflakes, snowflake.index)
                 delete(ctx.idToSnowflake, snowflake.id)
                 request.offerChannel <- nil
             }
         }(request)
 }}}

 Snowflakes are removed from the heap when they time out or if they are
 popped off the heap by a client:
 {{{
 func clientOffers(ctx *BrokerContext, w http.ResponseWriter, r
 *http.Request) {

 ...

     // Delete must be deferred in order to correctly process answer
 request later.
     snowflake := heap.Pop(ctx.snowflakes).(*Snowflake)
     defer delete(ctx.idToSnowflake, snowflake.id)
     snowflake.offerChannel <- offer
 }}}

 A race can always occur where the timeout happens after the `Pop` removes
 the snowflake but before the offer is sent through `offerChannel`. This
 can be fixed through the use of locks and a check to see if
 `snowflake.index` has been set to `-1` (which happens after it has been
 popped off the heap). Here's a patch that adds synchronization to the
 broker to prevent simultaneous access to the heap as well as the
 `idToSnowflake` map: https://github.com/cohosh/snowflake/pull/14

 I'd like to do some tests before merging this to make sure that the
 synchronization doesn't slow things down too much, but a code review would
 be good at this point.

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

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs