Avoid use-after-move in PeerConnection::Set[Local/Remote]Description.
In Set[Local/Remote]Description we have a raw pointer |desc| whose
ownership is passed to a helper function. Before this CL we continue
to use |desc| after ownership is passed under the assumption that the
object is not deleted.
In this CL, we instead rely on [local/remote]_description() after the
helper function has been called. In practice, this is a pointer to
the same object, but it removes the assumption about |desc| being
valid after its ownership is passed.
Bug: None
Change-Id: I144a190ea00f303f4713b64c45aa3e811c0f4b2e
Reviewed-on: https://webrtc-review.googlesource.com/21320
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20654}diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index 3ea4a3f..e8857a3 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -1423,7 +1423,7 @@
std::unique_ptr<SessionDescriptionInterface> desc_temp(desc);
if (IsClosed()) {
- std::string error = "Failed to set local " + desc->type() +
+ std::string error = "Failed to set local " + desc_temp->type() +
" sdp: Called in wrong state: STATE_CLOSED";
RTC_LOG(LS_ERROR) << error;
PostSetSessionDescriptionFailure(observer, error);
@@ -1434,10 +1434,13 @@
// streams that might be removed by updating the session description.
stats_->UpdateStats(kStatsOutputLevelStandard);
std::string error;
+ // Takes the ownership of |desc_temp|. On success, local_description() is
+ // updated to reflect the description that was passed in.
if (!SetLocalDescription(std::move(desc_temp), &error)) {
PostSetSessionDescriptionFailure(observer, error);
return;
}
+ RTC_DCHECK(local_description());
// If setting the description decided our SSL role, allocate any necessary
// SCTP sids.
@@ -1449,7 +1452,7 @@
// Update state and SSRC of local MediaStreams and DataChannels based on the
// local session description.
const cricket::ContentInfo* audio_content =
- GetFirstAudioContent(desc->description());
+ GetFirstAudioContent(local_description()->description());
if (audio_content) {
if (audio_content->rejected) {
RemoveTracks(cricket::MEDIA_TYPE_AUDIO);
@@ -1462,7 +1465,7 @@
}
const cricket::ContentInfo* video_content =
- GetFirstVideoContent(desc->description());
+ GetFirstVideoContent(local_description()->description());
if (video_content) {
if (video_content->rejected) {
RemoveTracks(cricket::MEDIA_TYPE_VIDEO);
@@ -1475,7 +1478,7 @@
}
const cricket::ContentInfo* data_content =
- GetFirstDataContent(desc->description());
+ GetFirstDataContent(local_description()->description());
if (data_content) {
const cricket::DataContentDescription* data_desc =
static_cast<const cricket::DataContentDescription*>(
@@ -1500,7 +1503,7 @@
// before signaling that SetLocalDescription completed.
transport_controller_->MaybeStartGathering();
- if (desc->type() == SessionDescriptionInterface::kAnswer) {
+ if (local_description()->type() == SessionDescriptionInterface::kAnswer) {
// TODO(deadbeef): We already had to hop to the network thread for
// MaybeStartGathering...
network_thread()->Invoke<void>(
@@ -1527,7 +1530,7 @@
std::unique_ptr<SessionDescriptionInterface> desc_temp(desc);
if (IsClosed()) {
- std::string error = "Failed to set remote " + desc->type() +
+ std::string error = "Failed to set remote " + desc_temp->type() +
" sdp: Called in wrong state: STATE_CLOSED";
RTC_LOG(LS_ERROR) << error;
PostSetSessionDescriptionFailure(observer, error);
@@ -1538,10 +1541,13 @@
// streams that might be removed by updating the session description.
stats_->UpdateStats(kStatsOutputLevelStandard);
std::string error;
+ // Takes the ownership of |desc_temp|. On success, remote_description() is
+ // updated to reflect the description that was passed in.
if (!SetRemoteDescription(std::move(desc_temp), &error)) {
PostSetSessionDescriptionFailure(observer, error);
return;
}
+ RTC_DCHECK(remote_description());
// If setting the description decided our SSL role, allocate any necessary
// SCTP sids.
@@ -1550,19 +1556,20 @@
AllocateSctpSids(role);
}
- const cricket::SessionDescription* remote_desc = desc->description();
- const cricket::ContentInfo* audio_content = GetFirstAudioContent(remote_desc);
- const cricket::ContentInfo* video_content = GetFirstVideoContent(remote_desc);
+ const cricket::ContentInfo* audio_content =
+ GetFirstAudioContent(remote_description()->description());
+ const cricket::ContentInfo* video_content =
+ GetFirstVideoContent(remote_description()->description());
const cricket::AudioContentDescription* audio_desc =
- GetFirstAudioContentDescription(remote_desc);
+ GetFirstAudioContentDescription(remote_description()->description());
const cricket::VideoContentDescription* video_desc =
- GetFirstVideoContentDescription(remote_desc);
+ GetFirstVideoContentDescription(remote_description()->description());
const cricket::DataContentDescription* data_desc =
- GetFirstDataContentDescription(remote_desc);
+ GetFirstDataContentDescription(remote_description()->description());
// Check if the descriptions include streams, just in case the peer supports
// MSID, but doesn't indicate so with "a=msid-semantic".
- if (remote_desc->msid_supported() ||
+ if (remote_description()->description()->msid_supported() ||
(audio_desc && !audio_desc->streams().empty()) ||
(video_desc && !video_desc->streams().empty())) {
remote_peer_supports_msid_ = true;
@@ -1631,7 +1638,7 @@
signaling_thread()->Post(RTC_FROM_HERE, this,
MSG_SET_SESSIONDESCRIPTION_SUCCESS, msg);
- if (desc->type() == SessionDescriptionInterface::kAnswer) {
+ if (remote_description()->type() == SessionDescriptionInterface::kAnswer) {
// TODO(deadbeef): We already had to hop to the network thread for
// MaybeStartGathering...
network_thread()->Invoke<void>(