Skip to content

Commit

Permalink
fix: panic due to negative seqnums in sequence unwrapper
Browse files Browse the repository at this point in the history
Out-of-order sequence numbers from a previous wrap around causes the
sequence unwrapper to generate negative sequence numbers. That, in turn,
generates a panic in the jitterbuffer and receive logs.

Adjust the sequence unwrapper to adjust the wraparound counters if older
packets from a previous wraparound arrive.
  • Loading branch information
prlanzarin committed Jan 18, 2024
1 parent c3b506b commit 602359d
Show file tree
Hide file tree
Showing 3 changed files with 115 additions and 7 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

All notable changes to this project will be documented in this file.

### v0.6.0 (UNRELEASED)

* feat: recorder.writeToDevNull option to write files to /dev/null (testing)
* fix: panic due to negative seqnums in sequence unwrapper

### v0.5.2

* fix: lock EBML write and close ops
Expand Down
49 changes: 42 additions & 7 deletions internal/webrtc/utils/sequence_unwrapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ type SequenceUnwrapper struct {
wrapArounds int64
highestSequenceNumber uint64
inMax uint64
lastSeqNo uint64
lastWrappedSeqNo int64
started bool
}

Expand Down Expand Up @@ -35,30 +37,63 @@ func (sw *SequenceUnwrapper) Unwrap(n uint64) int64 {
sw.m.Lock()
defer sw.m.Unlock()

sw.lastSeqNo = n
// If this is the first time we're called, just set the highest sequence number
// and return the value
if !sw.started {
sw.started = true
sw.highestSequenceNumber = n
return int64(n)
sw.lastWrappedSeqNo = int64(n)

return sw.lastWrappedSeqNo
}

// If the sequence number is the same as the highest sequence number, just return
// the last wrapped sequence number
if n == sw.highestSequenceNumber {
return sw.wrapArounds + int64(n)
sw.lastWrappedSeqNo = sw.wrapArounds + int64(n)
return sw.lastWrappedSeqNo
}

// If the sequence number is lower than the highest sequence number, it means that
// the sequence number has wrapped around. If the difference between the highest
// sequence number and the current sequence number is greater than half the maximum
// value of the sequence number, it means that the sequence number has wrapped
// around more than once. In this case, we need to adjust the wrapArounds
// counter and the highest sequence number accordingly and return the new value.
// Otherwise, we just return the last wrapped sequence number
if n < sw.highestSequenceNumber {
if sw.highestSequenceNumber-n > sw.inMax/2 {
sw.wrapArounds += int64(sw.inMax)
sw.highestSequenceNumber = n
return sw.wrapArounds + int64(n)
sw.lastWrappedSeqNo = sw.wrapArounds + int64(n)
return sw.lastWrappedSeqNo
}

return sw.wrapArounds + int64(n)
sw.lastWrappedSeqNo = sw.wrapArounds + int64(n)
return sw.lastWrappedSeqNo
}

if n-sw.highestSequenceNumber > sw.inMax/2 {
return sw.wrapArounds - int64(sw.inMax) + int64(n)
// If the difference between the current sequence number and the highest
// sequence number is greater than half the maximum value of the sequence
// number, it means that the sequence number has wrapped around.
// In this case, we need to adjust the wrapArounds counter and the
// highest sequence number accordingly and return the new value. Otherwise, we just
// return the last wrapped sequence number
delta := n - sw.highestSequenceNumber
if delta > (sw.inMax >> 1) {
// If the unwrapped seqnum ends up being negative, it means that the sequence
// number if from a previous wraparound. In this case, we need to adjust the
// wrapArounds counter accordingly
if sw.wrapArounds-int64(sw.inMax)+int64(n) <= 0 {
sw.wrapArounds += int64(sw.inMax)
}

sw.lastWrappedSeqNo = sw.wrapArounds - int64(sw.inMax) + int64(n)
return sw.lastWrappedSeqNo
}

sw.highestSequenceNumber = n
return sw.wrapArounds + int64(n)
sw.lastWrappedSeqNo = sw.wrapArounds + int64(n)
return sw.lastWrappedSeqNo
}
68 changes: 68 additions & 0 deletions internal/webrtc/utils/sequence_unwrapper_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package utils

import (
"testing"
)

func TestSequenceUnwrapper(t *testing.T) {
unwrapper := NewSequenceUnwrapper(16)

// Test case 1: Initialisation
result := unwrapper.Unwrap(uint64(65534))
expected := int64(65534)
if result != expected {
t.Errorf("[T1] Unexpected result. Expected: %d, Got: %d", expected, result)
}

// Test case 2: in-order seqnum increment
result = unwrapper.Unwrap(uint64(65535))
expected = int64(65535)
if result != expected {
t.Errorf("[T2] Unexpected result. Expected: %d, Got: %d", expected, result)
}

// Test case 3: first wraparound (in-order seqnum increment)
result = unwrapper.Unwrap(uint64(0))
expected = int64(65536)
if result != expected {
t.Errorf("[T3] Unexpected result. Expected: %d, Got: %d", expected, result)
}

// Test case 4: Increment after wraparound (with gap)
result = unwrapper.Unwrap(uint64(2))
expected = int64(65538)
if result != expected {
t.Errorf("[T4] Unexpected result. Expected: %d, Got: %d", expected, result)
}

// Test case 5: second wraparound (simulated in-order seqnum increments)
for i := 3; i < (1 << 16); i++ {
unwrapper.Unwrap(uint64(i))
}
result = unwrapper.Unwrap(uint64(0))
expected = int64(131072)
if result != expected {
t.Errorf("[T5] Unexpected result. Expected: %d, Got: %d", expected, result)
}

// Test case 6: Duplicate seqnum
result = unwrapper.Unwrap(uint64(0))
expected = int64(131072)
if result != expected {
t.Errorf("[T6] Unexpected result. Expected: %d, Got: %d", expected, result)
}

// Test case for negative values
// Assuming highestSequenceNumber is 2, wrapArounds is 0, inMax is 65536
// and lastSeqNo is 65535
// Test case 7: Negative value
unwrapper.highestSequenceNumber = 2
unwrapper.wrapArounds = 0
unwrapper.inMax = 65536
unwrapper.lastSeqNo = 65530
result = unwrapper.Unwrap(uint64(65534))
expected = int64(65534)
if result != expected {
t.Errorf("[T7] Unexpected result for negative values. Expected: %d, Got: %d", expected, result)
}
}

0 comments on commit 602359d

Please sign in to comment.