Skip to content

Fix MQTT 5 property validation and QoS 2 state handling in broker and client - #618

Merged
embhorn merged 5 commits into
wolfSSL:masterfrom
aidangarske:fenrir-fixes-13223-13224-13237-13238
Sep 17, 2026
Merged

embhorn merged 5 commits into
wolfSSL:masterfrom
aidangarske:fenrir-fixes-13223-13224-13237-13238

Conversation

@aidangarske

Copy link
Copy Markdown
Member
F-13223, F-13224, F-13237, F-13238

@aidangarske aidangarske self-assigned this Sep 16, 2026
Copilot AI lite review requested due to automatic review settings September 16, 2026 22:02

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #618

Scan targets checked: wolfmqtt-src, wolfmqtt-bugs

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.

Comment thread src/mqtt_client.c Outdated

@embhorn embhorn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skoll Code Review

Scan type: review

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

Posted findings

  • [High] Byte-property range check added only to the encoder makes the broker silently drop v5 PUBLISHes it already ACKed as SUCCESSsrc/mqtt_packet.c:917-924
  • [Medium] Retained-store failure now suppresses live fan-out for v5 QoS 1/2 only, inverting QoS reliability orderingsrc/mqtt_broker.c:7827-7839
  • [Medium] MqttClient_Connect silently rewrites caller-owned MqttProp memory, contradicting the non-mutating pattern used three lines belowsrc/mqtt_client.c:3268-3282
  • [Medium] New error branch and the changed v5 QoS 1 behavior have no testssrc/mqtt_broker.c:7148-7158
  • [Low] MQTT_MAX_PROPS duplicated into a second .c file and a test instead of a public headersrc/mqtt_client.c:29-34
  • [Low] README still advertises Topic Alias without noting inbound aliases are now rejectedsrc/mqtt_client.c:1414-1429
  • [Low] New line exceeds the project's 80-column limitsrc/mqtt_client.c:1425
  • [Low] Fan-out gate uses a preprocessor-conditional bare if stacked in front of another ifsrc/mqtt_broker.c:7831-7835

Skipped findings

  • [Medium] New CONNECT refusal runs after the Will is stored, breaking the invariant BrokerClient_EndConnect documents
  • [Low] New PUBLISH rejection frees properties regardless of doProps and is redundant on the packet_obj path

Review generated by Skoll

Comment thread src/mqtt_packet.c
Comment thread src/mqtt_broker.c Outdated
Comment thread src/mqtt_client.c Outdated
Comment thread src/mqtt_broker.c
Comment thread src/mqtt_client.c Outdated
Comment thread src/mqtt_client.c
Comment thread src/mqtt_client.c Outdated
Comment thread src/mqtt_broker.c Outdated
@embhorn embhorn removed their assignment Sep 17, 2026

@embhorn embhorn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review the Skoll feedback

@embhorn
embhorn merged commit 42c837f into wolfSSL:master Sep 17, 2026
47 of 48 checks passed
@aidangarske
aidangarske deleted the fenrir-fixes-13223-13224-13237-13238 branch September 17, 2026 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants