Fenrir fixes: MQTT v5 client and broker

[embhorn]

• f-8609: suppress duplicate delivery of a retransmitted QoS 2 PUBLISH awaiting PUBREL
• f-8611: reject an empty ShareName in v5 shared-subscription filters
• f-8612: reject No Local on a v5 shared subscription
• f-8613: reject a Subscription Identifier on a client-to-server PUBLISH
• f-8614: refuse AUTH when the CONNECT carried no Authentication Method
• f-8616: refuse a v5 CONNECT whose Will QoS exceeds Maximum QoS instead of downgrading
• f-8617: refuse a v5 CONNECT with Will Retain when retained messages are unsupported
• f-10343: enforce Maximum Packet Size over the whole PUBLISH, not per transport write
• f-10344: refuse a CONNACK reporting Session Present after Clean Session
• f-10345: enforce Receive Maximum quota for write-only publishes, releasing on ack, cancel, and disconnect
• f-556: make the broker fan-out use-after-free guard O(1) via a subscription generation counter

[Copilot]

Pull request overview

This PR applies a set of MQTT v5 correctness and safety fixes across the client and embedded broker portions of wolfMQTT, with accompanying regression tests to lock in protocol compliance and concurrency behavior.

Changes:

• Adds stricter MQTT v5 protocol enforcement (shared-subscription validation, No Local rules, AUTH negotiation gating, illegal properties rejection, CONNACK session-present cross-check).
• Improves client correctness under QoS 2 and v5 flow control (inbound QoS2 de-dup, Receive Maximum enforcement for write-only publishes, whole-packet Maximum Packet Size enforcement).
• Hardens broker fan-out against re-entrant subscription frees with an O(1) guard using a subscription generation counter.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
wolfmqtt/mqtt_packet.h	Adjusts message/quota tracking fields and exposes MqttPacket_TopicFilterValid_ex API for v5-aware topic filter validation.
wolfmqtt/mqtt_client.h	Adds v5 auth negotiation state and inbound QoS2 packet-id tracking for duplicate suppression.
wolfmqtt/mqtt_broker.h	Adds subs_gen generation counter to support O(1) fan-out UAF guarding in dynamic-subscription mode.
src/mqtt_packet.c	Implements v5 shared-subscription topic-filter validation and enforces No Local constraints on shared subscriptions in SUBSCRIBE encode/decode.
src/mqtt_client.c	Implements inbound QoS2 de-dup, v5 AUTH gating, CONNACK Session Present validation, whole-packet max-size check, and Receive Maximum quota lifecycle changes.
src/mqtt_broker.c	Increments subs_gen on subscription list mutations and uses snapshot+revalidation in fan-out loops to avoid UAF under re-entrant closes.
tests/test_mqtt_packet.c	Adds tests for v5 shared-subscription filter rules and No Local rejection behavior (encode and decode).
tests/test_mqtt_client.c	Adds tests covering CONNACK session-present mismatch, AUTH gating, subscription-id rejection on client PUBLISH, max packet size enforcement, Receive Maximum behavior for write-only publishes, and QoS2 duplicate suppression.
tests/test_broker_connect.c	Adds a test that validates subs_gen bumps on unsubscribe and preserves baseline fan-out delivery behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #576

Scan targets checked: wolfmqtt-bugs, wolfmqtt-src

Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #576

Scan targets checked: wolfmqtt-bugs, wolfmqtt-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #576

Scan targets checked: wolfmqtt-bugs, wolfmqtt-src

Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #576

Scan targets checked: wolfmqtt-bugs, wolfmqtt-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[wolfSSL-Fenrir-bot]

Application-supplied Receive Maximum is not bounded by the QoS 2 dedup table size · Protocol state machine violations

When the application already supplies MQTT_PROP_RECEIVE_MAX, MqttClient_Connect sends it unchecked. A value above MQTT_MAX_RECV_QOS2 lets the server keep more QoS 2 messages in flight than recv_qos2_pending has slots; MqttClient_RecvQos2_Add (src/mqtt_client.c:501) then silently drops entries and retransmitted PUBLISHes are re-delivered to msg_cb, defeating the f-8609 dedup.

Related known finding #10345 (similar but distinct): Both involve MQTT v5 Receive Maximum enforcement in the client, but #10345 bypasses the server-advertised outbound publish quota in MqttPublishMsg. This candidate sends an application-supplied inbound limit exceeding the local QoS 2 dedup capacity in MqttClient_Connect; the faulting operations, root causes, and fixes are separate.

Fix: Clamp an application-supplied MQTT_PROP_RECEIVE_MAX to MQTT_MAX_RECV_QOS2, or reject the CONNECT when it exceeds that bound.

[wolfSSL-Fenrir-bot]

New Maximum Packet Size guard: header-overhead branch is never exercised · Missing edge-case coverage on a function the PR also changed

Both new tests only hit the publish->total_len > client->packet_sz_max disjunct (400 vs 300) or clear the limit outright (400 vs 1000). The substance of the f-10343 fix — the ((word32)rc - publish->intBuf_len) > (client->packet_sz_max - publish->total_len) term at src/mqtt_client.c:2879 — has no test, so deleting it would not fail CI.

Fix: Add a case where total_len is just under packet_sz_max so only the fixed+variable header overhead pushes the packet over the limit.

[aidangarske]

Skoll Multi-Scan Review

Modes: review + review-security

Overall recommendation: REQUEST_CHANGES
Findings: 6 total — 6 posted, 0 skipped
6 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [High] [review+review-security] Canceled write-only publishes can bypass Receive Maximum when reused — src/mqtt_client.c:613-616,2839-2844,3962-3968,4072-4075
• [High] [review+review-security] Reentrant subscription-list mutation guard truncates broker fan-out — src/mqtt_broker.c:5611-5615,7453-7457
• [High] [review] Rejected inbound QoS 2 messages leave a stale deduplication entry — src/mqtt_client.c:1174-1181
• [High] [review] Write-only publish timeouts are converted into an unbounded loop — examples/multithread/multithread.c:687-693
• [Medium] [review] Write-only PUBREC rejection behavior contradicts the public contract — src/mqtt_client.c:1232-1251
• [Medium] [review] AUTH positive test bypasses CONNECT property detection — tests/test_mqtt_client.c:780-789

Review generated by Skoll

[aidangarske]

Canceled write-only publishes can bypass Receive Maximum when reused · Logic

The PR newly reserves a Receive Maximum token for nonblocking write-only publishes and stores &publish->stat in recvQuotaStat, but MqttClient_RespList_Remove only nulls that pointer without clearing stat.recvQuotaHeld. MqttClient_CancelMessage resets the message to MQTT_MSG_BEGIN, and MqttClient_NetDisconnect also removes the pending response, so reusing the canceled object with a new packet ID makes MqttClient_RecvQuotaReserve treat the stale flag as an existing reservation and send without decrementing the live/new-connection quota. With Receive Maximum 1, cancel-and-reuse or disconnect/reconnect-and-reuse can put a second PUBLISH on the wire while the server still counts the first, exceeding the advertised Receive Maximum. A late acknowledgement cannot repair the accounting since its response entry was already removed. This violates the send-quota requirement in OASIS MQTT 5.0 section 4.9. The added cancellation test only asserts the quota stays held and the response disappears; it does not cover object reuse, reconnection, or a late ACK.

Fix: Move outstanding quota ownership into connection-owned state rather than the caller's reusable message object; for an already-transmitted canceled publish, retain an acknowledgement tombstone until PUBACK/PUBCOMP or connection close, and for disconnect, clear every associated recvQuotaHeld flag since that quota belongs to the old connection. Add Receive Maximum 1 tests for cancel-then-reuse, disconnect-then-reuse, and a late acknowledgement.

[aidangarske]

Reentrant subscription-list mutation guard truncates broker fan-out · Logic

The new generation guard avoids dereferencing a freed successor by breaking out of the fan-out loop, but that silently drops delivery to every still-live subscription after it, not just the freed one. For a list A -> B -> C, if writing to A services a WebSocket close for B (forwarding calls MqttPacket_Write, WebSocket writes synchronously call lws_service, and the close callback removes B's subscriptions and bumps subs_gen), next_sub is no longer linked and the new break prevents C from receiving the PUBLISH or Will even though C remained connected and subscribed; the publisher is then acknowledged as though fan-out completed. review-security rated this Medium (change_risk HIGH, blast_radius LOW) while review rated it High; kept at the stricter High given it is a confirmed message-loss bug reachable through executable broker paths. The added test only checks that an ordinary unsubscribe bumps the generation counter; it never exercises this reentrant multi-subscriber case.

Fix: Use mutation-safe iteration that can continue with the next surviving node — e.g. defer BrokerSub frees while fan-out is active and sweep tombstoned nodes afterward, or iterate over a stable snapshot. Add a deterministic re-entrant test (normal PUBLISH and Will) where the middle subscriber closes during the preceding subscriber's write and the trailing subscriber still receives exactly once.

[aidangarske]

Rejected inbound QoS 2 messages leave a stale deduplication entry · Logic

The application callback may set a v5 failure reason in publish->resp.reason_code, which is copied into the outgoing PUBREC. A PUBREC reason code at least 0x80 ends the QoS 2 exchange, so no PUBREL will arrive and the sender may reuse the packet identifier. The PR nevertheless records every QoS 2 identifier as awaiting PUBREL regardless of the reason code. That entry is never removed, and a later legitimate PUBLISH reusing the identifier is suppressed as a duplicate. There is no regression test for callback rejection followed by packet-ID reuse.

Fix: Only add the identifier when the PUBREC continues the QoS 2 exchange. Add a v5 test whose callback rejects a QoS 2 PUBLISH, verifies the negative PUBREC, then sends a fresh PUBLISH with the same identifier and verifies that the callback runs again.

Suggestion:

Suggested change

	#if WOLFMQTT_MAX_QOS >= 2
	if (packet_qos == MQTT_QOS_2
	#ifdef WOLFMQTT_V5
	&& (client->protocol_level <
	MQTT_CONNECT_PROTOCOL_LEVEL_5 ||
	(resp->reason_code & 0x80) == 0)
	#endif
	) {
	MqttClient_RecvQos2_Add(client, packet_id);
	}

[aidangarske]

Write-only publish timeouts are converted into an unbounded loop · Logic

The new code converts every command timeout back to MQTT_CODE_CONTINUE. mqtt_check_timeout resets startSec when it reports a timeout, so each conversion starts another complete timeout interval. If a broker never acknowledges an otherwise-written PUBLISH, the worker now polls forever and THREAD_JOIN cannot complete without an unrelated external stop. Before this PR, the timeout left the loop and the worker reported a terminal failure.

Fix: Keep the command timeout terminal, then disconnect or otherwise preserve outstanding quota conservatively. If periodic polling is desired, retain a separate bounded overall deadline or explicit stop condition. Add a test that withholds the ACK and verifies the multithread example terminates with an error.

[aidangarske]

Write-only PUBREC rejection behavior contradicts the public contract · api

The PR now marks the write-only publisher's PUBCOMP pending response done with MQTT_CODE_ERROR_PUBLISH_REJECTED, so its next nonblocking MqttClient_Publish_WriteOnly poll returns that error. The public documentation in wolfmqtt/mqtt_client.h still states this function never returns MQTT_CODE_ERROR_PUBLISH_REJECTED, and the adjacent source comment and ChangeLog still say the originating response is not completed and waits until timeout. The existing test inspects the pending node but never polls the public API, letting the contradiction pass.

Fix: Choose and document one contract. If returning the rejection on a later poll is intended, update the API documentation, source comment, and ChangeLog, and extend the test to poll MqttClient_Publish_WriteOnly and assert the error. Otherwise, do not complete the publisher's pending response with this code.

[aidangarske]

AUTH positive test bypasses CONNECT property detection · test

The positive test directly assigns test_client.auth_method_set = 1, so it tests only the final guard and cannot detect a failure in the new MqttConnect_HasAuthMethod helper or its assignment in MqttClient_Connect. It also does not verify the stated reconnect behavior that a subsequent CONNECT without an Authentication Method resets the flag.

Fix: Drive a canned successful v5 CONNECT carrying an Authentication Method, then call MqttClient_Auth. Reconnect without the property and verify the same AUTH call is rejected. This covers both the new detection helper and its per-connection reset.