#25688: proxy-go is still deadlocking occasionally
--------------------------------------------+------------------------------
Reporter: dcf | Owner: cohosh
Type: defect | Status:
| needs_revision
Priority: Low | Milestone:
Component: Obfuscation/Snowflake | Version:
Severity: Normal | Resolution:
Keywords: network-team-roadmap-2019-Q1Q2 | Actual Points:
Parent ID: | Points: 3
Reviewer: | Sponsor:
--------------------------------------------+------------------------------
Comment (by cohosh):
Here's a partial solution for the ICECandidateError problem:
https://github.com/cohosh/snowflake/compare/deadlock
Replying to [comment:16 dcf]:
> This code is weird... At first I saw the `answerChan <- struct{}{}` and
I thought, this channel is only used to send a single signal; maybe this
would be better as `close(answerChan)` as in
[https://blog.golang.org/pipelines#TOC_6. this example]. But then I don't
understand why it's checking `ok` from the channel read--that seems to
indicate that it's trying to distinguish a send from a channel close...
But the code only uses one of those two possibilities.
>
The code was a bit unusual. I think CreateAnswer was put into a go routine
because it's blocking on the ICE negotiation, but we need it to complete
before calling sendAnswer anyway, so I removed the go routine and it
appears to be functioning fine. This is not a complete solution (see
below).
> Replying to [comment:15 arlolra]:
> > > This error appears to occur *after* OnIceComplete() is fired,
meaning that in the proxy-go code:
> >
> > Ah, so it is a bug in go-webrtc to assume `iceComplete`, when only
`iceGathering` has completed,
> > https://github.com/keroserene/go-
webrtc/blob/master/peerconnection.go#L518-L522
>
> Yes, I think you're right.
It's also worth pointing out that the proxy's completion of ICE to create
the SDP answer doesn't necessarily imply that OnDataChannel will be called
(which is what was causing the problem in the first place). In order for
OnDataChannel to be called, the client has to receive the answer from the
broker, call SetRemoteDescripter, and trigger the data channel to open.
The token is being returned only once OnDataChannel has been called:
{{{
func datachannelHandler(conn *webRTCConn, remoteAddr net.Addr) {
defer conn.Close()
defer retToken()
}}}
and the rest of the code assumes that as long as `pc.CreateAnswer`
succeeded, it doesn't need to worry about returning the token.
That means that a client can cause the proxy to lose tokens by sending an
offer, and then doing nothing with the answer. I think it was happening
because of an ICE error before, but we need to make sure we fix the
faulty/malicious client case as well.
This is tricky to solve because there isn't a PeerConnection callback for
"SDP answer sent, but data channel was never opened".
>
> > > It's also worth pointing out that apparently OnICEComplete is being
deprecated (​peerconnection.go#L518) we are better off not relying on it
anyway.
> >
> > If that's the case, we should remove it there too.
>
> Agreed.
Just noting that the snowflake client also uses the OnICEComplete callback
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25688#comment:17>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs
