Harden QoS delivery, persistent sessions, reauthentication, and MQTT SN sends

[aidangarske]

F-12444, F-12445, F-12478, F-12479, F-12443

[Copilot]

🟡 Changes recommended

The MQTT-SN resume/write-ownership path lacks a verified thread-ownership guard on resumed writes, which can allow non-owner threads to call into MqttPacket_Write without holding lockSend in multithreaded/non-blocking scenarios.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens MQTT client/broker behavior around QoS delivery correctness, persistent session durability, MQTT v5 re-authentication constraints, and MQTT-SN send/resume semantics across non-blocking partial writes.

Changes:

• Enforces MQTT v5 re-authentication to reuse the CONNECT Authentication Method, retaining a bounded copy on the client and rejecting mismatches pre-wire.
• Tightens inbound QoS 2 handling when the dedup table is full (drop/deny delivery instead of risking duplicate application delivery), with protocol-appropriate behavior for MQTT 3.1.1 vs v5.
• Refactors MQTT-SN send paths to use centralized write-ownership handling (MqttWriteStart/Stop) and adds targeted regression tests for partial-write resume and error recovery.

File summaries

File	Description
wolfmqtt/mqtt_client.h	Adds bounded storage for CONNECT auth method (v5) and persistent MQTT-SN disconnect state.
src/mqtt_client.c	Implements auth-method matching enforcement; hardens QoS2 dedup-table-full behavior; clears v5 PUBREC reason on reused objects.
src/mqtt_sn_client.c	Introduces SN_Client_WriteOwned and transitions MQTT-SN sends to ownership-based write/resume model; adds NULL disconnect persistent state.
src/mqtt_broker.c	Prevents PUBREC from advancing non-QoS2 outqueue entries; preserves persisted outbound queue records across orphan reclaim.
tests/test_mqtt_client.c	Adds tests for auth-method mismatch rejection and QoS2 dedup-table-full behavior (v3.1.1 disconnect vs v5 quota PUBREC).
tests/test_mqtt_sn_client.c	Adds tests for NULL disconnect chunked write framing, publish reuse after write error, and multithread partial-write send-state retention.
tests/test_broker_connect.c	Adds tests for PUBREC-on-QoS1 protocol error handling and persisted OUTQ retention during orphan reclaim.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 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 #579

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.

[embhorn]

Skoll review — 4 High, 2 Medium

Automated skoll review pass over the PR diff, filtered to High and Medium severity findings (all at High confidence). Each is left as an inline comment; all six were re-verified by hand against head 598f761.

High

1. tests/test_mqtt_sn_client.c — TEST(sn_publish_reusable_after_write_error) is defined unconditionally but registered under a NONBLOCK || MULTITHREAD guard, so ./configure --enable-sn fails on -Werror/-Wunused-function. Four CI workflows run that config.
2. src/mqtt_sn_client.c — SN_WillMessage misses the new short-write→error mapping, so a zero-byte WILLMSG write returns MQTT_CODE_SUCCESS and CONNECT stalls to timeout.
3. src/mqtt_sn_client.c — same bug in SN_Client_WillMsgUpdate, on a public API, where the failure is completely invisible to the caller.
4. src/mqtt_client.c — on MQTT 3.1.1, a QoS 2 message dropped for a full dedup table is still PUBREC'd, so the publisher records an exactly-once delivery the application never saw. A behavioral regression vs. pre-PR.

Medium

5. src/mqtt_client.c — new comment in MqttClient_RecvQos2_Add claims a free-slot invariant that does not hold on 3.1.1.
6. src/mqtt_sn_client.c — SN_Client_Publish discards a short-write error for QoS 1/2 and falls through to the wait state.

Findings 2, 3 and 6 share one root cause: the new SN_Client_WriteOwned helper got the short-write semantics right, but the three send paths that bypass it did not. Findings 4 and 5 are two views of the same 3.1.1 QoS 2 gap.

Ten Low/Info findings were filtered out of this post; notable ones were the SN_Client_Disconnect/_ex mutual call (no-recursion rule), disconnectSN added mid-struct in MqttClient rather than at the end, and MQTT_AUTH_METHOD_MAX lacking the range validation its sibling MQTT_MAX_RECV_QOS2 has.

[embhorn]

HIGH — Unused static test function breaks the --enable-sn build (-Werror)

The #endif at line 1992 closes the #ifdef WOLFMQTT_NONBLOCK guard opened at 1971, so TEST(sn_publish_reusable_after_write_error) here is defined unconditionally. But it is registered only under #if defined(WOLFMQTT_NONBLOCK) || defined(WOLFMQTT_MULTITHREAD) (lines 2975-2977).

unit_test.h expands TEST(name) to static void test_##name(void), so in a build with WOLFMQTT_SN but neither WOLFMQTT_NONBLOCK nor WOLFMQTT_MULTITHREAD this is an unreferenced static function. The generated CFLAGS include -Werror -Wall, so -Wunused-function becomes a hard error. ./configure --enable-sn is exactly that config, and it is run by four CI workflows: ubuntu-check.yml, fsanitize-check.yml, macos-check.yml, and mqtt-sn-check.yml.

Recommendation: drop the registration guard at 2975-2977 so the test runs in plain blocking builds too — the test only fails a write and retries, so it should pass there, and that gives the --enable-sn config real coverage. (Alternatively, wrap the definition in the same guard.) Verify with ./configure --enable-sn && make check.

[embhorn]

HIGH — SN_WillMessage returns MQTT_CODE_SUCCESS for a zero-byte write, so CONNECT continues as if the will was sent

This PR adds the shared helper SN_Client_WriteOwned, which correctly maps a non-negative short write to an error:

return (rc >= 0) ? MQTT_TRACE_ERROR(MQTT_CODE_ERROR_NETWORK) : rc;

SN_WillMessage hand-rolls its MqttPacket_Write call (it must scrub the will payload first) and was not given that mapping. In the blocking path MqttSocket_Write (src/mqtt_socket.c:209-222) breaks its loop when MqttSocket_WriteDo returns <= 0; a user net callback returning exactly 0 leaves rc == 0, the if (rc > 0) at line 226 is false, and MqttSocket_Write returns 0. MqttPacket_HandleNetError passes 0 through unchanged.

Here rc == 0 != xfer, so the if (rc == xfer) normalization is skipped and rc stays 0 == MQTT_CODE_SUCCESS. SN_Client_Connect then does rc = SN_WillMessage(...); if (rc != 0) return rc;, sees success, sets will_done = SN_WILL_DONE_ALL, and waits for CONNACK — while the gateway is still waiting for a WILLMSG that never reached the transport. The connect hangs until cmd_timeout_ms with no diagnostic.

Note SN_WillTopic, the sibling half of the same handshake, was converted to SN_Client_WriteOwned, so the two halves now behave inconsistently.

Recommendation: map a non-negative short write to MQTT_CODE_ERROR_NETWORK, matching SN_Client_WriteOwned. Add a regression test using the existing g_mock.write_zero_count hook asserting SN_Client_Connect with a will does not return success when the WILLMSG write transfers zero bytes.

[embhorn]

HIGH — SN_Client_WillMsgUpdate returns MQTT_CODE_SUCCESS for a zero-byte write on a public API

Same root cause as the SN_WillMessage case above, but on a public API, and here the failure is completely invisible to the application.

When MqttPacket_Write returns 0 (blocking-path zero-byte write — see src/mqtt_socket.c:209-222 and 226-230), this rc != xfer branch unlinks the pending response, resets will->stat.write = MQTT_MSG_BEGIN, and then return rc — returning the raw 0, which is MQTT_CODE_SUCCESS (wolfmqtt/mqtt_types.h:207).

So the application is told the will-message update succeeded, the WILLMSGRESP pending response has already been removed so no reply will ever be matched, and the WILLMSGUPD packet never left the transport. This PR's own SN_Client_WriteOwned handles precisely this case.

Recommendation: normalize a non-negative short write to MQTT_CODE_ERROR_NETWORK before returning, so the caller can distinguish it from a completed update. Cover with a g_mock.write_zero_count test asserting SN_Client_WillMsgUpdate returns a negative code.

[embhorn]

HIGH — MQTT 3.1.1: a QoS 2 message is silently dropped but positively acknowledged when the dedup table is full

When a new QoS 2 packet id arrives and recv_qos2_pending is full, untrackable is set, which feeds suppress_cb so msg_cb is never invoked — the payload is drained and discarded. For MQTT v5 the code sets MQTT_REASON_QUOTA_EXCEEDED, which correctly ends the exchange.

But on the MQTT 3.1.1 path this block does nothing at all: the entire body is inside #ifdef WOLFMQTT_V5, and even in a V5 build the inner if requires protocol_level >= 5. Execution continues with rc == MQTT_CODE_SUCCESS, MqttClient_HandlePacket sets resp->packet_type = MQTT_PACKET_TYPE_PUBLISH_REC with a success reason, the peer replies PUBREL, the client replies PUBCOMP, and the publisher records a completed exactly-once delivery. The application never saw the message and MqttClient_WaitMessage returns MQTT_CODE_SUCCESS with no error whatsoever.

The new test wait_message_qos2_full_dedup_table_v311_pubrec_kept_open codifies this (ASSERT_EQ(0, g_dedup_msg_cb_calls); ASSERT_TRUE(g_pubresp_written); ASSERT_EQ(MQTT_CODE_SUCCESS, rc);).

This is a behavioral regression: the pre-PR code delivered the message (worst case, a later retransmit could arrive twice), whereas the new code guarantees silent loss under a positive ack. For QoS 2, whose entire contract is exactly-once delivery, trading a possible duplicate for a guaranteed undetectable loss should be a deliberate and signalled decision. Compare the sibling client->msg_cb == NULL case immediately below, which deliberately returns MQTT_CODE_ERROR_CALLBACK so the caller is notified rather than falsely acking.

Recommendation: on the 3.1.1 path, do not ack a message that was never delivered. Preferred: return a distinct error (mirroring the MQTT_CODE_ERROR_CALLBACK precedent below) and skip the PUBREC, so the peer retransmits and the message is delivered once a slot frees — that is normal MQTT receive-side flow control, not a livelock, since the peer is bounded by its own in-flight window. If dropping really is the intended policy, it should at minimum surface a distinct return code so the loss is observable; a silent MQTT_CODE_SUCCESS is not safe for QoS 2.

Minor, same block: the 3.1.1 explanatory comment sits inside #ifdef WOLFMQTT_V5, so it disappears in exactly the builds it describes. Move it outside the guard.

[embhorn]

MEDIUM — This new comment asserts an invariant the code does not hold

The PR replaced the previous (accurate) comment about the table-full fall-through with a claim that a free slot is guaranteed. That is false on the MQTT 3.1.1 path.

In MqttClient_HandlePacket the guard is:

packet_qos == MQTT_QOS_2 &&
    (client->protocol_level < MQTT_CONNECT_PROTOCOL_LEVEL_5 ||
     (resp->reason_code & 0x80) == 0)

For a 3.1.1 connection the first disjunct is true, so MqttClient_RecvQos2_Add is called unconditionally — including for the untrackable id whose whole premise is that no free slot exists. The loop below then finds nothing and returns silently.

The behaviour is unchanged from before, but the code is now documented as having a guarantee it does not have, which will mislead the next reader into treating the silent fall-through as dead code.

Recommendation: restore an accurate comment covering the 3.1.1 case. If the 3.1.1 policy changes per the MqttClient_Publish_ReadPayload comment, revisit this wording again.

[embhorn]

MEDIUM — Short-write error is discarded for QoS 1/2, then the client waits for an ack that will never come

On a non-negative short write (rc == 0, reachable per src/mqtt_socket.c:209-222) the else branch below sets rc = -1 — which is MQTT_CODE_ERROR_BAD_ARG, a misleading code for a transport failure — and then, for QoS 1 and QoS 2, does not return. It sets publish->stat.write = MQTT_MSG_WAIT and falls through into case MQTT_MSG_WAIT, where rc = SN_Client_WaitType(...) overwrites the error entirely. The client then blocks waiting for a PUBACK/PUBCOMP for a packet that never reached the transport, until cmd_timeout_ms expires.

This is verbatim the failure mode this PR's own SN_Client_WriteOwned comment says it prevents: "so callers do not advance to awaiting a reply for a packet that never fully reached the transport." The rc = -1 line predates the PR, but this block was rewritten here (snapshotting xfer, inserting MqttWriteStop, restructuring the CONTINUE branch) and the fix was applied to nine other send paths via the new helper — leaving this one inconsistent.

Recommendation: return MQTT_CODE_ERROR_NETWORK immediately on a short write instead of setting rc = -1 and falling through to the wait state. Add a g_mock.write_zero_count test for SN_Client_Publish at QoS 1 asserting a negative return rather than a timeout.

[embhorn]

Please review skoll findings