Repository: pion/ice (go)

Issue:
Candidate.Priority() is completely wrong
https://github.com/pion/ice/blob/7245ac0866456f7c39a534f496d0f346ab7aad30/candidate.go#L212

`^` is XOR in Go, not exponentiation. `2^24 == 26`.

It should be `(1<<24) * c.Type().Preference()` or simply `c.Type().Preference() << 24`.

`uint16` is also not large enough to hold the correct value.

Pull request description:
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


Patch:
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/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) {


Test patch:
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_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)
+	}
+}