From ad7c8697fdefab1096bedfa3e4b09f92fbcd21fe Mon Sep 17 00:00:00 2001 From: Michael MacDonald Date: Wed, 10 Apr 2019 22:31:59 -0400 Subject: [PATCH] Candidate priority is incorrectly calculated Some constants were being incorrectly generated using 2^N (XOR) instead of 1 << N. This resulted in incorrect candidate priorities being used to evaluate candidate pairs. Resolves #14. --- agent.go | 4 +- candidate.go | 8 +-- candidate_test.go | 47 ++++++++++++++++++ candidatepair.go | 32 ++++++------ candidatepair_test.go | 111 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 181 insertions(+), 21 deletions(-) create mode 100644 candidate_test.go create mode 100644 candidatepair_test.go diff --git a/agent.go b/agent.go index 8f31414..bbfada4 100644 --- a/agent.go +++ b/agent.go @@ -368,7 +368,7 @@ func (a *Agent) pingCandidate(local, remote *Candidate) { &stun.Username{Username: a.remoteUfrag + ":" + a.localUfrag}, &stun.UseCandidate{}, &stun.IceControlling{TieBreaker: a.tieBreaker}, - &stun.Priority{Priority: uint32(local.Priority())}, + &stun.Priority{Priority: local.Priority()}, &stun.MessageIntegrity{ Key: []byte(a.remotePwd), }, @@ -378,7 +378,7 @@ func (a *Agent) pingCandidate(local, remote *Candidate) { msg, err = stun.Build(stun.ClassRequest, stun.MethodBinding, stun.GenerateTransactionID(), &stun.Username{Username: a.remoteUfrag + ":" + a.localUfrag}, &stun.IceControlled{TieBreaker: a.tieBreaker}, - &stun.Priority{Priority: uint32(local.Priority())}, + &stun.Priority{Priority: local.Priority()}, &stun.MessageIntegrity{ Key: []byte(a.remotePwd), }, diff --git a/candidate.go b/candidate.go index 8f0cc4c..2eaa49f 100644 --- a/candidate.go +++ b/candidate.go @@ -202,16 +202,16 @@ func (c *Candidate) writeTo(raw []byte, dst *Candidate) (int, error) { } // Priority computes the priority for this ICE Candidate -func (c *Candidate) Priority() uint16 { +func (c *Candidate) Priority() uint32 { // The local preference MUST be an integer from 0 (lowest preference) to // 65535 (highest preference) inclusive. When there is only a single IP // address, this value SHOULD be set to 65535. If there are multiple // candidates for a particular component for a particular data stream // that have the same type, the local preference MUST be unique for each // one. - return (2^24)*c.Type.Preference() + - (2^8)*c.LocalPreference + - (2^0)*(256-c.Component) + return (1<<24)*uint32(c.Type.Preference()) + + (1<<8)*uint32(c.LocalPreference) + + uint32(256-c.Component) } // Equal is used to compare two CandidateBases diff --git a/candidate_test.go b/candidate_test.go new file mode 100644 index 0000000..aeb8955 --- /dev/null +++ b/candidate_test.go @@ -0,0 +1,47 @@ +package ice + +import "testing" + +func TestCandidatePriority(t *testing.T) { + for _, test := range []struct { + Candidate *Candidate + WantPriority uint32 + }{ + { + Candidate: &Candidate{ + Type: CandidateTypeHost, + LocalPreference: defaultLocalPreference, + Component: ComponentRTP, + }, + WantPriority: 2130706431, + }, + { + Candidate: &Candidate{ + Type: CandidateTypePeerReflexive, + LocalPreference: defaultLocalPreference, + Component: ComponentRTP, + }, + WantPriority: 1862270975, + }, + { + Candidate: &Candidate{ + Type: CandidateTypeServerReflexive, + LocalPreference: defaultLocalPreference, + Component: ComponentRTP, + }, + WantPriority: 1694498815, + }, + { + Candidate: &Candidate{ + Type: CandidateTypeRelay, + LocalPreference: defaultLocalPreference, + Component: ComponentRTP, + }, + WantPriority: 16777215, + }, + } { + if got, want := test.Candidate.Priority(), test.WantPriority; got != want { + t.Fatalf("Candidate(%v).Priority() = %d, want %d", test.Candidate, got, want) + } + } +} diff --git a/candidatepair.go b/candidatepair.go index 8a717f0..1e9670a 100644 --- a/candidatepair.go +++ b/candidatepair.go @@ -41,39 +41,41 @@ func (p *candidatePair) Equal(other *candidatePair) bool { // agent. Let D be the priority for the candidate provided by the // controlled agent. // pair priority = 2^32*MIN(G,D) + 2*MAX(G,D) + (G>D?1:0) -func (p *candidatePair) Priority() uint32 { +func (p *candidatePair) Priority() uint64 { var g uint32 var d uint32 if p.iceRoleControlling { - g = uint32(p.local.Priority()) - d = uint32(p.remote.Priority()) + g = p.local.Priority() + d = p.remote.Priority() } else { - g = uint32(p.remote.Priority()) - d = uint32(p.local.Priority()) + g = p.remote.Priority() + d = p.local.Priority() } // Just implement these here rather // than fooling around with the math package - min := func(x, y uint32) uint32 { + min := func(x, y uint32) uint64 { if x < y { - return x + return uint64(x) } - return y + return uint64(y) } - max := func(x, y uint32) uint32 { + max := func(x, y uint32) uint64 { if x > y { - return x + return uint64(x) } - return y + return uint64(y) } - cmp := func(x, y uint32) uint32 { + cmp := func(x, y uint32) uint64 { if x > y { - return 1 + return uint64(1) } - return 0 + return uint64(0) } - return (2^32)*min(g, d) + 2*max(g, d) + cmp(g, d) + // 1<<32 overflows uint32; and if both g && d are + // maxUint32, this result would overflow uint64 + return (1<<32-1)*min(g, d) + 2*max(g, d) + cmp(g, d) } func (p *candidatePair) Write(b []byte) (int, error) { diff --git a/candidatepair_test.go b/candidatepair_test.go new file mode 100644 index 0000000..313e684 --- /dev/null +++ b/candidatepair_test.go @@ -0,0 +1,111 @@ +package ice + +import "testing" + +var ( + hostCandidate = &Candidate{ + Type: CandidateTypeHost, + LocalPreference: defaultLocalPreference, + Component: ComponentRTP, + } + prflxCandidate = &Candidate{ + Type: CandidateTypePeerReflexive, + LocalPreference: defaultLocalPreference, + Component: ComponentRTP, + } + srflxCandidate = &Candidate{ + Type: CandidateTypeServerReflexive, + LocalPreference: defaultLocalPreference, + Component: ComponentRTP, + } + relayCandidate = &Candidate{ + Type: CandidateTypeRelay, + LocalPreference: defaultLocalPreference, + Component: ComponentRTP, + } +) + +func TestCandidatePairPriority(t *testing.T) { + for _, test := range []struct { + Pair *candidatePair + WantPriority uint64 + }{ + { + Pair: newCandidatePair( + hostCandidate, + hostCandidate, + false, + ), + WantPriority: 9151314440652587007, + }, + { + Pair: newCandidatePair( + hostCandidate, + hostCandidate, + true, + ), + WantPriority: 9151314440652587007, + }, + { + Pair: newCandidatePair( + hostCandidate, + prflxCandidate, + true, + ), + WantPriority: 7998392936314175488, + }, + { + Pair: newCandidatePair( + hostCandidate, + prflxCandidate, + false, + ), + WantPriority: 7998392936314175487, + }, + { + Pair: newCandidatePair( + hostCandidate, + srflxCandidate, + true, + ), + WantPriority: 7277816996102668288, + }, + { + Pair: newCandidatePair( + hostCandidate, + srflxCandidate, + false, + ), + WantPriority: 7277816996102668287, + }, + { + Pair: newCandidatePair( + hostCandidate, + relayCandidate, + true, + ), + WantPriority: 72057593987596288, + }, + { + Pair: newCandidatePair( + hostCandidate, + relayCandidate, + false, + ), + WantPriority: 72057593987596287, + }, + } { + if got, want := test.Pair.Priority(), test.WantPriority; got != want { + t.Fatalf("CandidatePair(%v).Priority() = %d, want %d", test.Pair, got, want) + } + } +} + +func TestCandidatePairEquality(t *testing.T) { + pairA := newCandidatePair(hostCandidate, srflxCandidate, true) + pairB := newCandidatePair(hostCandidate, srflxCandidate, false) + + if !pairA.Equal(pairB) { + t.Fatalf("Expected %v to equal %v", pairA, pairB) + } +}