[tor-bugs] #21312 [Obfuscation/Snowflake]: snowflake-client is pegged at 100% cpu

[Tor Bug Tracker & Wiki]

#21312: snowflake-client is pegged at 100% cpu
-----------------------------------+------------------------------
 Reporter:  arlolra                |          Owner:  arlolra
     Type:  defect                 |         Status:  needs_review
 Priority:  High                   |      Milestone:
Component:  Obfuscation/Snowflake  |        Version:
 Severity:  Major                  |     Resolution:
 Keywords:                         |  Actual Points:
Parent ID:                         |         Points:
 Reviewer:                         |        Sponsor:
-----------------------------------+------------------------------

Comment (by yawning):

 Replying to [comment:49 arlolra]:
 > > I put some guards around that and now my proxy seems to be happily
 serving connections w/o crash.
 >
 > I mean, in go-webrtc,
 >
 > {{{
 > --- a/peerconnection.go
 > +++ b/peerconnection.go
 > @@ -44,6 +44,7 @@ import "C"
 >  import (
 >         "errors"
 >         "fmt"
 > +       "sync"
 >         "unsafe"
 >  )
 >
 > @@ -124,6 +125,8 @@ type PeerConnection struct {
 >         localDescription        *SessionDescription
 >         remoteDescription       *SessionDescription
 >         canTrickleIceCandidates bool
 > +       destroyed               bool
 > +       destroyLock             *sync.Mutex
 > }}}

 Don't need to use a pointer, the zero value of a sync.Mutex is initialized
 and available for use.
 If this is the only mutex in the struct, it would be more idiomatic to do
 something like:
 {{{
 type PeerConn struct {
   sync.Mutex

   // Other members here.
 }

 foo := new(PeerConn)
 foo.Lock()
 // Critical section.
 foo.Unlock()
 }}}

 > {{{
 >         // Event handlers
 >         OnNegotiationNeeded        func()
 > @@ -152,6 +155,7 @@ func NewPeerConnection(config *Configuration)
 (*PeerConnection, error) {
 >                 return nil, errors.New("PeerConnection requires a
 Configuration.")
 >         }
 >         pc := new(PeerConnection)
 > +       pc.destroyLock = &sync.Mutex{}
 > }}}

 Remove assuming you change the pointer sillyness.

 > {{{
 >         pc.index = PCMap.Set(pc)
 >         // Internal CGO Peer wraps the native
 webrtc::PeerConnectionInterface.
 >         pc.cgoPeer = C.CGO_InitializePeer(C.int(pc.index))
 > @@ -169,6 +173,12 @@ func NewPeerConnection(config *Configuration)
 (*PeerConnection, error) {
 >  }
 >
 >  func (pc *PeerConnection) Destroy() error {
 > +       pc.destroyLock.Lock()
 > +       if pc.destroyed {
 > +               return nil
 > +       }
 > +       pc.destroyed = true
 > +       pc.destroyLock.Unlock()
 > }}}

 If you were having problems with multiple calls to `Destroy()`, changing
 the behavior from a nil pointer deref to a deadlock, is not what I would
 define as "happy".

 {{{
   pc.destroyLock.Lock()
   defer pc.destroyLock.Unlock()
   if pc.destroyed {
     return nil
   }
   pc.destroyed = true

   // Blah blah blah, unlock handled by the defered call.
 }}}

 Alternatively, if you don't want to use defer, then you need to unlock
 before returning in the branch.

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