zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fixing off-by-one error with max SCTP id.

Browse Source 
Normally, when creating a data channel with an out-of-range ID,
createDataChannel returns nullptr. But due to an off-by-one
error, creating a data channel with ID 1023 returns a data channel
that silently fails later.

This probably occurred because it wasn't clear whether "kMaxSctpSid" was an
inclusive or exclusive maximum, so I changed the value to
"kMaxSctpStreams". This wasn't caught by unit tests because the
off-by-one error persisted to the unit tests as well.

Also getting rid of some dead code. We were adding SCTP streams to the
ContentDescription object but they weren't being used.

BUG=619849
R=pthatcher@webrtc.org, skvlad@webrtc.org

Review URL: https://codereview.webrtc.org/2254003002 .

Cr-Commit-Position: refs/heads/master@{#13906}
This commit is contained in:
Taylor Brandstetter
2016-08-24 13:15:27 -07:00
parent fcada90485
commit 1d7a637340
6 changed files with 33 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
webrtc/api/datachannel.cc
View File

@ -57,7 +57,8 @@ void SctpSidAllocator::ReleaseSid(int sid) {
}
bool SctpSidAllocator::IsSidAvailable(int sid) const {
if (sid < 0 || sid > static_cast<int>(cricket::kMaxSctpSid)) {
if (sid < static_cast<int>(cricket::kMinSctpSid) ||
sid > static_cast<int>(cricket::kMaxSctpSid)) {
return false;
}
return used_sids_.find(sid) == used_sids_.end();

2
webrtc/api/webrtcsdp.cc
View File

@ -1391,7 +1391,7 @@ void BuildSctpContentAttributes(std::string* message, int sctp_port) {
InitAttrLine(kAttributeSctpmap, &os);
os << kSdpDelimiterColon << sctp_port << kSdpDelimiterSpace
<< kDefaultSctpmapProtocol << kSdpDelimiterSpace
<< (cricket::kMaxSctpSid + 1);
<< cricket::kMaxSctpStreams;
AddLine(os.str(), message);
}

18
webrtc/media/sctp/sctpdataengine.cc
View File

@ -284,12 +284,9 @@ void InitializeUsrSctp() {
// See: http://svnweb.freebsd.org/base?view=revision&revision=229805
// usrsctp_sysctl_set_sctp_blackhole(2);
// Set the number of default outgoing streams. This is the number we'll
// send in the SCTP INIT message. The 'appropriate default' in the
// second paragraph of
// http://tools.ietf.org/html/draft-ietf-rtcweb-data-channel-05#section-6.2
// is kMaxSctpSid.
usrsctp_sysctl_set_sctp_nr_outgoing_streams_default(kMaxSctpSid);
// Set the number of default outgoing streams. This is the number we'll
// send in the SCTP INIT message.
usrsctp_sysctl_set_sctp_nr_outgoing_streams_default(kMaxSctpStreams);
}
void UninitializeUsrSctp() {
@ -751,23 +748,22 @@ bool SctpDataMediaChannel::AddStream(const StreamParams& stream) {
}
const uint32_t ssrc = stream.first_ssrc();
if (ssrc >= kMaxSctpSid) {
if (ssrc > kMaxSctpSid) {
LOG(LS_WARNING) << debug_name_ << "->Add(Send|Recv)Stream(...): "
<< "Not adding data stream '" << stream.id
<< "' with ssrc=" << ssrc
<< " because stream ssrc is too high.";
<< "' with sid=" << ssrc << " because sid is too high.";
return false;
} else if (open_streams_.find(ssrc) != open_streams_.end()) {
LOG(LS_WARNING) << debug_name_ << "->Add(Send|Recv)Stream(...): "
<< "Not adding data stream '" << stream.id
<< "' with ssrc=" << ssrc
<< "' with sid=" << ssrc
<< " because stream is already open.";
return false;
} else if (queued_reset_streams_.find(ssrc) != queued_reset_streams_.end()
|| sent_reset_streams_.find(ssrc) != sent_reset_streams_.end()) {
LOG(LS_WARNING) << debug_name_ << "->Add(Send|Recv)Stream(...): "
<< "Not adding data stream '" << stream.id
<< "' with ssrc=" << ssrc
<< "' with sid=" << ssrc
<< " because stream is still closing.";
return false;
}

16
webrtc/media/sctp/sctpdataengine.h
View File

@ -37,9 +37,19 @@ struct sctp_stream_reset_event;
// Defined by <sys/socket.h>
struct socket;
namespace cricket {
// The highest stream ID (Sid) that SCTP allows, and the number of streams we
// tell SCTP we're going to use.
const uint32_t kMaxSctpSid = 1023;
// The number of outgoing streams that we'll negotiate. Since stream IDs (SIDs)
// are 0-based, the highest usable SID is 1023.
//
// It's recommended to use the maximum of 65535 in:
// https://tools.ietf.org/html/draft-ietf-rtcweb-data-channel-13#section-6.2
// However, we use 1024 in order to save memory. usrsctp allocates 104 bytes
// for each pair of incoming/outgoing streams (on a 64-bit system), so 65535
// streams would waste ~6MB.
//
// Note: "max" and "min" here are inclusive.
constexpr uint16_t kMaxSctpStreams = 1024;
constexpr uint16_t kMaxSctpSid = kMaxSctpStreams - 1;
constexpr uint16_t kMinSctpSid = 0;
// This is the default SCTP port to use. It is passed along the wire and the
// connectee and connector must be using the same port. It is not related to the

4
webrtc/media/sctp/sctpdataengine_unittest.cc
View File

@ -490,8 +490,8 @@ TEST_F(SctpDataMediaChannelTest, EngineSignalsRightChannel) {
TEST_F(SctpDataMediaChannelTest, RefusesHighNumberedChannels) {
SetupConnectedChannels();
EXPECT_TRUE(AddStream(1022));
EXPECT_FALSE(AddStream(1023));
EXPECT_TRUE(AddStream(kMaxSctpSid));
EXPECT_FALSE(AddStream(kMaxSctpSid + 1));
}
// Flaky, see webrtc:4453.

52
webrtc/pc/mediasession.cc
View File

@ -28,12 +28,6 @@
#include "webrtc/pc/channelmanager.h"
#include "webrtc/pc/srtpfilter.h"
#ifdef HAVE_SCTP
#include "webrtc/media/sctp/sctpdataengine.h"
#else
static const uint32_t kMaxSctpSid = 1023;
#endif
namespace {
const char kInline[] = "inline:";
@ -282,33 +276,6 @@ static void GenerateSsrcs(const StreamParamsVec& params_vec,
}
}
// Returns false if we exhaust the range of SIDs.
static bool GenerateSctpSid(const StreamParamsVec& params_vec, uint32_t* sid) {
if (params_vec.size() > kMaxSctpSid) {
LOG(LS_WARNING) <<
"Could not generate an SCTP SID: too many SCTP streams.";
return false;
}
while (true) {
uint32_t candidate = rtc::CreateRandomNonZeroId() % kMaxSctpSid;
if (!GetStreamBySsrc(params_vec, candidate)) {
*sid = candidate;
return true;
}
}
}
static bool GenerateSctpSids(const StreamParamsVec& params_vec,
std::vector<uint32_t>* sids) {
uint32_t sid;
if (!GenerateSctpSid(params_vec, &sid)) {
LOG(LS_WARNING) << "Could not generated an SCTP SID.";
return false;
}
sids->push_back(sid);
return true;
}
// Finds all StreamParams of all media types and attach them to stream_params.
static void GetCurrentStreamParams(const SessionDescription* sdesc,
StreamParamsVec* stream_params) {
@ -454,6 +421,11 @@ static bool AddStreamParams(MediaType media_type,
StreamParamsVec* current_streams,
MediaContentDescriptionImpl<C>* content_description,
const bool add_legacy_stream) {
// SCTP streams are not negotiated using SDP/ContentDescriptions.
if (IsSctp(content_description)) {
return true;
}
const bool include_rtx_streams =
ContainsRtxCodec(content_description->codecs());
@ -461,12 +433,8 @@ static bool AddStreamParams(MediaType media_type,
if (streams.empty() && add_legacy_stream) {
// TODO(perkj): Remove this legacy stream when all apps use StreamParams.
std::vector<uint32_t> ssrcs;
if (IsSctp(content_description)) {
GenerateSctpSids(*current_streams, &ssrcs);
} else {
int num_ssrcs = include_rtx_streams ? 2 : 1;
GenerateSsrcs(*current_streams, num_ssrcs, &ssrcs);
}
int num_ssrcs = include_rtx_streams ? 2 : 1;
GenerateSsrcs(*current_streams, num_ssrcs, &ssrcs);
if (include_rtx_streams) {
content_description->AddLegacyStream(ssrcs[0], ssrcs[1]);
content_description->set_multistream(true);
@ -489,11 +457,7 @@ static bool AddStreamParams(MediaType media_type,
if (!param) {
// This is a new stream.
std::vector<uint32_t> ssrcs;
if (IsSctp(content_description)) {
GenerateSctpSids(*current_streams, &ssrcs);
} else {
GenerateSsrcs(*current_streams, stream_it->num_sim_layers, &ssrcs);
}
GenerateSsrcs(*current_streams, stream_it->num_sim_layers, &ssrcs);
StreamParams stream_param;
stream_param.id = stream_it->id;
// Add the generated ssrc.