Fix MQTTv5 QoS 2 CONNACK interop and add WOLFMQTT_MAX_QOS build cap#537
Open
dgarske wants to merge 6 commits into
Open
Fix MQTTv5 QoS 2 CONNACK interop and add WOLFMQTT_MAX_QOS build cap#537dgarske wants to merge 6 commits into
dgarske wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Removes an invalid MQTT v5 CONNACK property that violated the spec and caused strict clients to disconnect.
Changes:
- Drop emission of
MQTT_PROP_MAX_QOS = 2in CONNACK, which is a protocol error per [MQTT-3.2.2.3.4]. - Add an explanatory comment documenting why the property is intentionally omitted.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
|
Should this be a compile-time configuration option for the broker? Then if QoS-2 is max, don't send the property. |
Builds with -DWOLFMQTT_MAX_QOS=1 (or 0) now compile out the broker's inbound QoS-2 dedup state, BrokerHandle_PublishRel / PublishRec, and the PUBLISH_REC/REL/COMP dispatch cases; subscribe grants are capped at the build's max; PUBLISH at QoS > cap is rejected with a v5 DISCONNECT reason 0x9B (QoS Not Supported); will QoS is clamped on CONNECT. Client side clamps initial max_qos and post-CONNACK property value against the same cap so MqttPublishMsg rejects out-of-range publishes locally. Default build (WOLFMQTT_MAX_QOS=2) is unchanged: broker.test passes in full, no behavior change. Measured code-size delta on src/.libs/mqtt_broker (.text): default 48178 -> capped 45587 (-2591 bytes / -5.4%). Verified against mosquitto_pub -q 2: capped broker disconnects with "Message QoS not supported on broker, try a lower QoS."
… mqtt_types.h and reorder includes in mqtt_broker.c
Comment on lines
+3715
to
+3726
| if (pub.qos > WOLFMQTT_MAX_QOS) { | ||
| WBLOG_ERR(broker, | ||
| "broker: PUBLISH QoS %d exceeds WOLFMQTT_MAX_QOS=%d sock=%d", | ||
| pub.qos, WOLFMQTT_MAX_QOS, (int)bc->sock); | ||
| #ifdef WOLFMQTT_V5 | ||
| if (bc->protocol_level >= MQTT_CONNECT_PROTOCOL_LEVEL_5) { | ||
| (void)BrokerSend_Disconnect(bc, MQTT_REASON_QOS_NOT_SUPPORTED); | ||
| } | ||
| #endif | ||
| rc = MQTT_CODE_ERROR_MALFORMED_DATA; | ||
| goto publish_cleanup; | ||
| } |
Comment on lines
+3276
to
+3278
| * sent Will QoS > advertised Max QoS would already be in | ||
| * Protocol Error territory, but for v3.1.1 (no advertisement) | ||
| * we silently downgrade rather than rejecting CONNECT. */ |
Comment on lines
+200
to
+207
| * Define in user_settings.h to compile a QoS-capped client/broker. Wired | ||
| * into broker CONNACK Maximum QoS property emission today; broader code | ||
| * gating lives on the mqtt_qos_max branch. */ | ||
| #ifndef WOLFMQTT_MAX_QOS | ||
| #define WOLFMQTT_MAX_QOS 2 | ||
| #endif | ||
| #if (WOLFMQTT_MAX_QOS < 0) || (WOLFMQTT_MAX_QOS > 2) | ||
| #error "WOLFMQTT_MAX_QOS must be 0, 1, or 2" |
| /* Maximum QoS supported by this build. Legal values: 0, 1, 2. Default 2. | ||
| * Define in user_settings.h to compile a QoS-capped client/broker. Wired | ||
| * into broker CONNACK Maximum QoS property emission today; broader code | ||
| * gating lives on the mqtt_qos_max branch. */ |
Comment on lines
+58
to
+61
| - name: "Broker MAX_QOS=1 (build only)" | ||
| cflags: "" | ||
| wolfmqtt_opts: "--enable-v5 --enable-broker --enable-max-qos=1" | ||
| skip_broker_test: "yes" |
| T12_PROPS=yes | ||
| grep -q "Property CB: Type 37" "${TMP_DIR}/t12.log" 2>/dev/null || T12_PROPS=no | ||
| grep -q "Property CB: Type 36" "${TMP_DIR}/t12.log" 2>/dev/null || T12_PROPS=no | ||
| grep -q "Property CB: Type 40" "${TMP_DIR}/t12.log" 2>/dev/null || T12_PROPS=no |
Comment on lines
+17
to
+19
| # Smoke-build the CMake project under each WOLFMQTT_MAX_QOS value | ||
| # to keep the build-option plumbing exercised. "" means leave the | ||
| # cache value at its 2 default. |
Comment on lines
27
to
+28
| #include "wolfmqtt/mqtt_broker.h" | ||
| #include "wolfmqtt/mqtt_types.h" |
Comment on lines
+26
to
+31
| #if !defined(WOLFMQTT_USER_SETTINGS) && \ | ||
| !defined(_WIN32) && !defined(USE_WINDOWS_API) | ||
| /* If options.h is missing use the "./configure" script. Otherwise, copy | ||
| * the template "wolfmqtt/options.h.in" into "wolfmqtt/options.h" */ | ||
| #include <wolfmqtt/options.h> | ||
| #endif |
Comment on lines
+3461
to
+3472
| /* [MQTT-3.2.2.3.4] Maximum QoS property MUST be 0 or 1. Absence | ||
| * of the property signals server supports Maximum QoS 2. Emitting | ||
| * Maximum QoS = 2 is a Protocol Error and strict v5 clients (e.g. | ||
| * mosquitto) will disconnect on receipt. Emit the property only | ||
| * when this build caps below QoS 2 via WOLFMQTT_MAX_QOS. */ | ||
| #if WOLFMQTT_MAX_QOS < 2 | ||
| prop = MqttProps_Add(&ack.props); | ||
| if (prop != NULL) { | ||
| prop->type = MQTT_PROP_MAX_QOS; | ||
| prop->data_byte = MQTT_QOS_2; | ||
| prop->data_byte = (byte)WOLFMQTT_MAX_QOS; | ||
| } | ||
| #endif |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Maximum QoS = 2in CONNACK ([MQTT-3.2.2.3.4] Protocol Error; mosquitto disconnects).WOLFMQTT_MAX_QOS(0/1/2, default 2) wired throughconfigure.ac(--enable-max-qos=N) and CMake (-DWOLFMQTT_MAX_QOS=N).0x9B QoS Not Supported. Client clampsmax_qosagainst the build cap. ~2.6 KB.textsavings atMAX_QOS=1.broker-check.ymladds MAX_QOS=0/1/2 entries (capped = build-only sincebroker.testhas QoS-2 cases);cmake-build.ymlnow a matrix over the same values.