From 00101cd881e4260a5a41344e45d21c198768d268 Mon Sep 17 00:00:00 2001 From: kaage Date: Sat, 12 Sep 2026 03:50:19 +0900 Subject: [PATCH 1/5] Modified code to meet specification --- src/mqtt_broker.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/mqtt_broker.c b/src/mqtt_broker.c index b065993cf..cc65fbc93 100644 --- a/src/mqtt_broker.c +++ b/src/mqtt_broker.c @@ -4408,7 +4408,7 @@ static int BrokerSubs_Add(MqttBroker* broker, BrokerClient* bc, return rc; } -static void BrokerSubs_Remove(MqttBroker* broker, BrokerClient* bc, +static bool BrokerSubs_Remove(MqttBroker* broker, BrokerClient* bc, const char* filter, word16 filter_len) { #ifdef WOLFMQTT_STATIC_MEMORY @@ -4431,7 +4431,7 @@ static void BrokerSubs_Remove(MqttBroker* broker, BrokerClient* bc, if (bc->sub_count > 0) { bc->sub_count--; } - return; + return true; } } #else @@ -4460,12 +4460,13 @@ static void BrokerSubs_Remove(MqttBroker* broker, BrokerClient* bc, if (bc->sub_count > 0) { bc->sub_count--; } - return; + return true; } prev = cur; cur = next; } #endif + return false; } /* -------------------------------------------------------------------------- */ @@ -7544,12 +7545,17 @@ static int BrokerHandle_Unsubscribe(BrokerClient* bc, int rx_len, for (i = 0; i < unsub.topic_count && i < MAX_MQTT_TOPICS; i++) { const char* f = unsub.topics[i].topic_filter; word16 flen = 0; + bool removed = false; if (f && MqttDecode_Num((byte*)f - MQTT_DATA_LEN_SIZE, &flen, MQTT_DATA_LEN_SIZE) == MQTT_DATA_LEN_SIZE) { - BrokerSubs_Remove(broker, bc, f, flen); + removed = BrokerSubs_Remove(broker, bc, f, flen); +#ifdef WOLFMQTT_V5 + if (removed) reasons[i] = MQTT_REASON_SUCCESS; + else reasons[i] = MQTT_REASON_NO_SUB_EXIST; +#endif } #ifdef WOLFMQTT_V5 - reasons[i] = MQTT_REASON_SUCCESS; + else reasons[i] = MQTT_REASON_TOPIC_FILTER_INVALID; #endif } From bc5da653b2711e7c22fb05f32d8bc59216573764 Mon Sep 17 00:00:00 2001 From: kaage Date: Sat, 12 Sep 2026 05:26:19 +0900 Subject: [PATCH 2/5] Added tests checking reason codes --- tests/test_broker_connect.c | 84 +++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/tests/test_broker_connect.c b/tests/test_broker_connect.c index 1c06c3bf0..6b78b27b5 100644 --- a/tests/test_broker_connect.c +++ b/tests/test_broker_connect.c @@ -1514,6 +1514,89 @@ TEST(connect_v5_emptyid_clean0_accepted) MqttBroker_Stop(&broker); MqttBroker_Free(&broker); } + +TEST(unsubscribe_v5_reason_codes) +{ + MqttBroker broker; + MqttBrokerNet net; + int i; + int rc; + MqttUnsubscribeAck ack; + /* CONNECT subscriber "A". */ + static const byte connect_sub[] = { + 0x10, 0x0E, 0x00, 0x04, 'M', 'Q', 'T', 'T', 0x05, 0x02, 0x00, 0x3C, + 0x00, + 0x00, 0x01, 'A' + }; + /* SUBSCRIBE packet_id=1, filter "x", QoS 0, properties length 0. */ + static const byte subscribe_x[] = { + 0x82, 0x07, /* Fixed header, remaining length 7. */ + 0x00, 0x01, /* Packet Identifier 1. */ + 0x00, /* Properties length 0 (MQTT 5). */ + 0x00, 0x01, 'x', 0x00 /* Filter "x", QoS 0. */ + }; + /* UNSUBSCRIBE packet_id=2, filter "x", properties length 0. */ + static const byte unsubscribe_x[] = { + 0xA2, 0x06, 0x00, 0x02, 0x00, 0x00, 0x01, 'x' + }; + /* UNSUBSCRIBE packet_id=3, filter "y", properties length 0. */ + static const byte unsubscribe_y[] = { + 0xA2, 0x06, 0x00, 0x03, 0x00, 0x00, 0x01, 'y' + }; + + install_mock_net(&net); + XMEMSET(&broker, 0, sizeof(broker)); + ASSERT_EQ(MQTT_CODE_SUCCESS, MqttBroker_Init(&broker, &net)); + ASSERT_EQ(MQTT_CODE_SUCCESS, MqttBroker_Start(&broker)); + + reset_mock_clients(1); + mock_client_input_append(0, connect_sub, sizeof(connect_sub)); + mock_client_input_append(0, subscribe_x, sizeof(subscribe_x)); + for (i = 0; i < 32; i++) { + MqttBroker_Step(&broker); + } + + /* Discard the CONNECT/SUBACK bytes so the next captured stream contains + * only the UNSUBACK being checked. */ + ASSERT_FALSE(g_clients[0].closed); + g_clients[0].out_len = 0; + + /* Filter "x" will be successfuly unsubscribed. */ + mock_client_input_append(0, unsubscribe_x, sizeof(unsubscribe_x)); + for (i = 0; i < 16; i++) { + MqttBroker_Step(&broker); + } + + /* Confirms reason code is RMQTT_REASON_SUCCESS. */ + ASSERT_TRUE(g_clients[0].out_len > 0); + XMEMSET(&ack, 0, sizeof(ack)); + ack.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; + rc = MqttDecode_UnsubscribeAck(g_clients[0].out_buf, + (int)g_clients[0].out_len, &ack); + ASSERT_TRUE(rc > 0); + ASSERT_EQ(2, ack.packet_id); + ASSERT_EQ(1, ack.reason_code_count); + ASSERT_EQ(MQTT_REASON_SUCCESS, ack.reason_codes[0]); + + /* Filter "y" was never subscribed. + * Reason code should be MQTT_REASON_NO_SUB_EXIST. */ + g_clients[0].out_len = 0; + mock_client_input_append(0, unsubscribe_y, sizeof(unsubscribe_y)); + for (i = 0; i < 16; i++) { + MqttBroker_Step(&broker); + } + XMEMSET(&ack, 0, sizeof(ack)); + ack.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; + rc = MqttDecode_UnsubscribeAck(g_clients[0].out_buf, + (int)g_clients[0].out_len, &ack); + ASSERT_TRUE(rc > 0); + ASSERT_EQ(3, ack.packet_id); + ASSERT_EQ(1, ack.reason_code_count); + ASSERT_EQ(MQTT_REASON_NO_SUB_EXIST, ack.reason_codes[0]); + + MqttBroker_Stop(&broker); + MqttBroker_Free(&broker); +} #endif /* WOLFMQTT_V5 */ /* -------------------------------------------------------------------------- */ @@ -9726,6 +9809,7 @@ int main(int argc, char** argv) RUN_TEST(connect_v5_emptyid_assigned_id_emitted); RUN_TEST(connect_v5_emptyid_assigned_id_survives_prop_pool_pressure); RUN_TEST(connect_v5_emptyid_clean0_accepted); + RUN_TEST(unsubscribe_v5_reason_codes); #endif #ifndef WOLFMQTT_STATIC_MEMORY RUN_TEST(fanout_subs_generation_bumped_on_unsubscribe); From 1bb853ccac6727f5db445a2f5f389061c97fc1c4 Mon Sep 17 00:00:00 2001 From: kaage Date: Tue, 15 Sep 2026 13:03:28 +0900 Subject: [PATCH 3/5] Reflected Copilot's code review --- src/mqtt_broker.c | 16 ++++++++++------ tests/test_broker_connect.c | 4 ++-- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/mqtt_broker.c b/src/mqtt_broker.c index cc65fbc93..13cfbae16 100644 --- a/src/mqtt_broker.c +++ b/src/mqtt_broker.c @@ -4408,7 +4408,7 @@ static int BrokerSubs_Add(MqttBroker* broker, BrokerClient* bc, return rc; } -static bool BrokerSubs_Remove(MqttBroker* broker, BrokerClient* bc, +static int BrokerSubs_Remove(MqttBroker* broker, BrokerClient* bc, const char* filter, word16 filter_len) { #ifdef WOLFMQTT_STATIC_MEMORY @@ -4431,7 +4431,7 @@ static bool BrokerSubs_Remove(MqttBroker* broker, BrokerClient* bc, if (bc->sub_count > 0) { bc->sub_count--; } - return true; + return 1; } } #else @@ -4460,13 +4460,13 @@ static bool BrokerSubs_Remove(MqttBroker* broker, BrokerClient* bc, if (bc->sub_count > 0) { bc->sub_count--; } - return true; + return 1; } prev = cur; cur = next; } #endif - return false; + return 0; } /* -------------------------------------------------------------------------- */ @@ -7545,13 +7545,17 @@ static int BrokerHandle_Unsubscribe(BrokerClient* bc, int rx_len, for (i = 0; i < unsub.topic_count && i < MAX_MQTT_TOPICS; i++) { const char* f = unsub.topics[i].topic_filter; word16 flen = 0; - bool removed = false; +#ifdef WOLFMQTT_V5 + int removed = 0; +#endif if (f && MqttDecode_Num((byte*)f - MQTT_DATA_LEN_SIZE, &flen, MQTT_DATA_LEN_SIZE) == MQTT_DATA_LEN_SIZE) { - removed = BrokerSubs_Remove(broker, bc, f, flen); #ifdef WOLFMQTT_V5 + removed = BrokerSubs_Remove(broker, bc, f, flen); if (removed) reasons[i] = MQTT_REASON_SUCCESS; else reasons[i] = MQTT_REASON_NO_SUB_EXIST; +#else + BrokerSubs_Remove(broker, bc, f, flen); #endif } #ifdef WOLFMQTT_V5 diff --git a/tests/test_broker_connect.c b/tests/test_broker_connect.c index 6b78b27b5..e8b3a5ca3 100644 --- a/tests/test_broker_connect.c +++ b/tests/test_broker_connect.c @@ -1561,13 +1561,13 @@ TEST(unsubscribe_v5_reason_codes) ASSERT_FALSE(g_clients[0].closed); g_clients[0].out_len = 0; - /* Filter "x" will be successfuly unsubscribed. */ + /* Filter "x" will be successfully unsubscribed. */ mock_client_input_append(0, unsubscribe_x, sizeof(unsubscribe_x)); for (i = 0; i < 16; i++) { MqttBroker_Step(&broker); } - /* Confirms reason code is RMQTT_REASON_SUCCESS. */ + /* Confirms reason code is MQTT_REASON_SUCCESS. */ ASSERT_TRUE(g_clients[0].out_len > 0); XMEMSET(&ack, 0, sizeof(ack)); ack.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; From 3912f344d30f64332a515b6f9232e7a9bd4735f2 Mon Sep 17 00:00:00 2001 From: kaage Date: Fri, 18 Sep 2026 07:52:44 +0900 Subject: [PATCH 4/5] Reflected Takeda-san's code review --- src/mqtt_broker.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/mqtt_broker.c b/src/mqtt_broker.c index 13cfbae16..a04fda5ed 100644 --- a/src/mqtt_broker.c +++ b/src/mqtt_broker.c @@ -7546,21 +7546,21 @@ static int BrokerHandle_Unsubscribe(BrokerClient* bc, int rx_len, const char* f = unsub.topics[i].topic_filter; word16 flen = 0; #ifdef WOLFMQTT_V5 - int removed = 0; + reasons[i] = MQTT_REASON_TOPIC_FILTER_INVALID; #endif if (f && MqttDecode_Num((byte*)f - MQTT_DATA_LEN_SIZE, &flen, MQTT_DATA_LEN_SIZE) == MQTT_DATA_LEN_SIZE) { #ifdef WOLFMQTT_V5 - removed = BrokerSubs_Remove(broker, bc, f, flen); - if (removed) reasons[i] = MQTT_REASON_SUCCESS; - else reasons[i] = MQTT_REASON_NO_SUB_EXIST; + if (BrokerSubs_Remove(broker, bc, f, flen)) { + reasons[i] = MQTT_REASON_SUCCESS; + } + else { + reasons[i] = MQTT_REASON_NO_SUB_EXIST; + } #else BrokerSubs_Remove(broker, bc, f, flen); #endif } -#ifdef WOLFMQTT_V5 - else reasons[i] = MQTT_REASON_TOPIC_FILTER_INVALID; -#endif } XMEMSET(&ack, 0, sizeof(ack)); From d4185c71619a60af69f37a0f7c0c16f65adda0a1 Mon Sep 17 00:00:00 2001 From: kaage Date: Tue, 22 Sep 2026 13:08:08 +0900 Subject: [PATCH 5/5] Reflected skoll's code review --- src/mqtt_broker.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/mqtt_broker.c b/src/mqtt_broker.c index a04fda5ed..0c3651ae1 100644 --- a/src/mqtt_broker.c +++ b/src/mqtt_broker.c @@ -4408,6 +4408,10 @@ static int BrokerSubs_Add(MqttBroker* broker, BrokerClient* bc, return rc; } +/* 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) { @@ -7546,7 +7550,7 @@ static int BrokerHandle_Unsubscribe(BrokerClient* bc, int rx_len, const char* f = unsub.topics[i].topic_filter; word16 flen = 0; #ifdef WOLFMQTT_V5 - reasons[i] = MQTT_REASON_TOPIC_FILTER_INVALID; + reasons[i] = MQTT_REASON_UNSPECIFIED_ERR; #endif if (f && MqttDecode_Num((byte*)f - MQTT_DATA_LEN_SIZE, &flen, MQTT_DATA_LEN_SIZE) == MQTT_DATA_LEN_SIZE) {