diff --git a/include/upnp/mapping.h b/include/upnp/mapping.h
index 0468352e08776488041f99142b5b6b34bb72692e..9d009c2eaf6f8bdc377c42510a726b8aed31ad22 100644
--- a/include/upnp/mapping.h
+++ b/include/upnp/mapping.h
@@ -24,6 +24,7 @@
#include <functional>
#include <mutex>
#include <memory>
+#include <optional>
namespace dhtnet {
namespace upnp {
@@ -124,6 +125,9 @@ public:
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 @@ private:
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_;
diff --git a/include/upnp/upnp_context.h b/include/upnp/upnp_context.h
index da22e25c6c832941a4c82010efc64fdc4b35d1eb..46254dc1b7bdd9cbd7d12fe7b1c9e3f6338209f6 100644
--- a/include/upnp/upnp_context.h
+++ b/include/upnp/upnp_context.h
@@ -162,7 +162,12 @@ private:
// 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 7e3c03669b9ccb74238f8319706ca17f896fb865..6e47ae6d11eb07cd8075c0e93b85b4547a6a64fd 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 5a3d8a284bcc7e4df746229236c3fba6dd79a421..8bb2fece23c5ba9fd2b0f8976d255c8f33426a32 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>& 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);