admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix potential bug due to malformed input

Browse Source 
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}
This commit is contained in:
Johannes Kron 2019-04-08 12:57:06 +02:00 committed by Commit Bot
parent 1e2d436cbf
commit f59666b3b2
6 changed files with 91 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
call/call.cc
View File

@ -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 @@ webrtc::VideoReceiveStream* Call::CreateVideoReceiveStream(
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();

2
modules/congestion_controller/include/receive_side_congestion_controller.h
View File

@ -38,7 +38,7 @@ class ReceiveSideCongestionController : public CallStatsObserver,
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(

7
modules/congestion_controller/receive_side_congestion_controller.cc
View File

@ -139,10 +139,9 @@ void ReceiveSideCongestionController::OnReceivedPacket(
}
}
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*

33
modules/remote_bitrate_estimator/remote_estimator_proxy.cc
View File

@ -43,7 +43,7 @@ RemoteEstimatorProxy::RemoteEstimatorProxy(
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 @@ bool RemoteEstimatorProxy::LatestEstimate(std::vector<unsigned int>* ssrcs,
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 @@ int64_t RemoteEstimatorProxy::TimeUntilNextProcess() {
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 @@ void RemoteEstimatorProxy::OnBitrateChanged(int bitrate_bps) {
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 @@ void RemoteEstimatorProxy::OnPacketArrival(
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 @@ void RemoteEstimatorProxy::OnPacketArrival(
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);

4
modules/remote_bitrate_estimator/remote_estimator_proxy.h
View File

@ -51,7 +51,7 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator {
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 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator {
// 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

64
modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
View File

@ -215,6 +215,60 @@ TEST_F(RemoteEstimatorProxyTest, HandlesReorderingAndWrap) {
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 @@ TEST_F(RemoteEstimatorProxyTest, TwccReportsUse5PercentOfAvailableBandwidth) {
//////////////////////////////////////////////////////////////////////////////
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, RequestSinglePacketFeedback) {
}
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, RequestLastFivePacketFeedback) {
TEST_F(RemoteEstimatorProxyOnRequestTest,
RequestLastFivePacketFeedbackMissingPackets) {
proxy_.SetSendFeedbackOnRequestOnly(true);
proxy_.SetSendPeriodicFeedback(false);
int i = 0;
for (; i < 10; ++i) {
if (i != 7 && i != 9)