From 41d80670d96b431f3358c4ac8906847d3e62ee40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois-Simon=20Fauteux-Chapleau?= Date: Thu, 10 Oct 2024 17:46:37 -0400 Subject: [PATCH] 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 | 11 +++++++++++ include/upnp/upnp_context.h | 7 ++++++- src/upnp/protocol/mapping.cpp | 19 +++++++++++++++++++ src/upnp/upnp_context.cpp | 26 ++++++++++++++++---------- 4 files changed, 52 insertions(+), 11 deletions(-) diff --git a/include/upnp/mapping.h b/include/upnp/mapping.h index 0468352e..9d009c2e 100644 --- a/include/upnp/mapping.h +++ b/include/upnp/mapping.h @@ -24,6 +24,7 @@ #include #include #include +#include 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_; // 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 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_; diff --git a/include/upnp/upnp_context.h b/include/upnp/upnp_context.h index da22e25c..46254dc1 100644 --- a/include/upnp/upnp_context.h +++ b/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); diff --git a/src/upnp/protocol/mapping.cpp b/src/upnp/protocol/mapping.cpp index 7e3c0366..6e47ae6d 100644 --- a/src/upnp/protocol/mapping.cpp +++ b/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 { diff --git a/src/upnp/upnp_context.cpp b/src/upnp/upnp_context.cpp index 5a3d8a28..8bb2fece 100644 --- a/src/upnp/upnp_context.cpp +++ b/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, 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);