call: fix invalid call instance access

Link a call and a subcall using shared_ptr relation (parent_).
This prevents invalid access on destroyed call pointer,
as seen on Android platform.

Change-Id: I2c6423afb6bd9846a9fd84a206c8fbf4bce969ee
Reviewed-by: Aline Bonnet <aline.bonnet@savoirfairelinux.com>

src/call.cpp

@@ -212,7 +212,7 @@ Call::setState(CallState call_state, ConnectionState cnx_state, signed code)
         l(callState_, connectionState_, code);
 
     if (old_client_state != new_client_state) {
-        if (not parent_.load()) {
+        if (not parent_) {
             RING_DBG("[call:%s] emit client call state change %s, code %d",
                      id_.c_str(), new_client_state.c_str(), code);
             emitSignal<DRing::CallSignal::StateChange>(id_, new_client_state, code);
@@ -342,10 +342,14 @@ Call::getNullDetails()
 void
 Call::onTextMessage(std::map<std::string, std::string>&& messages)
 {
-    if (parent_.load())
-        pendingInMessages_.emplace_back(std::move(messages), "");
-    else
-        Manager::instance().incomingMessage(getCallId(), getPeerNumber(), messages);
+    {
+        std::lock_guard<std::recursive_mutex> lk {callMutex_};
+        if (parent_) {
+            pendingInMessages_.emplace_back(std::move(messages), "");
+            return;
+        }
+    }
+    Manager::instance().incomingMessage(getCallId(), getPeerNumber(), messages);
 }
 
 void
@@ -376,18 +380,15 @@ Call::addSubCall(Call& subcall)
     }
 
     RING_DBG("[call:%s] add subcall %s", getCallId().c_str(), subcall.getCallId().c_str());
-    subcall.parent_ = this;
+    subcall.parent_ = getPtr(*this);
 
     for (const auto& msg : pendingOutMessages_)
         subcall.sendTextMessage(msg.first, msg.second);
 
     subcall.addStateListener(
-        [&subcall](Call::CallState new_state,
-                   Call::ConnectionState new_cstate,
-                   UNUSED int code) {
-            auto ptr = getPtr(subcall); // keep the subcall valid (subcallStateChanged may remove it)
-            auto parent = subcall.parent_.load();
-            assert(parent != nullptr);
+        [&subcall](Call::CallState new_state, Call::ConnectionState new_cstate, UNUSED int code) {
+            auto parent = subcall.parent_;
+            assert(parent != nullptr); // subcall cannot be "un-parented"
             parent->subcallStateChanged(subcall, new_state, new_cstate);
         });
 }
@@ -494,7 +495,7 @@ Call::checkPendingIM()
 
     auto state = getStateStr();
     // Let parent call handles IM after the merge
-    if (not parent_.load()) {
+    if (not parent_) {
         if (state == DRing::Call::StateEvent::CURRENT) {
             for (const auto& msg : pendingInMessages_)
                 Manager::instance().incomingMessage(getCallId(), getPeerNumber(), msg.first);

src/call.h

@@ -40,7 +40,6 @@
 #include <condition_variable>
 #include <set>
 #include <list>
-#include <atomic>
 
 namespace ring {
 
@@ -262,7 +261,8 @@ class Call : public Recordable, public std::enable_shared_from_this<Call> {
         /// Return true if this call instance is a subcall (internal call for multi-device handling)
         ///
         bool isSubcall() const {
-            return parent_.load() != nullptr;
+            std::lock_guard<std::recursive_mutex> lk {callMutex_};
+            return parent_ != nullptr;
         }
 
     public: // media management
@@ -318,8 +318,8 @@ class Call : public Recordable, public std::enable_shared_from_this<Call> {
         bool isAudioMuted_{false};
         bool isVideoMuted_{false};
 
-        ///< MultiDevice: parent call, nullptr otherwise.
-        std::atomic<Call*> parent_ {nullptr};
+        ///< MultiDevice: parent call, nullptr otherwise. Access protected by callMutex_.
+        mutable std::shared_ptr<Call> parent_;
 
         ///< MultiDevice: list of attached subcall
         SubcallSet subcalls_;

src/sip/sipcall.cpp

@@ -712,7 +712,7 @@ SIPCall::onAnswered()
     RING_WARN("[call:%s] onAnswered()", getCallId().c_str());
     if (getConnectionState() != ConnectionState::CONNECTED) {
         setState(CallState::ACTIVE, ConnectionState::CONNECTED);
-        if (not parent_.load())
+        if (not isSubcall())
             Manager::instance().peerAnsweredCall(*this);
     }
 }
@@ -882,7 +882,7 @@ SIPCall::startAllMedia()
         }
     }
 
-    if (not parent_.load() and peerHolding_ != peer_holding) {
+    if (not isSubcall() and peerHolding_ != peer_holding) {
         peerHolding_ = peer_holding;
         emitSignal<DRing::CallSignal::PeerHold>(getCallId(), peerHolding_);
     }
@@ -928,7 +928,7 @@ SIPCall::muteMedia(const std::string& mediaType, bool mute)
         isVideoMuted_ = mute;
         videoInput_ = isVideoMuted_ ? "" : Manager::instance().getVideoManager().videoDeviceMonitor.getMRLForDefaultDevice();
         DRing::switchInput(getCallId(), videoInput_);
-        if (not parent_.load())
+        if (not isSubcall())
             emitSignal<DRing::CallSignal::VideoMuted>(getCallId(), isVideoMuted_);
 #endif
     } else if (mediaType.compare(DRing::Media::Details::MEDIA_TYPE_AUDIO) == 0) {
@@ -936,7 +936,7 @@ SIPCall::muteMedia(const std::string& mediaType, bool mute)
         RING_WARN("[call:%s] audio muting %s", getCallId().c_str(), bool_to_str(mute));
         isAudioMuted_ = mute;
         avformatrtp_->setMuted(isAudioMuted_);
-        if (not parent_.load())
+        if (not isSubcall())
             emitSignal<DRing::CallSignal::AudioMuted>(getCallId(), isAudioMuted_);
     }
 }
@@ -961,8 +961,9 @@ SIPCall::onMediaUpdate()
         return;
     }
 
-    // If we are a subcall let the parent manage it after merge operation
-    if (not parent_.load())
+    // Main call (no subcalls) must wait for ICE now, the rest of code needs to access
+    // to a negotiated transport.
+    if (not isSubcall())
         waitForIceAndStartMedia();
 }