Get the semantics of incoming and outgoing aliases correct

Wiebe Cazemier
1 parent 39c1873d
Showing 5 changed files with 39 additions and 17 deletions
client.cpp
client.h
configfileparser.cpp
mqttpacket.cpp
settings.h
@@ -208,11 +208,10 @@ int Client::writeMqttPacket(const MqttPacket &amp;packet)
 int Client::writeMqttPacketAndBlameThisClient(PublishCopyFactory &copyFactory, char max_qos, uint16_t packet_id)
 {
-    const Settings *settings = ThreadGlobals::getSettings();
     uint16_t topic_alias = 0;
     bool skip_topic = false;
-    if (protocolVersion >= ProtocolVersion::Mqtt5 && settings->maxOutgoingTopicAliases > this->curOutgoingTopicAlias)
+    if (protocolVersion >= ProtocolVersion::Mqtt5 && this->maxOutgoingTopicAliasValue > this->curOutgoingTopicAlias)
     {
         uint16_t &id = this->outgoingTopicAliases[copyFactory.getTopic()];
@@ -358,15 +357,18 @@ void Client::setTopicAlias(const uint16_t alias_id, const std::string &amp;topic)
     if (topic.empty())
         return;
-    if (alias_id > this->maxTopicAliases)
-        throw ProtocolError("Client exceeded max topic aliases.");
+    const Settings *settings = ThreadGlobals::getSettings();
+
+    // The specs actually say "The Client MUST NOT send a Topic Alias [...] to the Server greater than this value [Topic Alias Maximum]". So, it's not about count.
+    if (alias_id > settings->maxIncomingTopicAliasValue)
+        throw ProtocolError(formatString("Client tried to set more topic aliases than the server max of %d per client", settings->maxIncomingTopicAliasValue));
-    this->topicAliases[alias_id] = topic;
+    this->incomingTopicAliases[alias_id] = topic;
 }
 const std::string &Client::getTopicAlias(const uint16_t id)
 {
-    return this->topicAliases[id];
+    return this->incomingTopicAliases[id];
 }
 #ifndef NDEBUG
@@ -460,7 +462,7 @@ void Client::setClientProperties(ProtocolVersion protocolVersion, const std::str
 void Client::setClientProperties(ProtocolVersion protocolVersion, const std::string &clientId, const std::string username, bool connectPacketSeen, uint16_t keepalive,
-                                 uint32_t maxPacketSize, uint16_t maxTopicAliases)
+                                 uint32_t maxPacketSize, uint16_t maxOutgoingTopicAliasValue)
 {
     this->protocolVersion = protocolVersion;
     this->clientid = clientId;
@@ -468,7 +470,7 @@ void Client::setClientProperties(ProtocolVersion protocolVersion, const std::str
     this->connectPacketSeen = connectPacketSeen;
     this->keepalive = keepalive;
     this->maxPacketSize = maxPacketSize;
-    this->maxTopicAliases = maxTopicAliases;
+    this->maxOutgoingTopicAliasValue = maxOutgoingTopicAliasValue;
 }
 void Client::setWill(Publish &&willPublish)
@@ -53,7 +53,7 @@ class Client
     const size_t initialBufferSize = 0;
     uint32_t maxPacketSize = 0;
-    uint16_t maxTopicAliases = 0;
+    uint16_t maxOutgoingTopicAliasValue = 0;
     IoWrapper ioWrapper;
     std::string transportStr;
@@ -82,7 +82,7 @@ class Client
     std::shared_ptr<Session> session;
-    std::unordered_map<uint16_t, std::string> topicAliases;
+    std::unordered_map<uint16_t, std::string> incomingTopicAliases;
     uint16_t curOutgoingTopicAlias = 0;
     std::unordered_map<std::string, uint16_t> outgoingTopicAliases;
@@ -111,7 +111,7 @@ public:
     void bufferToMqttPackets(std::vector<MqttPacket> &packetQueueIn, std::shared_ptr<Client> &sender);
     void setClientProperties(ProtocolVersion protocolVersion, const std::string &clientId, const std::string username, bool connectPacketSeen, uint16_t keepalive);
     void setClientProperties(ProtocolVersion protocolVersion, const std::string &clientId, const std::string username, bool connectPacketSeen, uint16_t keepalive,
-                             uint32_t maxPacketSize, uint16_t maxTopicAliases);
+                             uint32_t maxPacketSize, uint16_t maxOutgoingTopicAliasValue);
     void setWill(const std::string &topic, const std::string &payload, bool retain, char qos);
     void setWill(Publish &&willPublish);
     void clearWill();
@@ -461,6 +461,26 @@ void ConfigFileParser::loadFile(bool test)
                     }
                     tmpSettings->maxQosBytesPendingPerClient = newVal;
                 }
+
+                if (key == "max_incoming_topic_alias_value")
+                {
+                    int newVal = std::stoi(value);
+                    if (newVal < 0 || newVal > 0xFFFF)
+                    {
+                        throw ConfigFileException(formatString("max_incoming_topic_alias_value value '%d' is invalid. Valid values are between 0 and 65535.", newVal));
+                    }
+                    tmpSettings->maxIncomingTopicAliasValue = newVal;
+                }
+
+                if (key == "max_outgoing_topic_alias_value")
+                {
+                    int newVal = std::stoi(value);
+                    if (newVal < 0 || newVal > 0xFFFF)
+                    {
+                        throw ConfigFileException(formatString("max_outgoing_topic_alias_value value '%d' is invalid. Valid values are between 0 and 65535.", newVal));
+                    }
+                    tmpSettings->maxOutgoingTopicAliasValue = newVal;
+                }
             }
         }
         catch (std::invalid_argument &ex) // catch for the stoi()
@@ -345,7 +345,7 @@ void MqttPacket::handleConnect()
         uint16_t max_qos_packets = settings.maxQosMsgPendingPerClient;
         uint32_t session_expire = settings.expireSessionsAfterSeconds > 0 ? settings.expireSessionsAfterSeconds : std::numeric_limits<uint32_t>::max();
         uint32_t max_packet_size = settings.maxPacketSize;
-        uint16_t max_topic_aliases = settings.maxOutgoingTopicAliases;
+        uint16_t max_outgoing_topic_aliases = 0; // Default MUST BE 0, meaning server won't initiate aliases
         bool request_response_information = false;
         bool request_problem_information = false;
@@ -370,7 +370,7 @@ void MqttPacket::handleConnect()
                     max_packet_size = std::min<uint32_t>(readFourBytesToUint32(), max_packet_size);
                     break;
                 case Mqtt5Properties::TopicAliasMaximum:
-                    max_topic_aliases = std::min<uint16_t>(readTwoBytesToUInt16(), max_topic_aliases);
+                    max_outgoing_topic_aliases = std::min<uint16_t>(readTwoBytesToUInt16(), settings.maxOutgoingTopicAliasValue);
                     break;
                 case Mqtt5Properties::RequestResponseInformation:
                     request_response_information = !!readByte();
@@ -541,7 +541,7 @@ void MqttPacket::handleConnect()
             clientIdGenerated = true;
         }
-        sender->setClientProperties(protocolVersion, client_id, username, true, keep_alive, max_packet_size, max_topic_aliases);
+        sender->setClientProperties(protocolVersion, client_id, username, true, keep_alive, max_packet_size, max_outgoing_topic_aliases);
         if (will_flag)
             sender->setWill(std::move(willpublish));
@@ -582,7 +582,7 @@ void MqttPacket::handleConnect()
                 connAck.propertyBuilder->writeMaxPacketSize(max_packet_size);
                 if (clientIdGenerated)
                     connAck.propertyBuilder->writeAssignedClientId(client_id);
-                connAck.propertyBuilder->writeMaxTopicAliases(max_topic_aliases);
+                connAck.propertyBuilder->writeMaxTopicAliases(settings.maxIncomingTopicAliasValue);
                 connAck.propertyBuilder->writeWildcardSubscriptionAvailable(1);
                 connAck.propertyBuilder->writeSubscriptionIdentifiersAvailable(0);
                 connAck.propertyBuilder->writeSharedSubscriptionAvailable(0);
@@ -42,8 +42,8 @@ public:
     bool authPluginSerializeAuthChecks = false;
     int clientInitialBufferSize = 1024; // Must be power of 2
     int maxPacketSize = 268435461; // 256 MB + 5
-    uint16_t maxIncomingTopicAliases = 65535;
-    uint16_t maxOutgoingTopicAliases = 0; // TODO: setting, when I can confirm with clients that support it that it works.
+    uint16_t maxIncomingTopicAliasValue = 65535;
+    uint16_t maxOutgoingTopicAliasValue = 65535;
 #ifdef TESTING
     bool logDebug = true;
 #else