Fix potential bug due to malformed input
A reasonable amount of incoming packets could generate feedback
for millions of packets.
Bug: chromium:949020
Change-Id: I7f3e6b75b683af5b2732c472cc92c6788540486b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/131333
Commit-Queue: Johannes Kron <kron@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27481}diff --git a/call/call.cc b/call/call.cc
index 1e8d3a9..38a4dd5 100644
--- a/call/call.cc
+++ b/call/call.cc
@@ -69,12 +69,12 @@
namespace webrtc {
namespace {
-bool SendFeedbackOnRequestOnly(const std::vector<RtpExtension>& extensions) {
+bool SendPeriodicFeedback(const std::vector<RtpExtension>& extensions) {
for (const auto& extension : extensions) {
if (extension.uri == RtpExtension::kTransportSequenceNumberV2Uri)
- return true;
+ return false;
}
- return false;
+ return true;
}
// TODO(nisse): This really begs for a shared context struct.
@@ -931,8 +931,8 @@
TRACE_EVENT0("webrtc", "Call::CreateVideoReceiveStream");
RTC_DCHECK_CALLED_SEQUENTIALLY(&configuration_sequence_checker_);
- receive_side_cc_.SetSendFeedbackOnRequestOnly(
- SendFeedbackOnRequestOnly(configuration.rtp.extensions));
+ receive_side_cc_.SetSendPeriodicFeedback(
+ SendPeriodicFeedback(configuration.rtp.extensions));
RegisterRateObserver();
diff --git a/modules/congestion_controller/include/receive_side_congestion_controller.h b/modules/congestion_controller/include/receive_side_congestion_controller.h
index 5532b3c..fdf210f 100644
--- a/modules/congestion_controller/include/receive_side_congestion_controller.h
+++ b/modules/congestion_controller/include/receive_side_congestion_controller.h
@@ -38,7 +38,7 @@
size_t payload_size,
const RTPHeader& header);
- void SetSendFeedbackOnRequestOnly(bool send_feedback_on_request_only);
+ void SetSendPeriodicFeedback(bool send_periodic_feedback);
// TODO(nisse): Delete these methods, design a more specific interface.
virtual RemoteBitrateEstimator* GetRemoteBitrateEstimator(bool send_side_bwe);
virtual const RemoteBitrateEstimator* GetRemoteBitrateEstimator(
diff --git a/modules/congestion_controller/receive_side_congestion_controller.cc b/modules/congestion_controller/receive_side_congestion_controller.cc
index 1a95f8c..f886b8e 100644
--- a/modules/congestion_controller/receive_side_congestion_controller.cc
+++ b/modules/congestion_controller/receive_side_congestion_controller.cc
@@ -139,10 +139,9 @@
}
}
-void ReceiveSideCongestionController::SetSendFeedbackOnRequestOnly(
- bool send_feedback_on_request_only) {
- remote_estimator_proxy_.SetSendFeedbackOnRequestOnly(
- send_feedback_on_request_only);
+void ReceiveSideCongestionController::SetSendPeriodicFeedback(
+ bool send_periodic_feedback) {
+ remote_estimator_proxy_.SetSendPeriodicFeedback(send_periodic_feedback);
}
RemoteBitrateEstimator*
diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
index 009f867..5220d15 100644
--- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
+++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
@@ -43,7 +43,7 @@
media_ssrc_(0),
feedback_packet_count_(0),
send_interval_ms_(kDefaultSendIntervalMs),
- send_feedback_on_request_only_(false) {}
+ send_periodic_feedback_(true) {}
RemoteEstimatorProxy::~RemoteEstimatorProxy() {}
@@ -69,7 +69,7 @@
int64_t RemoteEstimatorProxy::TimeUntilNextProcess() {
rtc::CritScope cs(&lock_);
- if (send_feedback_on_request_only_) {
+ if (!send_periodic_feedback_) {
// Wait a day until next process.
return 24 * 60 * 60 * 1000;
} else if (last_process_time_ms_ != -1) {
@@ -82,7 +82,7 @@
void RemoteEstimatorProxy::Process() {
rtc::CritScope cs(&lock_);
- if (send_feedback_on_request_only_) {
+ if (!send_periodic_feedback_) {
return;
}
last_process_time_ms_ = clock_->TimeInMilliseconds();
@@ -109,10 +109,10 @@
rtc::SafeClamp(0.05 * bitrate_bps, kMinTwccRate, kMaxTwccRate));
}
-void RemoteEstimatorProxy::SetSendFeedbackOnRequestOnly(
- bool send_feedback_on_request_only) {
+void RemoteEstimatorProxy::SetSendPeriodicFeedback(
+ bool send_periodic_feedback) {
rtc::CritScope cs(&lock_);
- send_feedback_on_request_only_ = send_feedback_on_request_only;
+ send_periodic_feedback_ = send_periodic_feedback;
}
void RemoteEstimatorProxy::OnPacketArrival(
@@ -126,12 +126,7 @@
int64_t seq = unwrapper_.Unwrap(sequence_number);
- if (send_feedback_on_request_only_) {
- // Remove old packet arrival times.
- auto clear_to_it =
- packet_arrival_times_.lower_bound(seq - kMaxNumberOfPackets);
- packet_arrival_times_.erase(packet_arrival_times_.begin(), clear_to_it);
- } else {
+ if (send_periodic_feedback_) {
if (periodic_window_start_seq_ &&
packet_arrival_times_.lower_bound(*periodic_window_start_seq_) ==
packet_arrival_times_.end()) {
@@ -153,6 +148,20 @@
packet_arrival_times_[seq] = arrival_time;
+ // Limit the range of sequence numbers to send feedback for.
+ auto first_arrival_time_to_keep = packet_arrival_times_.lower_bound(
+ packet_arrival_times_.rbegin()->first - kMaxNumberOfPackets);
+ if (first_arrival_time_to_keep != packet_arrival_times_.begin()) {
+ packet_arrival_times_.erase(packet_arrival_times_.begin(),
+ first_arrival_time_to_keep);
+ if (send_periodic_feedback_) {
+ // |packet_arrival_times_| cannot be empty since we just added one element
+ // and the last element is not deleted.
+ RTC_DCHECK(!packet_arrival_times_.empty());
+ periodic_window_start_seq_ = packet_arrival_times_.begin()->first;
+ }
+ }
+
if (feedback_request) {
// Send feedback packet immediately.
SendFeedbackOnRequest(seq, *feedback_request);
diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.h b/modules/remote_bitrate_estimator/remote_estimator_proxy.h
index d13e1a1..fa9f7c8 100644
--- a/modules/remote_bitrate_estimator/remote_estimator_proxy.h
+++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.h
@@ -51,7 +51,7 @@
int64_t TimeUntilNextProcess() override;
void Process() override;
void OnBitrateChanged(int bitrate);
- void SetSendFeedbackOnRequestOnly(bool send_feedback_on_request_only);
+ void SetSendPeriodicFeedback(bool send_periodic_feedback);
private:
static const int kMaxNumberOfPackets;
@@ -86,7 +86,7 @@
// Map unwrapped seq -> time.
std::map<int64_t, int64_t> packet_arrival_times_ RTC_GUARDED_BY(&lock_);
int64_t send_interval_ms_ RTC_GUARDED_BY(&lock_);
- bool send_feedback_on_request_only_ RTC_GUARDED_BY(&lock_);
+ bool send_periodic_feedback_ RTC_GUARDED_BY(&lock_);
};
} // namespace webrtc
diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
index ab20d5a..e66b9c9 100644
--- a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
+++ b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
@@ -215,6 +215,60 @@
Process();
}
+TEST_F(RemoteEstimatorProxyTest, HandlesMalformedSequenceNumbers) {
+ // This test generates incoming packets with large jumps in sequence numbers.
+ // When unwrapped, the sequeunce numbers of these 30 incoming packets, will
+ // span a range of roughly 650k packets. Test that we only send feedback for
+ // the last packets. Test for regression found in chromium:949020.
+ const int64_t kDeltaMs = 1000;
+ for (int i = 0; i < 10; ++i) {
+ IncomingPacket(kBaseSeq + i, kBaseTimeMs + 3 * i * kDeltaMs);
+ IncomingPacket(kBaseSeq + 20000 + i, kBaseTimeMs + (3 * i + 1) * kDeltaMs);
+ IncomingPacket(kBaseSeq + 40000 + i, kBaseTimeMs + (3 * i + 2) * kDeltaMs);
+ }
+
+ // Only expect feedback for the last two packets.
+ EXPECT_CALL(router_, SendTransportFeedback(_))
+ .WillOnce(Invoke([&](rtcp::TransportFeedback* feedback_packet) {
+ EXPECT_EQ(kBaseSeq + 20000 + 9, feedback_packet->GetBaseSequence());
+ EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc());
+ EXPECT_THAT(SequenceNumbers(*feedback_packet),
+ ElementsAre(kBaseSeq + 20009, kBaseSeq + 40009));
+ EXPECT_THAT(TimestampsMs(*feedback_packet),
+ ElementsAre(kBaseTimeMs + 28 * kDeltaMs,
+ kBaseTimeMs + 29 * kDeltaMs));
+ return true;
+ }));
+
+ Process();
+}
+
+TEST_F(RemoteEstimatorProxyTest, HandlesBackwardsWrappingSequenceNumbers) {
+ // This test is like HandlesMalformedSequenceNumbers but for negative wrap
+ // arounds. Test that we only send feedback for the packets with highest
+ // sequence numbers. Test for regression found in chromium:949020.
+ const int64_t kDeltaMs = 1000;
+ for (int i = 0; i < 10; ++i) {
+ IncomingPacket(kBaseSeq + i, kBaseTimeMs + 3 * i * kDeltaMs);
+ IncomingPacket(kBaseSeq + 40000 + i, kBaseTimeMs + (3 * i + 1) * kDeltaMs);
+ IncomingPacket(kBaseSeq + 20000 + i, kBaseTimeMs + (3 * i + 2) * kDeltaMs);
+ }
+
+ // Only expect feedback for the first two packets.
+ EXPECT_CALL(router_, SendTransportFeedback(_))
+ .WillOnce(Invoke([&](rtcp::TransportFeedback* feedback_packet) {
+ EXPECT_EQ(kBaseSeq + 40000, feedback_packet->GetBaseSequence());
+ EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc());
+ EXPECT_THAT(SequenceNumbers(*feedback_packet),
+ ElementsAre(kBaseSeq + 40000, kBaseSeq));
+ EXPECT_THAT(TimestampsMs(*feedback_packet),
+ ElementsAre(kBaseTimeMs + kDeltaMs, kBaseTimeMs));
+ return true;
+ }));
+
+ Process();
+}
+
TEST_F(RemoteEstimatorProxyTest, ResendsTimestampsOnReordering) {
IncomingPacket(kBaseSeq, kBaseTimeMs);
IncomingPacket(kBaseSeq + 2, kBaseTimeMs + 2);
@@ -348,19 +402,19 @@
//////////////////////////////////////////////////////////////////////////////
typedef RemoteEstimatorProxyTest RemoteEstimatorProxyOnRequestTest;
TEST_F(RemoteEstimatorProxyOnRequestTest, TimeUntilNextProcessIsHigh) {
- proxy_.SetSendFeedbackOnRequestOnly(true);
+ proxy_.SetSendPeriodicFeedback(false);
EXPECT_GE(proxy_.TimeUntilNextProcess(), 60 * 60 * 1000);
}
TEST_F(RemoteEstimatorProxyOnRequestTest, ProcessDoesNotSendFeedback) {
- proxy_.SetSendFeedbackOnRequestOnly(true);
+ proxy_.SetSendPeriodicFeedback(false);
IncomingPacket(kBaseSeq, kBaseTimeMs);
EXPECT_CALL(router_, SendTransportFeedback(_)).Times(0);
Process();
}
TEST_F(RemoteEstimatorProxyOnRequestTest, RequestSinglePacketFeedback) {
- proxy_.SetSendFeedbackOnRequestOnly(true);
+ proxy_.SetSendPeriodicFeedback(false);
IncomingPacket(kBaseSeq, kBaseTimeMs);
IncomingPacket(kBaseSeq + 1, kBaseTimeMs + kMaxSmallDeltaMs);
IncomingPacket(kBaseSeq + 2, kBaseTimeMs + 2 * kMaxSmallDeltaMs);
@@ -384,7 +438,7 @@
}
TEST_F(RemoteEstimatorProxyOnRequestTest, RequestLastFivePacketFeedback) {
- proxy_.SetSendFeedbackOnRequestOnly(true);
+ proxy_.SetSendPeriodicFeedback(false);
int i = 0;
for (; i < 10; ++i) {
IncomingPacket(kBaseSeq + i, kBaseTimeMs + i * kMaxSmallDeltaMs);
@@ -415,7 +469,7 @@
TEST_F(RemoteEstimatorProxyOnRequestTest,
RequestLastFivePacketFeedbackMissingPackets) {
- proxy_.SetSendFeedbackOnRequestOnly(true);
+ proxy_.SetSendPeriodicFeedback(false);
int i = 0;
for (; i < 10; ++i) {
if (i != 7 && i != 9)
