Fix various spec compliance gaps#504
Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets MQTT v3.1.1 compliance for fixed-header reserved flags (notably SUBSCRIBE) by validating the first-byte flag nibble during decode/dispatch and ensuring malformed packets lead to connection close in the broker.
Changes:
- Add a public helper to validate fixed-header reserved flags (
MqttPacket_FixedHeaderFlagsValid) and use it fromMqttDecode_FixedHeader. - Update broker dispatch to (a) pre-validate fixed-header flags for packet types not run through decoders and (b) close connections when handlers report malformed/protocol errors.
- Add unit tests covering canonical/invalid fixed-header flag permutations and end-to-end decode rejection for SUBSCRIBE/UNSUBSCRIBE/PUBREL and malformed PUBLISH flags.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
wolfmqtt/mqtt_packet.h |
Exposes new fixed-header flag validation helper as public API. |
src/mqtt_packet.c |
Implements reserved-flag validation and enforces it in MqttDecode_FixedHeader. |
src/mqtt_broker.c |
Closes connections on malformed packets by validating flags pre-dispatch and honoring handler error returns. |
tests/test_mqtt_packet.c |
Adds coverage for valid/invalid reserved-flag nibbles and malformed PUBLISH QoS/DUP combinations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
embhorn
changed the title
embhorn
changed the title
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #504
Scan targets checked: wolfmqtt-bugs, wolfmqtt-src
No new issues found in the changed files. ✅
…he Generic Encoder
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #504
Scan targets checked: wolfmqtt-bugs, wolfmqtt-src
No new issues found in the changed files. ✅
| * unlike packet IDs, 0 has no protocol significance here. */ | ||
| broker->next_auto_id = 1; | ||
| } | ||
| auto_len = XSNPRINTF(auto_id, (int)sizeof(auto_id), |
There was a problem hiding this comment.
We should rewrite this section to avoid the XSNPRINTF all together. Its the only use...
| * values, but this helper is the final boundary — a future caller | ||
| * passing a reserved value should fail loudly here rather than emit | ||
| * a malformed SUBACK on the wire. */ | ||
| { |
There was a problem hiding this comment.
Please fix the uses of empty braces.
| /* Returns 1 if packet_id is currently awaiting PUBREL, 0 otherwise. */ | ||
| static int BrokerInboundQos2_Contains(BrokerClient* bc, word16 packet_id) | ||
| { | ||
| if (bc == NULL || packet_id == 0) { |
There was a problem hiding this comment.
These new functions need to avoid empty braces
| * gate (especially for QoS 0, where the function- | ||
| * level rc is otherwise never overwritten before | ||
| * return). */ | ||
| int sub_rc = MqttEncode_Publish(sub->client->tx_buf, |
| candidate = client->next_packet_id; | ||
|
|
||
| #ifdef WOLFMQTT_MULTITHREAD | ||
| { |
| } | ||
| } | ||
|
|
||
| /* [MQTT-3.9.3-2] Validate a SUBACK return code against the spec-allowed |
There was a problem hiding this comment.
No unicode. Replace § with "Section" or remove.
### LOW-9: ASCII-only rule violated: em dashes (U+2014) and section signs (U+00A7) in new comments
- **File:** `src/mqtt_broker.c:multiple, src/mqtt_packet.c:multiple, src/mqtt_client.c:3098, wolfmqtt/mqtt_broker.h:253, wolfmqtt/mqtt_packet.h:678,691,745`
- **Function:** `(many)`
- **Action:** SUGGEST
- **Tag:** convention
- **Confidence:** High
**Description:** The diff introduces dozens of non-ASCII characters in new comments: em dashes (U+2014, encoded 0xE2 0x80 0x94) and section signs (U+00A7, encoded 0xC2 0xA7) in references like `v5 §3.2.2.3.20`. The wolfSSL coding rules require 7-bit ASCII for all committed code, comments, and commit messages. Examples include: src/mqtt_broker.c:252,980,995,2854,2957,3097-3099,3328,3500-3508,3675,3713,3719,3872,3952,4144-4156; src/mqtt_packet.c:214-215,257,293-294,357,520,619,1342-1343,1362,1371,1875-1876,2084-2086,2378-2381,2408,2737,2961,3081; src/mqtt_client.c:3098; wolfmqtt/mqtt_broker.h:253; wolfmqtt/mqtt_packet.h:678,691,745.
**Code:**
/* MQTT 3.1.1 §3.12 / v5 §3.12: PINGREQ is fixed-header-
- only — Remaining Length MUST be 0. */
**Recommendation:** Replace all em dashes and section signs in the diff with ASCII equivalents.
| * [MQTT-3.9.3-2] reserved-code rejection branch. The prior prototype | ||
| * silences -Wmissing-prototypes; the symbol is intentionally not in | ||
| * any public header. */ | ||
| int BrokerSend_SubAck(BrokerClient* bc, word16 packet_id, |
There was a problem hiding this comment.
Is this forward declaration needed? Should it be static or put as WOLFMQTT_LOCAL in header.
| } | ||
| WOLFMQTT_FREE(cur); | ||
| if (bc->qos2_pending_count > 0) { | ||
| bc->qos2_pending_count--; |
There was a problem hiding this comment.
Consider debug log for case where this is "else". Shouldn't happen.
| * count of accepted clients. */ | ||
| char auto_id[32]; | ||
| int auto_len; | ||
| unsigned long id_value = (unsigned long)broker->next_auto_id++; |
There was a problem hiding this comment.
For "auto" consider these options...
(a) only increment after CONNECT acceptance
(b) seed next_auto_id from a CSPRNG at init time, would harden against value enumeration.
| { | ||
| word16 i; | ||
| if (topic_name == NULL && len > 0) { | ||
| return 0; |
There was a problem hiding this comment.
The function returns 1 for (NULL, 0, level >= 5), which is inconsistent with MqttEncode_Publish's stricter contract that topic_name == NULL is BAD_ARG. The encoder check fires before this helper, so production is safe, but the helper itself is permissive enough that a future caller could pass NULL believing it is a legal Topic Alias placeholder.
Thanks to @LiD0209 and @itmanlee for the excellent issue reports
MqttClientNet_DeInit. #536