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

Add nullptr check in SctpTransport.

Browse Source 
In previous implementation, the SctpTransport always assumes the
DtlsTransport underneath is non-null, which is not true after switching
to new JsepTransportController model.

This CL adds nullptr when connecting/disconnecting the SctpTransport with
the DtlsTransport.

The "channel" related methods and variables are also renamed.

Bug: chromium:827917, chromium:828220
Change-Id: I95aa2900d23b0885f45500e2c53def771abdccad
Reviewed-on: https://webrtc-review.googlesource.com/66160
Commit-Queue: Zhi Huang <zhihuang@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22700}
This commit is contained in:
Zhi Huang 2018-04-02 19:16:26 -07:00 committed by Commit Bot
parent 5b4f075f9c
commit 644fde40a9
7 changed files with 108 additions and 74 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

74
media/sctp/sctptransport.cc
View File

@ -361,8 +361,8 @@ class SctpTransport::UsrSctpWrapper {
// CopyOnWriteBuffer is the most convenient way to do this.
transport->invoker_.AsyncInvoke<void>(
RTC_FROM_HERE, transport->network_thread_,
rtc::Bind(&SctpTransport::OnInboundPacketFromSctpToChannel, transport,
buffer, params, flags));
rtc::Bind(&SctpTransport::OnInboundPacketFromSctpToTransport,
transport, buffer, params, flags));
}
free(data);
return 1;
@ -405,14 +405,14 @@ class SctpTransport::UsrSctpWrapper {
};
SctpTransport::SctpTransport(rtc::Thread* network_thread,
rtc::PacketTransportInternal* channel)
rtc::PacketTransportInternal* transport)
: network_thread_(network_thread),
transport_channel_(channel),
was_ever_writable_(channel->writable()) {
transport_(transport),
was_ever_writable_(transport->writable()) {
RTC_DCHECK(network_thread_);
RTC_DCHECK(transport_channel_);
RTC_DCHECK(transport_);
RTC_DCHECK_RUN_ON(network_thread_);
ConnectTransportChannelSignals();
ConnectTransportSignals();
}
SctpTransport::~SctpTransport() {
@ -420,15 +420,14 @@ SctpTransport::~SctpTransport() {
CloseSctpSocket();
}
void SctpTransport::SetTransportChannel(rtc::PacketTransportInternal* channel) {
void SctpTransport::SetDtlsTransport(rtc::PacketTransportInternal* transport) {
RTC_DCHECK_RUN_ON(network_thread_);
RTC_DCHECK(channel);
DisconnectTransportChannelSignals();
transport_channel_ = channel;
ConnectTransportChannelSignals();
if (!was_ever_writable_ && channel->writable()) {
DisconnectTransportSignals();
transport_ = transport;
ConnectTransportSignals();
if (!was_ever_writable_ && transport && transport->writable()) {
was_ever_writable_ = true;
// New channel is writable, now we can start the SCTP connection if Start
// New transport is writable, now we can start the SCTP connection if Start
// was called already.
if (started_) {
RTC_DCHECK(!sock_);
@ -457,7 +456,7 @@ bool SctpTransport::Start(int local_sctp_port, int remote_sctp_port) {
remote_port_ = remote_sctp_port;
started_ = true;
RTC_DCHECK(!sock_);
// Only try to connect if the DTLS channel has been writable before
// Only try to connect if the DTLS transport has been writable before
// (indicating that the DTLS handshake is complete).
if (was_ever_writable_) {
return Connect();
@ -591,18 +590,23 @@ bool SctpTransport::ReadyToSendData() {
return ready_to_send_data_;
}
void SctpTransport::ConnectTransportChannelSignals() {
void SctpTransport::ConnectTransportSignals() {
RTC_DCHECK_RUN_ON(network_thread_);
transport_channel_->SignalWritableState.connect(
this, &SctpTransport::OnWritableState);
transport_channel_->SignalReadPacket.connect(this,
&SctpTransport::OnPacketRead);
if (!transport_) {
return;
}
transport_->SignalWritableState.connect(this,
&SctpTransport::OnWritableState);
transport_->SignalReadPacket.connect(this, &SctpTransport::OnPacketRead);
}
void SctpTransport::DisconnectTransportChannelSignals() {
void SctpTransport::DisconnectTransportSignals() {
RTC_DCHECK_RUN_ON(network_thread_);
transport_channel_->SignalWritableState.disconnect(this);
transport_channel_->SignalReadPacket.disconnect(this);
if (!transport_) {
return;
}
transport_->SignalWritableState.disconnect(this);
transport_->SignalReadPacket.disconnect(this);
}
bool SctpTransport::Connect() {
@ -836,7 +840,7 @@ void SctpTransport::SetReadyToSendData() {
void SctpTransport::OnWritableState(rtc::PacketTransportInternal* transport) {
RTC_DCHECK_RUN_ON(network_thread_);
RTC_DCHECK_EQ(transport_channel_, transport);
RTC_DCHECK_EQ(transport_, transport);
if (!was_ever_writable_ && transport->writable()) {
was_ever_writable_ = true;
if (started_) {
@ -852,7 +856,7 @@ void SctpTransport::OnPacketRead(rtc::PacketTransportInternal* transport,
const rtc::PacketTime& packet_time,
int flags) {
RTC_DCHECK_RUN_ON(network_thread_);
RTC_DCHECK_EQ(transport_channel_, transport);
RTC_DCHECK_EQ(transport_, transport);
TRACE_EVENT0("webrtc", "SctpTransport::OnPacketRead");
if (flags & PF_SRTP_BYPASS) {
@ -906,24 +910,24 @@ void SctpTransport::OnPacketFromSctpToNetwork(
}
TRACE_EVENT0("webrtc", "SctpTransport::OnPacketFromSctpToNetwork");
// Don't create noise by trying to send a packet when the DTLS channel isn't
// Don't create noise by trying to send a packet when the DTLS transport isn't
// even writable.
if (!transport_channel_->writable()) {
if (!transport_ || !transport_->writable()) {
return;
}
// Bon voyage.
transport_channel_->SendPacket(buffer.data<char>(), buffer.size(),
rtc::PacketOptions(), PF_NORMAL);
transport_->SendPacket(buffer.data<char>(), buffer.size(),
rtc::PacketOptions(), PF_NORMAL);
}
void SctpTransport::OnInboundPacketFromSctpToChannel(
void SctpTransport::OnInboundPacketFromSctpToTransport(
const rtc::CopyOnWriteBuffer& buffer,
ReceiveDataParams params,
int flags) {
RTC_DCHECK_RUN_ON(network_thread_);
RTC_LOG(LS_VERBOSE) << debug_name_
<< "->OnInboundPacketFromSctpToChannel(...): "
<< "->OnInboundPacketFromSctpToTransport(...): "
<< "Received SCTP data:"
<< " sid=" << params.sid
<< " notification: " << (flags & MSG_NOTIFICATION)
@ -932,22 +936,22 @@ void SctpTransport::OnInboundPacketFromSctpToChannel(
// connection" message. This sets sock_ = NULL;
if (!buffer.size() || !buffer.data()) {
RTC_LOG(LS_INFO) << debug_name_
<< "->OnInboundPacketFromSctpToChannel(...): "
<< "->OnInboundPacketFromSctpToTransport(...): "
"No data, closing.";
return;
}
if (flags & MSG_NOTIFICATION) {
OnNotificationFromSctp(buffer);
} else {
OnDataFromSctpToChannel(params, buffer);
OnDataFromSctpToTransport(params, buffer);
}
}
void SctpTransport::OnDataFromSctpToChannel(
void SctpTransport::OnDataFromSctpToTransport(
const ReceiveDataParams& params,
const rtc::CopyOnWriteBuffer& buffer) {
RTC_DCHECK_RUN_ON(network_thread_);
RTC_LOG(LS_VERBOSE) << debug_name_ << "->OnDataFromSctpToChannel(...): "
RTC_LOG(LS_VERBOSE) << debug_name_ << "->OnDataFromSctpToTransport(...): "
<< "Posting with length: " << buffer.size()
<< " on stream " << params.sid;
// Reports all received messages to upper layers, no matter whether the sid

31
media/sctp/sctptransport.h
View File

@ -46,15 +46,15 @@ struct SctpInboundPacket;
// 3. OnSctpOutboundPacket(wrapped_data)
// [sctp thread returns having async invoked on the network thread]
// 4. SctpTransport::OnPacketFromSctpToNetwork(wrapped_data)
// 5. TransportChannel::SendPacket(wrapped_data)
// 5. DtlsTransport::SendPacket(wrapped_data)
// 6. ... across network ... a packet is sent back ...
// 7. SctpTransport::OnPacketReceived(wrapped_data)
// 8. usrsctp_conninput(wrapped_data)
// [network thread returns; sctp thread then calls the following]
// 9. OnSctpInboundData(data)
// [sctp thread returns having async invoked on the network thread]
// 10. SctpTransport::OnInboundPacketFromSctpToChannel(inboundpacket)
// 11. SctpTransport::OnDataFromSctpToChannel(data)
// 10. SctpTransport::OnInboundPacketFromSctpToTransport(inboundpacket)
// 11. SctpTransport::OnDataFromSctpToTransport(data)
// 12. SctpTransport::SignalDataReceived(data)
// [from the same thread, methods registered/connected to
// SctpTransport are called with the recieved data]
@ -71,7 +71,7 @@ class SctpTransport : public SctpTransportInternal,
~SctpTransport() override;
// SctpTransportInternal overrides (see sctptransportinternal.h for comments).
void SetTransportChannel(rtc::PacketTransportInternal* channel) override;
void SetDtlsTransport(rtc::PacketTransportInternal* transport) override;
bool Start(int local_port, int remote_port) override;
bool OpenStream(int sid) override;
bool ResetStream(int sid) override;
@ -88,8 +88,8 @@ class SctpTransport : public SctpTransportInternal,
rtc::Thread* network_thread() const { return network_thread_; }
private:
void ConnectTransportChannelSignals();
void DisconnectTransportChannelSignals();
void ConnectTransportSignals();
void DisconnectTransportSignals();
// Creates the socket and connects.
bool Connect();
@ -124,11 +124,11 @@ class SctpTransport : public SctpTransportInternal,
// Called using |invoker_| to decide what to do with the packet.
// The |flags| parameter is used by SCTP to distinguish notification packets
// from other types of packets.
void OnInboundPacketFromSctpToChannel(const rtc::CopyOnWriteBuffer& buffer,
ReceiveDataParams params,
int flags);
void OnDataFromSctpToChannel(const ReceiveDataParams& params,
const rtc::CopyOnWriteBuffer& buffer);
void OnInboundPacketFromSctpToTransport(const rtc::CopyOnWriteBuffer& buffer,
ReceiveDataParams params,
int flags);
void OnDataFromSctpToTransport(const ReceiveDataParams& params,
const rtc::CopyOnWriteBuffer& buffer);
void OnNotificationFromSctp(const rtc::CopyOnWriteBuffer& buffer);
void OnNotificationAssocChange(const sctp_assoc_change& change);
@ -140,7 +140,7 @@ class SctpTransport : public SctpTransportInternal,
// Helps pass inbound/outbound packets asynchronously to the network thread.
rtc::AsyncInvoker invoker_;
// Underlying DTLS channel.
rtc::PacketTransportInternal* transport_channel_;
rtc::PacketTransportInternal* transport_;
bool was_ever_writable_ = false;
int local_port_ = kSctpDefaultPort;
int remote_port_ = kSctpDefaultPort;
@ -149,8 +149,7 @@ class SctpTransport : public SctpTransportInternal,
// Has Start been called? Don't create SCTP socket until it has.
bool started_ = false;
// Are we ready to queue data (SCTP socket created, and not blocked due to
// congestion control)? Different than |transport_channel_|'s "ready to
// send".
// congestion control)? Different than |transport_|'s "ready to send".
bool ready_to_send_data_ = false;
typedef std::set<uint32_t> StreamSet;
@ -179,9 +178,9 @@ class SctpTransportFactory : public SctpTransportInternalFactory {
: network_thread_(network_thread) {}
std::unique_ptr<SctpTransportInternal> CreateSctpTransport(
rtc::PacketTransportInternal* channel) override {
rtc::PacketTransportInternal* transport) override {
return std::unique_ptr<SctpTransportInternal>(
new SctpTransport(network_thread_, channel));
new SctpTransport(network_thread_, transport));
}
private:

22
media/sctp/sctptransport_unittest.cc
View File

@ -238,16 +238,16 @@ class SctpTransportTest : public testing::Test, public sigslot::has_slots<> {
};
// Test that data can be sent end-to-end when an SCTP transport starts with one
// transport channel (which is unwritable), and then switches to another
// channel. A common scenario due to how BUNDLE works.
TEST_F(SctpTransportTest, SwitchTransportChannel) {
// transport (which is unwritable), and then switches to another transport. A
// common scenario due to how BUNDLE works.
TEST_F(SctpTransportTest, SwitchDtlsTransport) {
FakeDtlsTransport black_hole("black hole", 0);
FakeDtlsTransport fake_dtls1("fake dtls 1", 0);
FakeDtlsTransport fake_dtls2("fake dtls 2", 0);
SctpFakeDataReceiver recv1;
SctpFakeDataReceiver recv2;
// Construct transport1 with the "black hole" channel.
// Construct transport1 with the "black hole" transport.
std::unique_ptr<SctpTransport> transport1(
CreateTransport(&black_hole, &recv1));
std::unique_ptr<SctpTransport> transport2(
@ -261,10 +261,10 @@ TEST_F(SctpTransportTest, SwitchTransportChannel) {
transport1->Start(kTransport1Port, kTransport2Port);
transport2->Start(kTransport2Port, kTransport1Port);
// Switch transport1_ to the normal fake_dtls1_ channel.
transport1->SetTransportChannel(&fake_dtls1);
// Switch transport1_ to the normal fake_dtls1_ transport.
transport1->SetDtlsTransport(&fake_dtls1);
// Connect the two fake DTLS channels.
// Connect the two fake DTLS transports.
bool asymmetric = false;
fake_dtls1.SetDestination(&fake_dtls2, asymmetric);
@ -274,6 +274,10 @@ TEST_F(SctpTransportTest, SwitchTransportChannel) {
ASSERT_TRUE(SendData(transport2.get(), 1, "bar", &result));
EXPECT_TRUE_WAIT(ReceivedData(&recv2, 1, "foo"), kDefaultTimeout);
EXPECT_TRUE_WAIT(ReceivedData(&recv1, 1, "bar"), kDefaultTimeout);
// Setting a null DtlsTransport should work. This could happen when an SCTP
// data section is rejected.
transport1->SetDtlsTransport(nullptr);
}
// Calling Start twice shouldn't do anything bad, if with the same parameters.
@ -317,7 +321,7 @@ TEST_F(SctpTransportTest, NegativeOnePortTreatedAsDefault) {
transport1->Start(kSctpDefaultPort, kSctpDefaultPort);
transport2->Start(-1, -1);
// Connect the two fake DTLS channels.
// Connect the two fake DTLS transports.
bool asymmetric = false;
fake_dtls1.SetDestination(&fake_dtls2, asymmetric);
@ -347,7 +351,7 @@ TEST_F(SctpTransportTest, ResetStreamWithAlreadyResetStreamFails) {
}
// Test that SignalReadyToSendData is fired after Start has been called and the
// DTLS channel is writable.
// DTLS transport is writable.
TEST_F(SctpTransportTest, SignalReadyToSendDataAfterDtlsWritable) {
FakeDtlsTransport fake_dtls("fake dtls", 0);
SctpFakeDataReceiver recv;

10
media/sctp/sctptransportinternal.h
View File

@ -48,17 +48,15 @@ constexpr uint16_t kMinSctpSid = 0;
// usrsctp.h)
const int kSctpDefaultPort = 5000;
// Abstract SctpTransport interface for use internally (by
// PeerConnection/WebRtcSession/etc.). Exists to allow mock/fake SctpTransports
// to be created.
// Abstract SctpTransport interface for use internally (by PeerConnection etc.).
// Exists to allow mock/fake SctpTransports to be created.
class SctpTransportInternal {
public:
virtual ~SctpTransportInternal() {}
// Changes what underlying DTLS channel is uses. Used when switching which
// Changes what underlying DTLS transport is uses. Used when switching which
// bundled transport the SctpTransport uses.
// Assumes |channel| is non-null.
virtual void SetTransportChannel(rtc::PacketTransportInternal* channel) = 0;
virtual void SetDtlsTransport(rtc::PacketTransportInternal* transport) = 0;
// When Start is called, connects as soon as possible; this can be called
// before DTLS completes, in which case the connection will begin when DTLS

10
pc/peerconnection.cc
View File

@ -5533,10 +5533,10 @@ Call::Stats PeerConnection::GetCallStats() {
bool PeerConnection::CreateSctpTransport_n(const std::string& mid) {
RTC_DCHECK(network_thread()->IsCurrent());
RTC_DCHECK(sctp_factory_);
cricket::DtlsTransportInternal* tc =
cricket::DtlsTransportInternal* dtls_transport =
transport_controller_->GetDtlsTransport(mid);
RTC_DCHECK(tc);
sctp_transport_ = sctp_factory_->CreateSctpTransport(tc);
RTC_DCHECK(dtls_transport);
sctp_transport_ = sctp_factory_->CreateSctpTransport(dtls_transport);
RTC_DCHECK(sctp_transport_);
sctp_invoker_.reset(new rtc::AsyncInvoker());
sctp_transport_->SignalReadyToSendData.connect(
@ -5546,7 +5546,7 @@ bool PeerConnection::CreateSctpTransport_n(const std::string& mid) {
sctp_transport_->SignalStreamClosedRemotely.connect(
this, &PeerConnection::OnSctpStreamClosedRemotely_n);
sctp_mid_ = mid;
sctp_transport_->SetTransportChannel(tc);
sctp_transport_->SetDtlsTransport(dtls_transport);
return true;
}
@ -6122,7 +6122,7 @@ void PeerConnection::OnDtlsTransportChanged(
cricket::DtlsTransportInternal* dtls_transport) {
if (sctp_transport_) {
RTC_DCHECK(mid == sctp_mid_);
sctp_transport_->SetTransportChannel(dtls_transport);
sctp_transport_->SetDtlsTransport(dtls_transport);
}
}

33
pc/peerconnectioninterface_unittest.cc
View File

@ -2258,8 +2258,8 @@ TEST_P(PeerConnectionInterfaceTest, DataChannelCloseWhenPeerConnectionClose) {
EXPECT_EQ(DataChannelInterface::kClosed, data2->state());
}
// This test that data channels can be rejected in an answer.
TEST_P(PeerConnectionInterfaceTest, TestRejectDataChannelInAnswer) {
// This tests that RTP data channels can be rejected in an answer.
TEST_P(PeerConnectionInterfaceTest, TestRejectRtpDataChannelInAnswer) {
FakeConstraints constraints;
constraints.SetAllowRtpDataChannels();
CreatePeerConnection(&constraints);
@ -2283,6 +2283,35 @@ TEST_P(PeerConnectionInterfaceTest, TestRejectDataChannelInAnswer) {
EXPECT_EQ(DataChannelInterface::kClosed, offer_channel->state());
}
#ifdef HAVE_SCTP
// This tests that SCTP data channels can be rejected in an answer.
TEST_P(PeerConnectionInterfaceTest, TestRejectSctpDataChannelInAnswer)
#else
TEST_P(PeerConnectionInterfaceTest, DISABLED_TestRejectSctpDataChannelInAnswer)
#endif
{
FakeConstraints constraints;
CreatePeerConnection(&constraints);
rtc::scoped_refptr<DataChannelInterface> offer_channel(
pc_->CreateDataChannel("offer_channel", NULL));
CreateOfferAsLocalDescription();
// Create an answer where the m-line for data channels are rejected.
std::string sdp;
EXPECT_TRUE(pc_->local_description()->ToString(&sdp));
std::unique_ptr<SessionDescriptionInterface> answer(
webrtc::CreateSessionDescription(SdpType::kAnswer, sdp));
ASSERT_TRUE(answer);
cricket::ContentInfo* data_info =
cricket::GetFirstDataContent(answer->description());
data_info->rejected = true;
DoSetRemoteDescription(std::move(answer));
EXPECT_EQ(DataChannelInterface::kClosed, offer_channel->state());
}
// Test that we can create a session description from an SDP string from
// FireFox, use it as a remote session description, generate an answer and use
// the answer as a local description.

2
pc/test/fakesctptransport.h
View File

@ -20,7 +20,7 @@
// local/remote ports.
class FakeSctpTransport : public cricket::SctpTransportInternal {
public:
void SetTransportChannel(rtc::PacketTransportInternal* channel) override {}
void SetDtlsTransport(rtc::PacketTransportInternal* transport) override {}
bool Start(int local_port, int remote_port) override {
local_port_.emplace(local_port);
remote_port_.emplace(remote_port);