[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:
-----------------------------------+-----------------------------------
Changes (by dcf):

 * status:  needs_review => needs_information


Comment:

 I agree with your diagnosis. I can make the segfault happen by applying
 this change, closing the datachannel immediately after it is opened:
 {{{#!diff
 --- a/proxy-go/snowflake.go
 +++ b/proxy-go/snowflake.go
 @@ -280,6 +280,7 @@ func makePeerConnectionFromOffer(sdp
 *webrtc.SessionDescription, config *webrtc.

                 dc.OnOpen = func() {
                         log.Println("OnOpen channel")
 +                       dc.Close()
                 }
                 dc.OnClose = func() {
                         conn.lock.Lock()
 }}}
 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.

 ----

 The fix looks okay to me. I presume there haven't been any more problems
 since you deployed it?

 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
 }}}

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