Fix random crashes - invariant broken in LinkedSet (LRU) implementation.

Root cause: IsNewSequenceNumber didn't respect strict weak ordering requirements.
            (e.g. 0, 0x1000, 0x2000, ... 0x9000 are increasing, but 0x9000 < 0)
Solution: Unwrap the sequence numbers into int64_t for proper sorting.

This CL also introduce a simpler interface,
which does a better job at hiding implementation details.

Bug: webrtc:9575
Change-Id: Ic9922426de32278e8b51c6ecef8e2efeb0997512
Reviewed-on: https://webrtc-review.googlesource.com/91165
Reviewed-by: Patrik Höglund <phoglund@webrtc.org>
Reviewed-by: Björn Terelius <terelius@webrtc.org>
Commit-Queue: Yves Gerey <yvesg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24202}

modules/include/module_common_types_public.h

@@ -83,11 +83,15 @@ class Unwrapper {
 using SequenceNumberUnwrapper = Unwrapper<uint16_t>;
 using TimestampUnwrapper = Unwrapper<uint32_t>;
 
+// NB: Doesn't fulfill strict weak ordering requirements.
+//     Mustn't be used as std::map Compare function.
 inline bool IsNewerSequenceNumber(uint16_t sequence_number,
                                   uint16_t prev_sequence_number) {
   return IsNewer(sequence_number, prev_sequence_number);
 }
 
+// NB: Doesn't fulfill strict weak ordering requirements.
+//     Mustn't be used as std::map Compare function.
 inline bool IsNewerTimestamp(uint32_t timestamp, uint32_t prev_timestamp) {
   return IsNewer(timestamp, prev_timestamp);
 }

modules/remote_bitrate_estimator/test/bwe.cc

@@ -10,6 +10,7 @@
 
 #include "modules/remote_bitrate_estimator/test/bwe.h"
 
+#include <algorithm>
 #include <limits>
 
 #include "modules/remote_bitrate_estimator/test/estimators/bbr.h"
@@ -18,16 +19,13 @@
 #include "modules/remote_bitrate_estimator/test/estimators/send_side.h"
 #include "modules/remote_bitrate_estimator/test/estimators/tcp.h"
 #include "rtc_base/constructormagic.h"
+#include "rtc_base/numerics/safe_conversions.h"
 #include "rtc_base/system/fallthrough.h"
 
 namespace webrtc {
 namespace testing {
 namespace bwe {
 
-// With the assumption that packet loss is lower than 97%, the max gap
-// between elements in the set is lower than 0x8000, hence we have a
-// total order in the set. For (x,y,z) subset of the LinkedSet,
-// (x<=y and y<=z) ==> x<=z so the set can be sorted.
 const int kSetCapacity = 1000;
 
 BweReceiver::BweReceiver(int flow_id)
@@ -163,12 +161,7 @@ LossAccount BweReceiver::LinkedSetPacketLossRatio() {
     return LossAccount();
   }
 
-  uint16_t oldest_seq_num = received_packets_.OldestSeqNumber();
+  size_t set_total_packets = received_packets_.Range();
-  uint16_t newest_seq_num = received_packets_.NewestSeqNumber();
-
-  size_t set_total_packets =
-      static_cast<uint16_t>(newest_seq_num - oldest_seq_num + 1);
-
   size_t set_received_packets = received_packets_.size();
   size_t set_lost_packets = set_total_packets - set_received_packets;
 
@@ -195,26 +188,21 @@ float BweReceiver::RecentPacketLossRatio() {
   // Lowest timestamp limit, oldest one that should be checked.
   int64_t time_limit_ms = (*node_it)->arrival_time_ms - kPacketLossTimeWindowMs;
   // Oldest and newest values found within the given time window.
-  uint16_t oldest_seq_num = (*node_it)->sequence_number;
+  int64_t oldest_seq_num = (*node_it)->unwrapped_sequence_number;
-  uint16_t newest_seq_num = oldest_seq_num;
+  int64_t newest_seq_num = oldest_seq_num;
 
   while (node_it != received_packets_.end()) {
     if ((*node_it)->arrival_time_ms < time_limit_ms) {
       break;
     }
-    uint16_t seq_num = (*node_it)->sequence_number;
+    int64_t seq_num = (*node_it)->unwrapped_sequence_number;
-    if (IsNewerSequenceNumber(seq_num, newest_seq_num)) {
+    newest_seq_num = std::max(newest_seq_num, seq_num);
-      newest_seq_num = seq_num;
+    oldest_seq_num = std::min(oldest_seq_num, seq_num);
-    }
-    if (IsNewerSequenceNumber(oldest_seq_num, seq_num)) {
-      oldest_seq_num = seq_num;
-    }
     ++node_it;
     ++number_packets_received;
   }
   // Interval width between oldest and newest sequence number.
-  // There was an overflow if newest_seq_num < oldest_seq_num.
+  int gap = rtc::dchecked_cast<int>(newest_seq_num - oldest_seq_num + 1);
-  int gap = static_cast<uint16_t>(newest_seq_num - oldest_seq_num + 1);
 
   return static_cast<float>(gap - number_packets_received) / gap;
 }
@@ -230,7 +218,9 @@ void LinkedSet::Insert(uint16_t sequence_number,
                        int64_t send_time_ms,
                        int64_t arrival_time_ms,
                        size_t payload_size) {
-  auto it = map_.find(sequence_number);
+  auto unwrapped_sequence_number = unwrapper_.Unwrap(sequence_number);
+  auto it = map_.find(unwrapped_sequence_number);
+  // Handle duplicate unwrapped sequence number.
   if (it != map_.end()) {
     PacketNodeIt node_it = it->second;
     PacketIdentifierNode* node = *node_it;
@@ -238,34 +228,36 @@ void LinkedSet::Insert(uint16_t sequence_number,
     if (node_it != list_.begin()) {
       list_.erase(node_it);
       list_.push_front(node);
-      map_[sequence_number] = list_.begin();
+      map_[unwrapped_sequence_number] = list_.begin();
     }
   } else {
     if (size() == capacity_) {
       RemoveTail();
     }
-    UpdateHead(new PacketIdentifierNode(sequence_number, send_time_ms,
+    UpdateHead(new PacketIdentifierNode(unwrapped_sequence_number, send_time_ms,
                                         arrival_time_ms, payload_size));
   }
 }
 
 void LinkedSet::Insert(PacketIdentifierNode packet_identifier) {
-  Insert(packet_identifier.sequence_number, packet_identifier.send_time_ms,
+  Insert(packet_identifier.unwrapped_sequence_number,
-         packet_identifier.arrival_time_ms, packet_identifier.payload_size);
+         packet_identifier.send_time_ms, packet_identifier.arrival_time_ms,
+         packet_identifier.payload_size);
 }
 
 void LinkedSet::RemoveTail() {
-  map_.erase(list_.back()->sequence_number);
+  map_.erase(list_.back()->unwrapped_sequence_number);
   delete list_.back();
   list_.pop_back();
 }
+
 void LinkedSet::UpdateHead(PacketIdentifierNode* new_head) {
   list_.push_front(new_head);
-  map_[new_head->sequence_number] = list_.begin();
+  map_[new_head->unwrapped_sequence_number] = list_.begin();
 }
 
 void LinkedSet::Erase(PacketNodeIt node_it) {
-  map_.erase((*node_it)->sequence_number);
+  map_.erase((*node_it)->unwrapped_sequence_number);
   delete (*node_it);
   list_.erase(node_it);
 }

modules/remote_bitrate_estimator/test/bwe.h

@@ -21,19 +21,12 @@
 #include "modules/remote_bitrate_estimator/test/packet.h"
 #include "rtc_base/constructormagic.h"
 #include "rtc_base/gtest_prod_util.h"
+#include "rtc_base/numerics/sequence_number_util.h"
 
 namespace webrtc {
 namespace testing {
 namespace bwe {
 
-// Overload map comparator.
-class SequenceNumberOlderThan {
- public:
-  bool operator()(uint16_t seq_num_1, uint16_t seq_num_2) const {
-    return IsNewerSequenceNumber(seq_num_2, seq_num_1);
-  }
-};
-
 // Holds information for computing global packet loss.
 struct LossAccount {
   LossAccount() : num_total(0), num_lost(0) {}
@@ -49,16 +42,16 @@ struct LossAccount {
 // Holds only essential information about packets to be saved for
 // further use, e.g. for calculating packet loss and receiving rate.
 struct PacketIdentifierNode {
-  PacketIdentifierNode(uint16_t sequence_number,
+  PacketIdentifierNode(int64_t unwrapped_sequence_number,
                        int64_t send_time_ms,
                        int64_t arrival_time_ms,
                        size_t payload_size)
-      : sequence_number(sequence_number),
+      : unwrapped_sequence_number(unwrapped_sequence_number),
         send_time_ms(send_time_ms),
         arrival_time_ms(arrival_time_ms),
         payload_size(payload_size) {}
 
-  uint16_t sequence_number;
+  int64_t unwrapped_sequence_number;
   int64_t send_time_ms;
   int64_t arrival_time_ms;
   size_t payload_size;
@@ -92,9 +85,10 @@ class LinkedSet {
   size_t size() const { return list_.size(); }
   size_t capacity() const { return capacity_; }
 
-  uint16_t OldestSeqNumber() const { return empty() ? 0 : map_.begin()->first; }
+  // Return size of interval covering current set, i.e.:
-  uint16_t NewestSeqNumber() const {
+  // unwrapped newest seq number - unwrapped oldest seq number + 1
-    return empty() ? 0 : map_.rbegin()->first;
+  int64_t Range() const {
+    return empty() ? 0 : map_.rbegin()->first - map_.begin()->first + 1;
   }
 
   void Erase(PacketNodeIt node_it);
@@ -105,7 +99,10 @@ class LinkedSet {
   // Add new element to the front of the list and insert it in the map.
   void UpdateHead(PacketIdentifierNode* new_head);
   size_t capacity_;
-  std::map<uint16_t, PacketNodeIt, SequenceNumberOlderThan> map_;
+  // We want to keep track of the current oldest and newest sequence_numbers.
+  // To get strict weak ordering, we unwrap uint16_t into an int64_t.
+  SeqNumUnwrapper<uint16_t> unwrapper_;
+  std::map<int64_t, PacketNodeIt> map_;
   std::list<PacketIdentifierNode*> list_;
 };
 

modules/remote_bitrate_estimator/test/bwe_unittest.cc

@@ -33,8 +33,7 @@ class LinkedSetTest : public ::testing::Test {
 };
 
 TEST_F(LinkedSetTest, EmptySet) {
-  EXPECT_EQ(linked_set_.OldestSeqNumber(), 0);
+  EXPECT_EQ(linked_set_.Range(), 0);
-  EXPECT_EQ(linked_set_.NewestSeqNumber(), 0);
 }
 
 TEST_F(LinkedSetTest, SinglePacket) {
@@ -42,8 +41,7 @@ TEST_F(LinkedSetTest, SinglePacket) {
   // Other parameters don't matter here.
   linked_set_.Insert(kSeqNumber, 0, 0, 0);
 
-  EXPECT_EQ(linked_set_.OldestSeqNumber(), kSeqNumber);
+  EXPECT_EQ(linked_set_.Range(), 1);
-  EXPECT_EQ(linked_set_.NewestSeqNumber(), kSeqNumber);
 }
 
 TEST_F(LinkedSetTest, MultiplePackets) {
@@ -61,9 +59,8 @@ TEST_F(LinkedSetTest, MultiplePackets) {
     linked_set_.Insert(static_cast<uint16_t>(i), 0, 0, 0);
   }
 
-  // Packets arriving out of order should not affect the following values:
+  // Packets arriving out of order should not affect the following value:
-  EXPECT_EQ(linked_set_.OldestSeqNumber(), 0);
+  EXPECT_EQ(linked_set_.Range(), kNumberPackets);
-  EXPECT_EQ(linked_set_.NewestSeqNumber(), kNumberPackets - 1);
 }
 
 TEST_F(LinkedSetTest, Overflow) {
@@ -75,32 +72,29 @@ TEST_F(LinkedSetTest, Overflow) {
     linked_set_.Insert(static_cast<uint16_t>(i), 0, 0, 0);
   }
 
-  // Packets arriving out of order should not affect the following values:
+  // Wrapping shouldn't matter
-  EXPECT_EQ(linked_set_.OldestSeqNumber(),
+  EXPECT_EQ(linked_set_.Range(), kLastSeqNumber - kFirstSeqNumber + 1);
-            static_cast<uint16_t>(kFirstSeqNumber));
-  EXPECT_EQ(linked_set_.NewestSeqNumber(),
-            static_cast<uint16_t>(kLastSeqNumber));
 }
 
-class SequenceNumberOlderThanTest : public ::testing::Test {
+TEST_F(LinkedSetTest, SameSequenceNumbers) {
- public:
+  // Test correct behavior when
-  SequenceNumberOlderThanTest() {}
+  // sequence numbers wrap (after 0xFFFF).
-  ~SequenceNumberOlderThanTest() {}
 
- protected:
+  // Choose step such as step*capacity < 0x8000
-  SequenceNumberOlderThan comparator_;
+  // (received packets in a reasonable window)
-};
+  const int kStep = 0x20;
+  // Choose iteration such as step*iteration > 0x10000
+  // (imply wrap)
+  const int kIterations = 0x1000;
 
-TEST_F(SequenceNumberOlderThanTest, Operator) {
+  int kSeqNumber = 1;
-  // Operator()(x, y) returns true <==> y is newer than x.
+  for (int i = 0; i < kIterations; ++i) {
-  EXPECT_TRUE(comparator_.operator()(0x0000, 0x0001));
+    // Other parameters don't matter here.
-  EXPECT_TRUE(comparator_.operator()(0x0001, 0x1000));
+    linked_set_.Insert(static_cast<uint16_t>(kSeqNumber), 0, 0, 0);
-  EXPECT_FALSE(comparator_.operator()(0x0001, 0x0000));
+    kSeqNumber += kStep;
-  EXPECT_FALSE(comparator_.operator()(0x0002, 0x0002));
+  }
-  EXPECT_TRUE(comparator_.operator()(0xFFF6, 0x000A));
+
-  EXPECT_FALSE(comparator_.operator()(0x000A, 0xFFF6));
+  EXPECT_EQ(linked_set_.Range(), (kSetCapacity - 1) * kStep + 1);
-  EXPECT_TRUE(comparator_.operator()(0x0000, 0x8000));
-  EXPECT_FALSE(comparator_.operator()(0x8000, 0x0000));
 }
 
 class LossAccountTest : public ::testing::Test {