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]

Filter-length guard failure reports 0x8F "Topic Filter invalid", which the spec reserves for an authorization outcome

The new default initializer sets reasons[i] = MQTT_REASON_TOPIC_FILTER_INVALID (0x8F) and that value survives only when the MqttDecode_Num((byte*)f - MQTT_DATA_LEN_SIZE, ...) length-recovery guard fails. MQTT v5 §3.11.3 Table 3-9 defines 0x8F as "The Topic Filter is correctly formed but is not allowed for this Client" — an authorization/policy outcome. The condition being signalled here is the opposite: the server could not recover the filter's length prefix, i.e. an internal/server-side failure, for which the table provides 0x80 ("Unspecified error … none of the other Reason Codes apply") or 0x83 ("Implementation specific error: the UNSUBSCRIBE is valid but the Server does not accept it"). The PR description itself quotes the 0x8F wording, so the chosen code contradicts the stated rationale. Impact is limited because the branch is effectively unreachable — MqttDecode_Unsubscribe propagates any decode error (src/mqtt_packet.c:3598-3607), so every topics[i].topic_filter in range is non-NULL and MqttDecode_Num on a valid 2-byte prefix always returns MQTT_DATA_LEN_SIZE. But if it ever does fire, a wolfMQTT peer maps any code with bit 0x80 set to MQTT_CODE_ERROR_UNSUBSCRIBE_REJECTED (src/mqtt_client.c:4577-4582) and MqttClient_ReturnCodeToString will report "Topic Filter invalid", misdirecting whoever debugs it.

Suggested fix: Use MQTT_REASON_UNSPECIFIED_ERR (0x80) — the catch-all the table designates for cases where no other code applies — or MQTT_REASON_IMPL_SPECIFIC_ERR (0x83) for this server-side failure, and reserve MQTT_REASON_TOPIC_FILTER_INVALID for a future policy/ACL rejection. Both alternatives are already accepted by MqttPacket_UnsubAckReasonCodeValid (src/mqtt_packet.c:296-311), so the encoder change is safe. Add a short comment on the initializer stating which failure it represents.
Basis: MQTT Version 5.0 OASIS Standard §3.11.3 Table 3-9: 0x8F "Topic Filter invalid — The Topic Filter is correctly formed but is not allowed for this Client"; 0x80 "Unspecified error — The unsubscribe could not be completed and the Server either does not wish to reveal the reason or none of the other Reason Codes apply".

Suggestion:

Suggested change

	reasons[i] = MQTT_REASON_TOPIC_FILTER_INVALID;
	reasons[i] = MQTT_REASON_UNSPECIFIED_ERR;

[embhorn]

BrokerSubs_Remove's new int return contract is undocumented and inverts the file's 0-is-success convention

The return type changed from void to int with boolean semantics (1 = removed, 0 = not found), but nothing at the declaration says so. Everywhere else in this file an int return follows the wolfMQTT convention of MQTT_CODE_SUCCESS (0) for success and negative for errors, so a reader scanning if (BrokerSubs_Remove(broker, bc, f, flen)) at line 7526 will plausibly parse it as an error check and conclude the SUCCESS/NO_SUB_EXIST branches are swapped. The immediately preceding BrokerSubs_Add sets the precedent for documenting a non-standard int return — it carries an inline /* 1 = newly created (vs. 0 = updated), for Retain Handling = 1. */ at line 4401 — and the new function has no equivalent. This matters most for future maintainers who add a third outcome (e.g. a negative error) and silently break the truthiness test at the call site.

Suggested fix: Add a doc comment above BrokerSubs_Remove stating the boolean return contract and explicitly flagging that it is not the MQTT_CODE_* convention, mirroring the inline note already present on BrokerSubs_Add. If a negative error outcome is ever added, change the call site to > 0 rather than bare truthiness.
Basis: wolfMQTT project convention: MQTT_CODE_SUCCESS is 0 with negative values for errors, per the MqttPacketResponseCodes enum in wolfmqtt/mqtt_types.h.

Suggestion:

Suggested change

	static int BrokerSubs_Remove(MqttBroker* broker, BrokerClient* bc,
	/* Remove the subscription owned by 'bc' whose Topic Filter matches exactly.
	* Returns 1 if a matching subscription was found and removed, 0 if this
	* client had no matching subscription. Note this is a boolean result, not
	* the MQTT_CODE_SUCCESS(0) / negative-error convention used elsewhere. */
	static int BrokerSubs_Remove(MqttBroker* broker, BrokerClient* bc,
	const char* filter, word16 filter_len)
	{

[embhorn]

A couple small changes suggested by skoll