Bug 1405940 - Fix Null Pointer dereference in sigslot::lock_block r?bwc draft
authorMichael Froman <mfroman@mozilla.com>
Thu, 12 Oct 2017 22:38:01 -0500
changeset 682845 853e36f1ddcb065c2e0af9d2cf015f04b40983e1
parent 682254 a29052590fc6538b89641e690d11731ca8e78120
child 736461 dfedede1e0af817e4a30b8a8c7c0d39d316f6cfc
push id85179
push userbmo:mfroman@nostrum.com
push dateWed, 18 Oct 2017 21:12:10 +0000
reviewersbwc
bugs1405940
milestone58.0a1
Bug 1405940 - Fix Null Pointer dereference in sigslot::lock_block r?bwc Caused by several issues: 1) We were allowing an answer with modified ufrag/pass to begin an ICE restart even if the offer didn't indicate it was restarting. 2) This should no longer happen, but in cases where restart logic was started inappropriately, TransportLayerIce::SetParameters could get a null stream, and we check for that now. MozReview-Commit-ID: JFQ1zz3l5wY
media/mtransport/transportlayerice.cpp
media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
media/webrtc/signaling/src/jsep/JsepSessionImpl.h
--- a/media/mtransport/transportlayerice.cpp
+++ b/media/mtransport/transportlayerice.cpp
@@ -94,16 +94,27 @@ TransportLayerIce::TransportLayerIce(con
 
 TransportLayerIce::~TransportLayerIce() {
   // No need to do anything here, since we use smart pointers
 }
 
 void TransportLayerIce::SetParameters(RefPtr<NrIceCtx> ctx,
                                       RefPtr<NrIceMediaStream> stream,
                                       int component) {
+  // Stream could be null in the case of some badly written js that causes
+  // us to be in an ICE restart case, but not have valid streams due to
+  // not calling PeerConnectionMedia::EnsureTransports if
+  // PeerConnectionImpl::SetSignalingState_m thinks the conditions were
+  // not correct.  We also solved a case where an incoming answer was
+  // incorrectly beginning an ICE restart when the offer did not indicate one.
+  if (!stream) {
+    MOZ_ASSERT(false);
+    return;
+  }
+
   // If SetParameters is called and we already have a stream_, this means
   // we're handling an ICE restart.  We need to hold the old stream until
   // we know the new stream is working.
   if (stream_ && !old_stream_ && (stream_ != stream)) {
     // Here we leave the old stream's signals connected until we don't need
     // it anymore.  They will be disconnected if ice restart is successful.
     old_stream_ = stream_;
     MOZ_MTLOG(ML_INFO, LAYER_INFO << "SetParameters save old stream("
--- a/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
+++ b/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
@@ -722,16 +722,17 @@ JsepSessionImpl::GetRemoteIds(const Sdp&
   return rv;
 }
 
 nsresult
 JsepSessionImpl::CreateOffer(const JsepOfferOptions& options,
                              std::string* offer)
 {
   mLastError.clear();
+  mLocalIceIsRestarting = options.mIceRestart.isSome() && *(options.mIceRestart);
 
   if (mState != kJsepStateStable) {
     JSEP_SET_ERROR("Cannot create offer in state " << GetStateStr(mState));
     return NS_ERROR_UNEXPECTED;
   }
 
   // Undo track assignments from a previous call to CreateOffer
   // (ie; if the track has not been negotiated yet, it doesn't necessarily need
@@ -1989,16 +1990,25 @@ JsepSessionImpl::ValidateRemoteDescripti
 
     if (oldMsection.GetMediaType() != newMsection.GetMediaType()) {
       JSEP_SET_ERROR("Remote description changes the media type of m-line "
                      << i);
       return NS_ERROR_INVALID_ARG;
     }
 
     bool differ = mSdpHelper.IceCredentialsDiffer(newMsection, oldMsection);
+
+    // Detect bad answer ICE restart when offer doesn't request ICE restart
+    if (mIsOfferer && differ && !mLocalIceIsRestarting) {
+      JSEP_SET_ERROR("Remote description indicates ICE restart but offer did not "
+                     "request ICE restart (new remote description changes either "
+                     "the ice-ufrag or ice-pwd)");
+      return NS_ERROR_INVALID_ARG;
+    }
+
     // Detect whether all the creds are the same or all are different
     if (!iceCredsDiffer.isSome()) {
       // for the first msection capture whether creds are different or same
       iceCredsDiffer = mozilla::Some(differ);
     } else if (iceCredsDiffer.isSome() && *iceCredsDiffer != differ) {
       // subsequent msections must match the first sections
       JSEP_SET_ERROR("Partial ICE restart is unsupported at this time "
                      "(new remote description changes either the ice-ufrag "
--- a/media/webrtc/signaling/src/jsep/JsepSessionImpl.h
+++ b/media/webrtc/signaling/src/jsep/JsepSessionImpl.h
@@ -29,16 +29,17 @@ public:
 class JsepSessionImpl : public JsepSession
 {
 public:
   JsepSessionImpl(const std::string& name, UniquePtr<JsepUuidGenerator> uuidgen)
       : JsepSession(name),
         mIsOfferer(false),
         mWasOffererLastTime(false),
         mIceControlling(false),
+        mLocalIceIsRestarting(false),
         mRemoteIsIceLite(false),
         mRemoteIceIsRestarting(false),
         mBundlePolicy(kBundleBalanced),
         mSessionId(0),
         mSessionVersion(0),
         mUuidGen(Move(uuidgen)),
         mSdpHelper(&mLastError)
   {
@@ -317,16 +318,17 @@ private:
   std::vector<RefPtr<JsepTransport> > mOldTransports;
   std::vector<JsepTrackPair> mNegotiatedTrackPairs;
 
   bool mIsOfferer;
   bool mWasOffererLastTime;
   bool mIceControlling;
   std::string mIceUfrag;
   std::string mIcePwd;
+  bool mLocalIceIsRestarting;
   bool mRemoteIsIceLite;
   bool mRemoteIceIsRestarting;
   std::vector<std::string> mIceOptions;
   JsepBundlePolicy mBundlePolicy;
   std::vector<JsepDtlsFingerprint> mDtlsFingerprints;
   uint64_t mSessionId;
   uint64_t mSessionVersion;
   std::vector<SdpExtmapAttributeList::Extmap> mAudioRtpExtensions;