Fix/fenrir13228

[ageprocpp]

Fenrir #13228

Description

For every MQTT v5 Topic Filter, the broker writes MQTT_REASON_SUCCESS even when BrokerSubs_Remove() found no matching subscription. MQTT_REASON_NO_SUB_EXIST is defined and accepted by the UNSUBACK encoder but is never produced here. The success assignment also occurs when the filter-length recovery guard fails, although that path is likely unreachable for successfully decoded packets.

Investigation

MQTT V5 specification requires that the reason code be “No subscription existed” when “No matching Topic Filter is being used by the Client,” and “Topic Filter invalid” when “The Topic Filter is correctly formed but is not allowed for this Client.” (l2459-2467)
The source should be modified to meet this specification.

Measures

• Have BrokerSubs_Remove to report whether it has succeeded to remove the subscription. (The return type was changed from void to int.)
• Set the reason code according to the return value of BrokerSubs_Remove and the result of filter-length guard.

Test Added

unsubscribe_v5_reason_codes

[wolfSSL-Bot]

Can one of the admins verify this patch?

[Copilot]

🟡 Changes recommended

Critical build issues and the deleted public version header must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates MQTT v5 UNSUBSCRIBE reason-code handling and adds regression tests.

Changes:

• Returns subscription-removal status and emits appropriate UNSUBACK codes.
• Adds coverage for matching and missing subscriptions.
• Deletes the tracked public version header.

File summaries

File	Review summary
wolfmqtt/version.h	Critical: Restore the tracked fallback/public header; its deletion breaks non-Autoconf builds.
tests/test_broker_connect.c	Nits: Fix the spelling and incorrect RMQTT_REASON_SUCCESS reference in comments.
src/mqtt_broker.c	Critical: Provide bool definitions and guard removed for non-v5 builds.

Review details

Suppressed comments (1)

wolfmqtt/version.h:1

• Deleting this tracked header removes the fallback copy that the template says is included for builds that do not run configure; it is also installed as a public header and included by examples such as awsiot.c. A fresh checkout or non-Autoconf/package build will therefore fail to find wolfmqtt/version.h. Restore the generated header.

• Files reviewed: 3/3 changed files
• Comments generated: 4
• Review effort level: Lite

---

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

[Copilot]

🟡 Changes recommended

Critical build blockers remain in src/mqtt_broker.c and wolfmqtt/version.h.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

wolfmqtt/version.h:1

• Deleting this checked-in public header breaks builds that do not run Autoconf: CMakeLists.txt:397-403 installs headers directly from wolfmqtt/, while the examples include <wolfmqtt/version.h>. Although configure.ac can regenerate it from version.h.in, the CMake and other non-configure paths do not, so restore the generated header or add equivalent generation logic to every affected build path.

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The WOLFMQTT_V5 build fails because false is used without including <stdbool.h>.

Get a fresh assessment by requesting another Copilot review.

Review details

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

[Copilot]

🔵 Needs a closer look

Correct the undeclared false initializer in src/mqtt_broker.c to avoid C build failures.

Review details

Suppressed comments (1)

src/mqtt_broker.c:7250

• Although removed is now an int, this initializer still uses false; mqtt_broker.c does not include <stdbool.h> and the project headers do not define it, so C builds fail with an undeclared identifier. Initialize it with the existing integer convention instead.

        int removed = false;

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

[Copilot]

🟢 Approval recommended

The requested behavior is implemented with regression coverage.

Review details

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

[kojiws]

I left some comments as suggestions.

[kojiws]

Thank you for reflecting my comments.
LGTM

[kojiws]

@embhorn
Could you review this PR?

@ageprocpp is an intern from Japan.
@cconlon completed all paper works needed to merge this PR.

[embhorn]

Skoll Code Review

Scan type: review

Overall recommendation: COMMENT
Findings: 5 total — 2 posted, 3 skipped
2 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Medium] Filter-length guard failure reports 0x8F "Topic Filter invalid", which the spec reserves for an authorization outcome — src/mqtt_broker.c:7521
• [Medium] BrokerSubs_Remove's new int return contract is undocumented and inverts the file's 0-is-success convention — src/mqtt_broker.c:4407-4409

Skipped findings

• [Medium] No test covers multiple Topic Filters in one UNSUBSCRIBE, so per-index reason-code ordering is unverified
• [Low] BrokerSubs_Remove is called from two preprocessor-duplicated sites and its result is discarded without a (void) cast in non-V5 builds
• [Low] Second half of the new test decodes the output buffer without first asserting the broker produced any bytes

Review generated by Skoll

[embhorn]

A couple small changes suggested by skoll

[ageprocpp]

Reflected the changes suggested.

[embhorn]

Excellent work, @ageprocpp ! Thanks for adding this fix to the project.

[embhorn]

@kojiws - This is ready for merge. The Zephyr test failure will pass once #621 is merged.

[kojiws]

LGTM

Thanks for reviewing and fixing it.
I'll run the test again after #621 is merged.

[aidangarske]

@ageprocpp looks like CI is failing can you please fix?

[ageprocpp]

@kojiws Could you rerun the CI? I don’t have permission to do so.

[aidangarske]

@kojiws Could you rerun the CI? I don’t have permission to do so.

Its ran twice already and failed it looks pr related please look into it

[kojiws]

@ageprocpp
Could you rebase the base branch onto the latest base so that it picks up the #621 changes?

[ageprocpp]

@kojiws Now this branch is rebased from updated master. Please check.

[kojiws]

@embhorn
This has been reviewed and all tests are passing. Could you merge it when you get a chance?