upnp: cleanup, avoid use-after-free

Change-Id: Ifac53c178d59434027d162be0400070c01144e81

src/upnp/protocol/natpmp/nat_pmp.cpp

@@ -73,14 +73,12 @@ NatPmp::NatPmp()
         closenatpmp(&natpmp);
     })
 {
-
 }
 
 NatPmp::~NatPmp()
 {
     {
         std::lock_guard<std::mutex> lock(validIgdMutex);
-
         if (pmpIGD_) {
             {
                 std::lock_guard<std::mutex> lk(pmpMutex_);
@@ -99,37 +97,30 @@ NatPmp::~NatPmp()
 }
 
 void
-NatPmp::connectivityChanged()
+NatPmp::clearIGDs()
 {
-    {
-        // Lock valid IGD.
-        std::lock_guard<std::mutex> lock(validIgdMutex);
-
-        // Clear internal IGD (nat pmp only supports one).
-        if (pmpIGD_) {
-            std::lock_guard<std::mutex> lk(pmpMutex_);
-            pmpIGD_->renewal_ = clock::now();
-            pmpIGD_.reset();
-        }
-
-        validIgdMutex.unlock();
+    // Lock valid IGD.
+    std::lock_guard<std::mutex> lock(validIgdMutex);
 
-        // Notify.
-        pmpCv_.notify_all();
+    // Clear internal IGD (nat pmp only supports one).
+    if (pmpIGD_) {
+        std::lock_guard<std::mutex> lk(pmpMutex_);
+        pmpIGD_->renewal_ = clock::now();
+        pmpIGD_.reset();
     }
 
+    pmpCv_.notify_all();
 }
 
 void
 NatPmp::searchForIGD()
 {
-    pmpRun_ = true;
-
-    // Clear internal IGD (nat pmp only supports one).
+    // Lock valid IGD.
+    std::lock_guard<std::mutex> lock(validIgdMutex);
     if (pmpIGD_) {
+        // Clear internal IGD (nat pmp only supports one).
         std::lock_guard<std::mutex> lk(pmpMutex_);
         pmpIGD_->renewal_ = clock::now();
-        pmpIGD_.reset();
     }
     pmpCv_.notify_all();
 }
@@ -250,14 +241,11 @@ NatPmp::searchForIGD(const std::shared_ptr<PMPIGD>& pmp_igd, natpmp_t& natpmp)
             if (not pmpIGD_) {
                 JAMI_DBG("NAT-PMP: Found device with external IP %s", pmp_igd->publicIp_.toString().c_str());
                 {
-                    // Store public Ip address.
-                    std::string publicIpStr(std::move(pmp_igd.get()->publicIp_.toString()));
-
                     // Add the igd to the upnp context class list.
-                    if (updateIgdListCb_(this, std::move(pmp_igd.get()), std::move(pmp_igd.get()->publicIp_), true)) {
-                        JAMI_DBG("NAT-PMP: IGD with public IP %s was added to the list", publicIpStr.c_str());
+                    if (updateIgdListCb_(this, pmp_igd.get(), pmp_igd->publicIp_, true)) {
+                        JAMI_DBG("NAT-PMP: IGD with public IP %s was added to the list", pmp_igd->publicIp_.toString().c_str());
                     } else {
-                        JAMI_DBG("NAT-PMP: IGD with public IP %s is already in the list", publicIpStr.c_str());
+                        JAMI_DBG("NAT-PMP: IGD with public IP %s is already in the list", pmp_igd->publicIp_.toString().c_str());
                     }
 
                     // Keep IGD internally.

src/upnp/protocol/natpmp/nat_pmp.h

@@ -55,7 +55,7 @@ public:
     Type getType() const override { return Type::NAT_PMP; }
 
     // Notifies a change in network.
-    void connectivityChanged() override;
+    void clearIGDs() override;
 
     // Renew pmp_igd.
     void searchForIGD() override;

src/upnp/protocol/natpmp/pmp_igd.h

@@ -30,7 +30,6 @@
 
 #include "noncopyable.h"
 #include "ip_utils.h"
-#include "string_utils.h"
 
 #include <map>
 #include <atomic>

src/upnp/protocol/pupnp/pupnp.cpp

@@ -142,7 +142,7 @@ PUPnP::~PUPnP()
 }
 
 void
-PUPnP::connectivityChanged()
+PUPnP::clearIGDs()
 {
     // Lock internal IGD list.
     std::lock_guard<std::mutex> lk(validIgdMutex);
@@ -197,8 +197,7 @@ PUPnP::addMapping(IGD* igd, uint16_t port_external, uint16_t port_internal, Port
         }
     }
 
-    auto pupnp_igd = dynamic_cast<const UPnPIGD*>(igd);
-    if (pupnp_igd) { 
+    if (auto pupnp_igd = dynamic_cast<const UPnPIGD*>(igd)) { 
         if (actionAddPortMapping(*pupnp_igd, mapping, upnp_error)) {
             JAMI_WARN("PUPnP: Opened port %s", mapping.toString().c_str());
             globalMappings->emplace(port_external, GlobalMapping{mapping});
@@ -329,11 +328,12 @@ PUPnP::handleCtrlPtUPnPEvents(Upnp_EventType event_type, const void* event)
         }
 
         // Check if this device ID is already in the list.
+        std::string cpDeviceId = UpnpDiscovery_get_DeviceID_cstr(d_event);
         std::lock_guard<std::mutex> lk(validIgdMutex);
-        if (cpDeviceList_.count(std::string(UpnpDiscovery_get_DeviceID_cstr(d_event))) > 0) {
+        if (cpDeviceList_.count(cpDeviceId) > 0) {
             break;
         }
-        cpDeviceList_.emplace(std::pair<std::string, std::string>(std::string(UpnpDiscovery_get_DeviceID_cstr(d_event)), ""));
+        cpDeviceList_.emplace(std::move(cpDeviceId), std::string{});
 
         /*
          * NOTE: This thing will block until success for the system socket timeout

src/upnp/protocol/pupnp/pupnp.h

@@ -75,7 +75,7 @@ public:
     Type getType() const override { return Type::PUPNP; }
 
     // Notifies a change in network.
-    void connectivityChanged() override;
+    void clearIGDs() override;
 
     // Sends out async search for IGD.
     void searchForIGD() override;

src/upnp/protocol/upnp_protocol.h

@@ -72,8 +72,8 @@ public:
     // Allows each protocol to return it's type.
     virtual Type getType() const = 0;
 
-    // Signals a change in the network.
-    virtual void connectivityChanged() = 0;
+    // Clear all known IGDs.
+    virtual void clearIGDs() = 0;
 
     // Search for IGD.
     virtual void searchForIGD() = 0;

src/upnp/upnp_context.cpp

@@ -67,26 +67,27 @@ UPnPContext::~UPnPContext()
 void
 UPnPContext::connectivityChanged()
 {
-    if (not igdList_.empty()) {
-
-        // Clear main IGD list.
+    {
         std::lock_guard<std::mutex> lock(igdListMutex_);
-        igdList_.clear();
-    
-        for (const auto& item : igdListeners_) {
-            item.second();
+        for (auto const& protocol : protocolList_)
+            protocol->clearIGDs();
+        if (not igdList_.empty()) {
+            // Clear main IGD list.
+            igdList_.clear();
+            for (const auto& listener : igdListeners_) {
+                listener.second();
+            }
         }
     }
 
-    for (auto const& item : protocolList_) {
-        item->connectivityChanged();
-        item->searchForIGD();
-    }
+    for (auto const& protocol : protocolList_)
+        protocol->searchForIGD();
 }
 
 bool
 UPnPContext::hasValidIGD()
 {
+    std::lock_guard<std::mutex> lock(igdListMutex_);
     return not igdList_.empty();
 }
 
@@ -153,7 +154,6 @@ UPnPContext::addMapping(uint16_t port_desired, uint16_t port_local, PortType typ
 
     // If we want a unique port, we must make sure the client isn't already using the port.
     if (unique) {
-
         bool unique_found = false;
 
         // Keep generating random ports until we find a unique one.
@@ -256,24 +256,8 @@ UPnPContext::getExternalIP() const
 }
 
 bool
-UPnPContext::isIgdInList(IGD* igd)
-{
-    std::lock_guard<std::mutex> igdListLock(igdListMutex_);
-
-    for (auto const& item : igdList_) {
-        if (item.second->publicIp_ == igd->publicIp_) {
-            return true;
-        }
-    }
-
-    return false;
-}
-
-bool
-UPnPContext::isIgdInList(IpAddr publicIpAddr)
+UPnPContext::isIgdInList(const IpAddr& publicIpAddr)
 {
-    std::lock_guard<std::mutex> igdListLock(igdListMutex_);
-
     for (auto const& item : igdList_) {
         if (item.second->publicIp_) {
             if (item.second->publicIp_ == publicIpAddr) {
@@ -281,7 +265,6 @@ UPnPContext::isIgdInList(IpAddr publicIpAddr)
             }
         }
     }
-
     return false;
 }
 
@@ -302,6 +285,7 @@ UPnPContext::getIgdProtocol(IGD* igd)
 bool
 UPnPContext::igdListChanged(UPnPProtocol* protocol, IGD* igd, IpAddr publicIpAddr, bool added)
 {
+    std::lock_guard<std::mutex> lock(igdListMutex_);
     if (added) {
         return addIgdToList(protocol, igd);
     } else {
@@ -317,27 +301,25 @@ bool
 UPnPContext::addIgdToList(UPnPProtocol* protocol, IGD* igd)
 {
     // Check if IGD has a valid public IP.
-    if (not igd->publicIp_.isValid(igd->publicIp_.toString().c_str())) {
+    if (not igd->publicIp_) {
         JAMI_WARN("UPnPContext: IGD trying to be added has invalid public IpAddress");
         return false;
     }
 
-    if (isIgdInList(igd)) {
+    if (isIgdInList(igd->publicIp_)) {
         // If the protocol of the IGD that is already in the list isn't NatPmp, then swap.
-        if (getIgdProtocol(igd) != UPnPProtocol::Type::NAT_PMP and
-            protocol->getType() == UPnPProtocol::Type::NAT_PMP) {
-                JAMI_WARN("UPnPContext: Attempting to swap IGD UPnP protocol");
-                if (!removeIgdFromList(igd)) {
-                    JAMI_WARN("UPnPContext: Failed to swap IGD UPnP protocol");
-                    return false;
-                }
+        if (getIgdProtocol(igd) != UPnPProtocol::Type::NAT_PMP and protocol->getType() == UPnPProtocol::Type::NAT_PMP) {
+            JAMI_WARN("UPnPContext: Attempting to swap IGD UPnP protocol");
+            if (!removeIgdFromList(igd)) {
+                JAMI_WARN("UPnPContext: Failed to swap IGD UPnP protocol");
+                return false;
+            }
         } else {
             return false;
         }
     }
 
-    IpAddr publicIp = igd->publicIp_;
-    igdList_.emplace_back(std::make_pair(protocol, igd));
+    igdList_.emplace_back(protocol, igd);
     
     for (const auto& item : igdListeners_) {
         item.second();
@@ -349,9 +331,7 @@ UPnPContext::addIgdToList(UPnPProtocol* protocol, IGD* igd)
 bool
 UPnPContext::removeIgdFromList(IGD* igd)
 {
-    std::lock_guard<std::mutex> igdListLock(igdListMutex_);
-
-    std::list<std::pair<UPnPProtocol*, IGD*>>::iterator it = igdList_.begin();
+    auto it = igdList_.begin();
     while (it != igdList_.end()) {
         if (it->second->publicIp_ == igd->publicIp_) {
             JAMI_WARN("UPnPContext: IGD with public IP %s was removed from the list", it->second->publicIp_.toString().c_str());
@@ -368,9 +348,7 @@ UPnPContext::removeIgdFromList(IGD* igd)
 bool
 UPnPContext::removeIgdFromList(IpAddr publicIpAddr)
 {
-    std::lock_guard<std::mutex> igdListLock(igdListMutex_);
-
-    std::list<std::pair<UPnPProtocol*, IGD*>>::iterator it = igdList_.begin();
+    auto it = igdList_.begin();
     while (it != igdList_.end()) {
         if (it->second->publicIp_ == publicIpAddr) {
             JAMI_WARN("UPnPContext: IGD with public IP %s was removed from the list", it->second->publicIp_.toString().c_str());
@@ -382,7 +360,6 @@ UPnPContext::removeIgdFromList(IpAddr publicIpAddr)
     }
 
     return false;
-
 }
 
 }} // namespace jami::upnp

src/upnp/upnp_context.h

@@ -95,6 +95,16 @@ public:
     // Inform the UPnP context that the network status has changed. This clears the list of known
     void connectivityChanged();
 
+private:
+    // Checks if the IGD is in the list by checking the IGD's public Ip.
+    bool isIgdInList(const IpAddr& publicIpAddr);
+
+    // Returns the protocol of the IGD.
+    UPnPProtocol::Type getIgdProtocol(IGD* igd);
+
+    // Returns a random port that is not yet used by the daemon for UPnP.
+    uint16_t chooseRandomPort(const IGD& igd, PortType type);
+
     // Tries to add or remove IGD to the list via callback.
     bool igdListChanged(UPnPProtocol* protocol, IGD* igd, const IpAddr publicIpAddr, bool added);
 
@@ -110,19 +120,6 @@ public:
     // Tries to add mapping. Assumes mutex is already locked.
     Mapping addMapping(IGD* igd, uint16_t port_external, uint16_t port_internal, PortType type, UPnPProtocol::UpnpError& upnp_error);
 
-private:
-    // Checks if the IGD is in the list by checking the IGD itself.
-    bool isIgdInList(IGD* igd);
-
-    // Checks if the IGD is in the list by checking the IGD's public Ip.
-    bool isIgdInList(IpAddr publicIpAddr);
-
-    // Returns the protocol of the IGD.
-    UPnPProtocol::Type getIgdProtocol(IGD* igd);
-
-    // Returns a random port that is not yet used by the daemon for UPnP.
-    uint16_t chooseRandomPort(const IGD& igd, PortType type);
-
 public:
     constexpr static unsigned MAX_RETRIES = 20;