[tor-bugs] #30206 [Obfuscation/Snowflake]: Segfault in proxy-go

[Tor Bug Tracker & Wiki]

#30206: Segfault in proxy-go
-----------------------------------+-----------------------------------
 Reporter:  irl                    |          Owner:  cohosh
     Type:  defect                 |         Status:  needs_information
 Priority:  Medium                 |      Milestone:
Component:  Obfuscation/Snowflake  |        Version:
 Severity:  Normal                 |     Resolution:
 Keywords:                         |  Actual Points:
Parent ID:                         |         Points:
 Reviewer:                         |        Sponsor:
-----------------------------------+-----------------------------------

Comment (by cohosh):

 Replying to [comment:5 dcf]:
 Thanks for taking a look at this!
 > I ''think'' the chain of events is as follows:
 > * `dc.OnClose` calls `pw.Close`, where `pw` feeds into the read part of
 a `webRTCConn`.
 > * Closing `pw` causes `CopyLoopTimeout` to exit, which causes
 `datachannelHandler` to exit, which makes a deferred call to
 `webRTCConn.Close`.
 > * `webRTCConn.Close` calls `pc.Destroy`.
 > * `pc.Destroy` calls `CGO_DestroyPeer`.
 > * `CGO_DestroyPeer` removes the peer from `localPeers`, which drops the
 peer's reference count to 0, which sets `pc_ = nullptr`.
 > * Later, when the `dataChannelTimeout` expires, it calls
 `pc.ConnectionState`, where `pc` is a stale object reference that has
 already had its destructor called. In particular, its `pc_` member is
 null. `CGO_IceConnectionState` deferences the null pointer.
 That checks out with my understanding of how this works.
 >
 > ----
 >
 > The fix looks okay to me. I presume there haven't been any more problems
 since you deployed it?
 Yup, no seg faults since I deployed the fix.
 >
 > Was [https://github.com/cohosh/snowflake/compare/ticket30206#diff-
 ba4bd8a4477426567c409d66c2cf8a28L33 this change] only to assist finding
 the bug, or is there another reason for it?
 > {{{#!diff
 > -const dataChannelTimeout = time.Minute
 > +const dataChannelTimeout = 20 * time.Second
 > }}}
 There were two reasons for this:
  1. An entire minute is a long time to wait in the event that there was an
 ICE error and the client needs to find a new proxy.
  2. The current proxy timeout is 30 seconds, which means that the earliest
 a client will attempt to close a data channel they believe is open is 30
 seconds later. So even without the seg fault fix, this would prevent a lot
 of failures. Obviously we don't need it with the fix, but it seems like a
 good idea to take into account our existing timeouts when creating this
 one.

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