From 98366c6dfb466bf89d2258d8b446278dd7d600b1 Mon Sep 17 00:00:00 2001 From: Wiebe Cazemier Date: Tue, 29 Mar 2022 20:23:54 +0200 Subject: [PATCH] Get the semantics of incoming and outgoing aliases correct --- client.cpp | 18 ++++++++++-------- client.h | 6 +++--- configfileparser.cpp | 20 ++++++++++++++++++++ mqttpacket.cpp | 8 ++++---- settings.h | 4 ++-- 5 files changed, 39 insertions(+), 17 deletions(-) diff --git a/client.cpp b/client.cpp index 89a6cb6..d49f496 100644 --- a/client.cpp +++ b/client.cpp @@ -208,11 +208,10 @@ int Client::writeMqttPacket(const MqttPacket &packet) int Client::writeMqttPacketAndBlameThisClient(PublishCopyFactory ©Factory, 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 &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) diff --git a/client.h b/client.h index d47654e..ece7ce7 100644 --- a/client.h +++ b/client.h @@ -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; - std::unordered_map topicAliases; + std::unordered_map incomingTopicAliases; uint16_t curOutgoingTopicAlias = 0; std::unordered_map outgoingTopicAliases; @@ -111,7 +111,7 @@ public: void bufferToMqttPackets(std::vector &packetQueueIn, std::shared_ptr &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(); diff --git a/configfileparser.cpp b/configfileparser.cpp index 86bb197..458f3cd 100644 --- a/configfileparser.cpp +++ b/configfileparser.cpp @@ -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() diff --git a/mqttpacket.cpp b/mqttpacket.cpp index 2ae99d5..722f77c 100644 --- a/mqttpacket.cpp +++ b/mqttpacket.cpp @@ -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::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(readFourBytesToUint32(), max_packet_size); break; case Mqtt5Properties::TopicAliasMaximum: - max_topic_aliases = std::min(readTwoBytesToUInt16(), max_topic_aliases); + max_outgoing_topic_aliases = std::min(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); diff --git a/settings.h b/settings.h index 1cc0f84..e32abce 100644 --- a/settings.h +++ b/settings.h @@ -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 -- libgit2 0.21.4