Gh issues - #615
Gh issues#615
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are remaining correctness issues in the new client-side state management (notably side effects on error paths and Packet Identifier release semantics) that can re-enable in-flight Packet Identifier reuse on an active connection.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens MQTT 3.1.1/5.0 protocol conformance across the client and broker, addressing multiple reported issues around CONNECT sequencing, Packet Identifier lifetime, strict decoding, partial-write behavior, and WebSocket framing.
Changes:
- Add explicit client handshake state (
MQTT_CLIENT_FLAG_CONNECT_SENT) and enforce CONNECT-first / single-CONNECT-per-transport semantics across send APIs. - Add connection-level outbound Packet Identifier occupancy tracking to prevent reuse until the corresponding acknowledgement is processed.
- Harden broker/client protocol handling (strict v3.1.1 CONNACK length, reject publish-response Packet Identifier 0, complete CONNACK delivery before closing, gate keepalive while CONNACK is pending, drop QoS0 after partial failure, enforce WebSocket binary frames), plus add regression tests.
File summaries
| File | Description |
|---|---|
| wolfmqtt/mqtt_client.h | Introduces MQTT_CLIENT_FLAG_CONNECT_SENT and send_inflight/MQTT_MAX_SEND_INFLIGHT API/ABI surface changes. |
| wolfmqtt/mqtt_broker.h | Adds connack_refused state to keep refused CONNACKs alive until fully written. |
| src/mqtt_socket.c | Clears handshake flag on connect/disconnect to scope handshake state to the transport connection. |
| src/mqtt_packet.c | Enforces v3.1.1 empty-ClientId+CleanSession rule, strict v3.1.1 CONNACK remaining length, and rejects publish-response Packet Identifier 0. |
| src/mqtt_client.c | Implements CONNECT-first gating, CONNECT-once enforcement, send-inflight reservation/release, and ACK-driven Packet Identifier release. |
| src/mqtt_broker.c | Enforces WebSocket binary-only frames; fixes CONNACK partial-write close timing; gates keepalive while CONNACK pending; fixes DUP/QoS0 replay corner cases. |
| examples/websocket/net_libwebsockets.c | Closes WebSocket connection on non-binary frames per MQTT-over-WebSocket requirements. |
| tests/test_mqtt_props_init.c | Adjusts ping test preconditions to reflect CONNECT-first gating. |
| tests/test_mqtt_packet.c | Adds protocol regression tests for CONNACK length, CONNECT empty ClientId rule, and publish-response Packet Identifier validation. |
| tests/test_mqtt_client.c | Adds client regression tests for CONNECT-first enforcement, CONNECT-once enforcement, and Packet Identifier occupancy behavior. |
| tests/test_broker_connect.c | Adds broker regression tests covering refused/accepted CONNACK partial-write behavior, keepalive gating, and replay semantics. |
| ChangeLog.md | Documents the behavioral changes and the fixed issues. |
Review details
Suppressed comments (1)
src/mqtt_client.c:4504
MqttClient_CancelMessageunconditionally releases any reserved outbound Packet Identifier viaMqttClient_SendIdReleaseOwner(). This makes it possible for application code to cancel an unacknowledged QoS>0 PUBLISH / SUBSCRIBE / UNSUBSCRIBE and then immediately reuse the same Packet Identifier on the same connection, which conflicts with the new in-flight Packet Identifier occupancy enforcement and the MQTT requirement that identifiers remain in use until the corresponding ACK is processed.
/* Give back any outbound Packet Identifier this object reserved. Cancel is
* the application saying it is done with the exchange and is reusing the
* object, so the identifier must not stay claimed for the rest of the
* connection - unlike the Receive Maximum unit below, a reused identifier
* is the application's own call rather than a flow-control promise made to
* the server. */
MqttClient_SendIdReleaseOwner(client, msg);
- Files reviewed: 12/12 changed files
- Comments generated: 1
- 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
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #615
Scan targets checked: wolfmqtt-bugs, wolfmqtt-src
Findings: 6
6 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
There was a problem hiding this comment.
🟡 Changes recommended
The duplicate-CONNECT guard in MqttClient_Connect uses MqttClient_Flags(...,0,0) as a getter, which can return 0 on lockClient acquisition failure and incorrectly bypass the CONNECT-sent check under WOLFMQTT_MULTITHREAD.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #615
Scan targets checked: wolfmqtt-src, wolfmqtt-bugs
Findings: 9
9 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
aidangarske
left a comment
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security
Overall recommendation: REQUEST_CHANGES
Findings: 5 total — 5 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] [review+review-security] Reject sends once the in-flight ID tracker is full —
src/mqtt_client.c:854-865 - [High] [review] Cancellation releases an identifier that may still be on the wire —
src/mqtt_client.c:5139-5145 - [High] [review-security] ClientId hash collisions cross persistent-session state —
src/mqtt_client.c:509-521,3421-3492 - [Medium] [review-security] Protocol DISCONNECT permits a second CONNECT on the same transport —
src/mqtt_client.c:4768-4775 - [Medium] [review-security] Replay copies of application payloads are freed without zeroization —
src/mqtt_client.c:604-615
Review generated by Skoll
|
Do you want ChangeLog.md changes in there? |
I think its fine. It will be useful going forward. |
Uh oh!
There was an error while loading. Please reload this page.