upnp: only call NotifyCallback once for each state

This patch modifies the Mapping class to prevent a mapping's
NotifyCallback from being called more than once if the mapping's state
hasn't changed (which can cause bugs, cf. the GitLab issue linked
below).

The patch also fixes a race condition in the reserveMapping function
where a mapping reserved by one thread could wrongly appear to still be
available from the perspective of a different thread.

https://git.jami.net/savoirfairelinux/jami-client-ios/-/issues/410

Change-Id: I3784d36ccbb2f278ecc1c2573ac6f36b125f58c1

include/upnp/mapping.h

@@ -24,6 +24,7 @@
 #include <functional>
 #include <mutex>
 #include <memory>
+#include <optional>
 
 namespace dhtnet {
 namespace upnp {
@@ -124,6 +125,9 @@ class Mapping
     sys_clock::time_point getExpiryTime() const;
 
 private:
+    // Call the mapping's NotifyCallback (notifyCb_) if it has never been called before
+    // or if the state of the mapping has changed since the last time it was called.
+    static void notify(sharedPtr_t mapping);
     NotifyCallback getNotifyCallback() const;
     void setInternalAddress(const std::string& addr);
     void setExternalPort(uint16_t port);
@@ -144,9 +148,16 @@ class Mapping
     std::shared_ptr<IGD> igd_;
     // Track if the mapping is available to use.
     bool available_;
+
     // Track the state of the mapping
     MappingState state_;
+    // Callback used to notify the user when the state of the mapping changes.
     NotifyCallback notifyCb_;
+    // State of the mapping the last time its NotifyCallback was called.
+    // Used by the `notify` function to avoid calling the NotifyCallback
+    // twice for the same mapping state.
+    std::optional<MappingState> lastNotifiedState_ {std::nullopt};
+
     // If true, a new mapping will be requested on behalf of the mapping
     // owner when the mapping state changes from "OPEN" to "FAILED".
     bool autoUpdate_;

include/upnp/upnp_context.h

@@ -162,7 +162,12 @@ class UPnPContext : public UpnpMappingObserver
 
     // Add a new mapping to the local list and
     // send a request to the IGD to create it.
-    Mapping::sharedPtr_t registerMapping(Mapping& map);
+    //
+    // On success, this function returns a shared pointer to the Mapping object
+    // added to the local list. If no mapping was added to the list (which can
+    // happen if the requested mapping was already present or if no valid IGD is
+    // available), then the returned pointer will be null.
+    Mapping::sharedPtr_t registerMapping(Mapping& map, bool available = true);
 
     // Remove the given mapping from the local list.
     void unregisterMapping(const Mapping::sharedPtr_t& map);

src/upnp/protocol/mapping.cpp

@@ -48,6 +48,7 @@ Mapping::Mapping(const Mapping& other)
     available_ = other.available_;
     state_ = other.state_;
     notifyCb_ = other.notifyCb_;
+    lastNotifiedState_ = other.lastNotifiedState_;
     autoUpdate_ = other.autoUpdate_;
     renewalTime_ = other.renewalTime_;
     expiryTime_ = other.expiryTime_;
@@ -289,6 +290,24 @@ Mapping::getState() const
     return state_;
 }
 
+void
+Mapping::notify(sharedPtr_t mapping)
+{
+    if (!mapping)
+        return;
+
+    NotifyCallback cb;
+    {
+        std::lock_guard lock(mapping->mutex_);
+        if (mapping->state_ != mapping->lastNotifiedState_) {
+            mapping->lastNotifiedState_ = mapping->state_;
+            cb = mapping->notifyCb_;
+        }
+    }
+    if (cb)
+        cb(mapping);
+}
+
 Mapping::NotifyCallback
 Mapping::getNotifyCallback() const
 {

src/upnp/upnp_context.cpp

@@ -353,6 +353,9 @@ UPnPContext::reserveMapping(Mapping& requestedMap)
                 if (map->getState() == MappingState::OPEN) {
                     // Found an "OPEN" mapping. We are done.
                     mapRes = map;
+                    // Make the mapping unavailable while we're holding the lock on
+                    // mappingMutex_ to ensure no other thread will try to use it.
+                    mapRes->setAvailable(false);
                     break;
                 }
             }
@@ -361,18 +364,15 @@ UPnPContext::reserveMapping(Mapping& requestedMap)
 
     // Create a mapping if none was available.
     if (not mapRes) {
-        mapRes = registerMapping(requestedMap);
+        mapRes = registerMapping(requestedMap, /* available */ false);
     }
 
     if (mapRes) {
-        // Make the mapping unavailable
-        mapRes->setAvailable(false);
         // Copy attributes.
         mapRes->setNotifyCallback(requestedMap.getNotifyCallback());
         mapRes->enableAutoUpdate(requestedMap.getAutoUpdate());
         // Notify the listener.
-        if (auto cb = mapRes->getNotifyCallback())
-            cb(mapRes);
+        Mapping::notify(mapRes);
     }
 
     enforceAvailableMappingsLimits();
@@ -1101,8 +1101,7 @@ UPnPContext::onMappingRemoved(const std::shared_ptr<IGD>& igd, const Mapping& ma
 
     auto map = getMappingWithKey(mapRes.getMapKey());
     // Notify the listener.
-    if (map and map->getNotifyCallback())
-        map->getNotifyCallback()(map);
+    Mapping::notify(map);
 }
 
 void
@@ -1159,7 +1158,7 @@ UPnPContext::setIgdDiscoveryTimeout(std::chrono::milliseconds timeout)
 }
 
 Mapping::sharedPtr_t
-UPnPContext::registerMapping(Mapping& map)
+UPnPContext::registerMapping(Mapping& map, bool available)
 {
     Mapping::sharedPtr_t mapPtr;
 
@@ -1186,6 +1185,10 @@ UPnPContext::registerMapping(Mapping& map)
             return {};
         }
         mapPtr = ret.first->second;
+        // It's important to set the mapping's availability while mappingMutex_ is locked,
+        // otherwise a call to reserveMapping from a different thread could try to use the
+        // mapping we just added to the mapping list even if `available` is false.
+        mapPtr->setAvailable(available);
         assert(mapPtr);
     }
 
@@ -1201,6 +1204,9 @@ UPnPContext::registerMapping(Mapping& map)
             if (logger_) logger_->warn("Request for mapping {} failed, no IGD available",
                                        map.toString());
             updateMappingState(mapPtr, MappingState::FAILED);
+            // The call to `updateMappingState` above will cause the mapping to be
+            // removed from the mapping list, so we return a null pointer.
+            return {};
         }
     } else {
         // There is a valid IGD available, request the mapping.
@@ -1283,8 +1289,8 @@ UPnPContext::updateMappingState(const Mapping::sharedPtr_t& map, MappingState ne
     map->setState(newState);
 
     // Notify the listener if set.
-    if (notify and map->getNotifyCallback())
-        map->getNotifyCallback()(map);
+    if (notify)
+        Mapping::notify(map);
 
     if (newState == MappingState::FAILED)
         handleFailedMapping(map);