Stop outbound drain when Session hand-off takes its queue. Claim Will before publishing it. Do not report a successful cancel while a reader owns the request. Take the client lock over the Session replay pool in MqttClient_Connect. by kareem-wolfssl · Pull Request #622 · wolfSSL/wolfMQTT

kareem-wolfssl · 2026-09-24T23:29:12Z

No description provided.

Copilot

Copilot review overview

🟡 Changes recommended

Build failure, replay races, non-terminating cancellation, and duplicate QoS 0 delivery remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3 · 1

Open (4)

Completed QoS 0 writes can be replayed after re-entrant close · New
Session reset can delete publishes added during connection setup · New
Missing forced-include allocator header breaks unit-test compilation · New
Terminal handler errors leave cancellation permanently stuck · New

What changed in this PR

Hardens MQTT client and broker state ownership during concurrent cancellation, session replay, queue hand-off, and Will publication.

Changes:

Adds synchronization and cancellation ownership safeguards.
Prevents broker queue and Will reuse during re-entrant callbacks.
Adds regression tests and allocator instrumentation.

File	Description
`wolfmqtt/mqtt_client.h`	Documents cancellation ownership semantics.
`src/mqtt_client.c`	Synchronizes replay state and pending-response cancellation.
`src/mqtt_broker.c`	Handles re-entrant queue transfer and Will publication.
`tests/test_mqtt_client.c`	Tests replay locking and cancellation.
`tests/test_broker_connect.c`	Tests queue hand-off and Will re-entry.
`tests/include.am`	Injects allocator hooks into unit tests.

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

embhorn

Review skoll comments and failing tests

…kes its queue. Thanks to Gwanhyun Lee for the report.

Thanks to Gwanhyun Lee for the report.

…s the request. Thanks to Gwanhyun Lee for the report.

… MqttClient_Connect. Thanks to Gwanhyun Lee for the report.

Copilot

Copilot review overview

🟡 Changes recommended

Replay provenance can be misclassified across connections, and broker persistence can retain stale DUP state.

Review effort: Balanced
Findings: 2 · 1

Open (3)

Replay provenance inferred from packet IDs, causing stale publish replay · New
Replay includes current-connection publishes and corrupts packet ownership · New
Persisted orphan retains QUEUED state and misses retransmit DUP flag · New

Resolved since last review (4)

Missing forced-include allocator header breaks unit-test compilation
Session reset can delete publishes added during connection setup
Completed QoS 0 writes can be replayed after re-entrant close
Terminal handler errors leave cancellation permanently stuck

+        if (client->replay[i].packet_id != 0 &&
+                MqttClient_SendIds_Find(client,
+                    client->replay[i].packet_id) >= 0) {
+            continue; /* published on this connection */


+                (void)MqttClient_SendIdReserve_Locked(client,
                    client->replay[i].packet_id, &client->replay[i], 1,
                    client->replay[i].pubrelSent ?
                        MQTT_PACKET_TYPE_PUBLISH_COMP :
                        ((client->replay[i].qos == MQTT_QOS_2) ?
                            MQTT_PACKET_TYPE_PUBLISH_COMP :
                            MQTT_PACKET_TYPE_PUBLISH_ACK));


+        if (cur->qos != MQTT_QOS_0) {
+            cur->retransmit_dup = 1;
+            return;
+        }


wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #622

Scan targets checked: wolfmqtt-src, wolfmqtt-bugs
Coverage: 2 of 5 in-scope changed file(s) opened by the reviewer; not opened: tests/test_broker_connect.c, tests/test_mqtt_client.c, wolfmqtt/mqtt_client.h

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

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

Review tier: Lite

wolfSSL-Fenrir-bot · 2026-10-02T21:40:46Z

+                tmpResp->packet_type, tmpResp->packet_id,
+                tmpResp->packetProcessing, tmpResp->packetDone);
+        #endif
+            if (tmpResp->packetProcessing && !tmpResp->packetDone) {


New CONTINUE return from CancelMessage is ignored by internal error paths, leaving a stale pendResp linked · API contract violations

MqttClient_CancelMessage can now return MQTT_CODE_CONTINUE without unlinking the pendResp or resetting stat. The failure paths in Publish (4247, 4294), Subscribe (4543), Unsubscribe (4698), Ping (4862) and Connect (3457) ignore that return and report the error anyway. The caller can then free or reuse an object that is still on firstPendResp, which leaves a dangling list node.

Suggested fix: Update every internal caller of MqttClient_CancelMessage to handle MQTT_CODE_CONTINUE. Either wait until the reader sets packetDone and cancel again, or keep the object owned instead of returning an error.

Related known findings (similar but distinct; listed for context, not part of this finding)

F-13261 (open): File/function: same function MqttClient_CancelMessage as F-13261. Operation: F-13261 is the active-read release path omitting an rx_buf scrub; candidate is the packetProcessing-and-not-done early return skipping RespList_Remove when callers ignore MQTT_CODE_CONTINUE. Root cause: F-13261 is missing buffer zeroization (info disclosure); candidate is a missing caller-side contract check leaving a stale linked-list node (dangling pointer / potential use-after-free). Patch: one requires clearing rx_buf

kareem-wolfssl requested review from embhorn and a balanced review from Copilot September 24, 2026 23:29

kareem-wolfssl self-assigned this Sep 24, 2026

Copilot started reviewing on behalf of kareem-wolfssl September 24, 2026 23:37 View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

Comment thread src/mqtt_broker.c

Comment thread src/mqtt_client.c Outdated

Comment thread tests/include.am Outdated

Comment thread src/mqtt_client.c

embhorn requested changes Oct 2, 2026

View reviewed changes

kareem-wolfssl added 5 commits October 2, 2026 12:19

wolfMQTT broker: stop the outbound drain when the Session hand-off ta…

fca9678

…kes its queue. Thanks to Gwanhyun Lee for the report.

wolfMQTT broker: claim the Will before publishing it.

83ebb13

Thanks to Gwanhyun Lee for the report.

wolfMQTT client: do not report a successful cancel while a reader own…

05abc80

…s the request. Thanks to Gwanhyun Lee for the report.

wolfMQTT client: take the client lock over the Session replay pool in…

333e5cc

… MqttClient_Connect. Thanks to Gwanhyun Lee for the report.

Address code review feedback and minimize comments.

ef23354

kareem-wolfssl force-pushed the zd22494 branch from 3908b69 to ef23354 Compare October 2, 2026 19:57

kareem-wolfssl requested review from wolfSSL-Fenrir-bot and a balanced review from Copilot October 2, 2026 21:31

Copilot started reviewing on behalf of kareem-wolfssl October 2, 2026 21:32 View session

Copilot AI reviewed Oct 2, 2026

View reviewed changes

wolfSSL-Fenrir-bot requested changes Oct 2, 2026

View reviewed changes

Navigation Menu

Stop outbound drain when Session hand-off takes its queue. Claim Will before publishing it. Do not report a successful cancel while a reader owns the request. Take the client lock over the Session replay pool in MqttClient_Connect. - #622

kareem-wolfssl commented Sep 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

embhorn left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

wolfSSL-Fenrir-bot left a comment

Uh oh!

wolfSSL-Fenrir-bot Oct 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

kareem-wolfssl commented Sep 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

embhorn left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #622

Uh oh!

wolfSSL-Fenrir-bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants