From 1ec0cea34af996d011d8726286bf6aff544090fb Mon Sep 17 00:00:00 2001 From: "Roger A. Light" Date: Sun, 17 Feb 2019 09:30:06 +0000 Subject: [PATCH] Fix missing reason_code on v5 UNSUBACK. Closes #1167. Thanks to Christoph Krey. --- src/handle_unsubscribe.c | 62 +++++++++++++------ src/mosquitto_broker_internal.h | 2 +- src/send_unsuback.c | 5 +- .../broker/02-unsubscribe-qos2-multiple-v5.py | 34 ++++++++++ test/broker/Makefile | 1 + test/broker/test.py | 1 + test/mosq_test.py | 11 +++- 7 files changed, 91 insertions(+), 25 deletions(-) create mode 100755 test/broker/02-unsubscribe-qos2-multiple-v5.py diff --git a/src/handle_unsubscribe.c b/src/handle_unsubscribe.c index 6fb24e2d..03d4025a 100644 --- a/src/handle_unsubscribe.c +++ b/src/handle_unsubscribe.c @@ -31,6 +31,9 @@ int handle__unsubscribe(struct mosquitto_db *db, struct mosquitto *context) char *sub; int slen; int rc; + int reason_code_count = 0; + int reason_code_max; + uint8_t *reason_codes = NULL, *reason_tmp; mosquitto_property *properties = NULL; if(!context) return MOSQ_ERR_INVAL; @@ -57,32 +60,49 @@ int handle__unsubscribe(struct mosquitto_db *db, struct mosquitto *context) return MOSQ_ERR_PROTOCOL; } } + + reason_code_max = 10; + reason_codes = mosquitto__malloc(reason_code_max); + if(!reason_codes){ + return MOSQ_ERR_NOMEM; + } + while(context->in_packet.pos < context->in_packet.remaining_length){ sub = NULL; if(packet__read_string(&context->in_packet, &sub, &slen)){ return 1; } - if(sub){ - if(!slen){ - log__printf(NULL, MOSQ_LOG_INFO, - "Empty unsubscription string from %s, disconnecting.", - context->id); - mosquitto__free(sub); - return 1; - } - if(mosquitto_sub_topic_check(sub)){ - log__printf(NULL, MOSQ_LOG_INFO, - "Invalid unsubscription string from %s, disconnecting.", - context->id); - mosquitto__free(sub); - return 1; - } - - log__printf(NULL, MOSQ_LOG_DEBUG, "\t%s", sub); - sub__remove(db, context, sub, db->subs); - log__printf(NULL, MOSQ_LOG_UNSUBSCRIBE, "%s %s", context->id, sub); + if(!slen){ + log__printf(NULL, MOSQ_LOG_INFO, + "Empty unsubscription string from %s, disconnecting.", + context->id); mosquitto__free(sub); + return 1; + } + if(mosquitto_sub_topic_check(sub)){ + log__printf(NULL, MOSQ_LOG_INFO, + "Invalid unsubscription string from %s, disconnecting.", + context->id); + mosquitto__free(sub); + return 1; + } + + log__printf(NULL, MOSQ_LOG_DEBUG, "\t%s", sub); + sub__remove(db, context, sub, db->subs); + log__printf(NULL, MOSQ_LOG_UNSUBSCRIBE, "%s %s", context->id, sub); + mosquitto__free(sub); + + reason_codes[reason_code_count] = 0; + reason_code_count++; + if(reason_code_count == reason_code_max){ + reason_tmp = mosquitto__realloc(reason_codes, reason_code_max*2); + if(!reason_tmp){ + mosquitto__free(reason_codes); + return MOSQ_ERR_NOMEM; + } + reason_codes = reason_tmp; + reason_code_max *= 2; } } #ifdef WITH_PERSISTENCE @@ -92,6 +112,8 @@ int handle__unsubscribe(struct mosquitto_db *db, struct mosquitto *context) log__printf(NULL, MOSQ_LOG_DEBUG, "Sending UNSUBACK to %s", context->id); /* We don't use Reason String or User Property yet. */ - return send__unsuback(context, mid, NULL); + rc = send__unsuback(context, mid, reason_code_count, reason_codes, NULL); + mosquitto__free(reason_codes); + return rc; } diff --git a/src/mosquitto_broker_internal.h b/src/mosquitto_broker_internal.h index c9981390..3a2ac517 100644 --- a/src/mosquitto_broker_internal.h +++ b/src/mosquitto_broker_internal.h @@ -525,7 +525,7 @@ int restore_privileges(void); * ============================================================ */ int send__connack(struct mosquitto_db *db, struct mosquitto *context, int ack, int reason_code, const mosquitto_property *properties); int send__suback(struct mosquitto *context, uint16_t mid, uint32_t payloadlen, const void *payload); -int send__unsuback(struct mosquitto *context, uint16_t mid, const mosquitto_property *properties); +int send__unsuback(struct mosquitto *context, uint16_t mid, int reason_code_count, uint8_t *reason_codes, const mosquitto_property *properties); /* ============================================================ * Network functions diff --git a/src/send_unsuback.c b/src/send_unsuback.c index ee0c8333..9a310fac 100644 --- a/src/send_unsuback.c +++ b/src/send_unsuback.c @@ -25,7 +25,7 @@ Contributors: #include "property_mosq.h" -int send__unsuback(struct mosquitto *mosq, uint16_t mid, const mosquitto_property *properties) +int send__unsuback(struct mosquitto *mosq, uint16_t mid, int reason_code_count, uint8_t *reason_codes, const mosquitto_property *properties) { struct mosquitto__packet *packet = NULL; int rc; @@ -41,7 +41,7 @@ int send__unsuback(struct mosquitto *mosq, uint16_t mid, const mosquitto_propert if(mosq->protocol == mosq_p_mqtt5){ proplen = property__get_length_all(properties); varbytes = packet__varint_bytes(proplen); - packet->remaining_length += varbytes + proplen; + packet->remaining_length += varbytes + proplen + reason_code_count; } rc = packet__alloc(packet); @@ -54,6 +54,7 @@ int send__unsuback(struct mosquitto *mosq, uint16_t mid, const mosquitto_propert if(mosq->protocol == mosq_p_mqtt5){ property__write_all(packet, properties, true); + packet__write_bytes(packet, reason_codes, reason_code_count); } return packet__queue(mosq, packet); diff --git a/test/broker/02-unsubscribe-qos2-multiple-v5.py b/test/broker/02-unsubscribe-qos2-multiple-v5.py new file mode 100755 index 00000000..1e6c3f53 --- /dev/null +++ b/test/broker/02-unsubscribe-qos2-multiple-v5.py @@ -0,0 +1,34 @@ +#!/usr/bin/env python + +# Test whether a v5 UNSUBSCRIBE to multiple topics with QoS 2 results in the correct UNSUBACK packet. + +from mosq_test_helper import * + +rc = 1 +mid = 3 +keepalive = 60 +connect_packet = mosq_test.gen_connect("unsubscribe-qos2-test", keepalive=keepalive, proto_ver=5) +connack_packet = mosq_test.gen_connack(rc=0, proto_ver=5) + +unsubscribe_packet = mosq_test.gen_unsubscribe_multiple(mid, ["qos2/one", "qos2/two"], proto_ver=5) +unsuback_packet = mosq_test.gen_unsuback(mid, proto_ver=5, reason_code=[0, 0]) + +port = mosq_test.get_port() +broker = mosq_test.start_broker(filename=os.path.basename(__file__), port=port) + +try: + sock = mosq_test.do_client_connect(connect_packet, connack_packet, port=port) + mosq_test.do_send_receive(sock, unsubscribe_packet, unsuback_packet, "unsuback") + + rc = 0 + + sock.close() +finally: + broker.terminate() + broker.wait() + (stdo, stde) = broker.communicate() + if rc: + print(stde) + +exit(rc) + diff --git a/test/broker/Makefile b/test/broker/Makefile index 28473325..3b5b6fe1 100644 --- a/test/broker/Makefile +++ b/test/broker/Makefile @@ -76,6 +76,7 @@ endif ./02-unsubscribe-qos2-v5.py ./02-unsubscribe-invalid-no-topic.py ./02-unsubscribe-qos2-multiple.py + ./02-unsubscribe-qos2-multiple-v5.py ./02-subscribe-invalid-utf8.py ./02-subscribe-persistence-flipflop.py ./02-subhier-crash.py diff --git a/test/broker/test.py b/test/broker/test.py index 200cf55b..526f5860 100755 --- a/test/broker/test.py +++ b/test/broker/test.py @@ -57,6 +57,7 @@ tests = [ (1, './02-unsubscribe-qos2-v5.py'), (1, './02-unsubscribe-invalid-no-topic.py'), (1, './02-unsubscribe-qos2-multiple.py'), + (1, './02-unsubscribe-qos2-multiple-v5.py'), (1, './02-subscribe-invalid-utf8.py'), (1, './02-subscribe-persistence-flipflop.py'), (1, './02-subhier-crash.py'), diff --git a/test/mosq_test.py b/test/mosq_test.py index e9da563f..bb5e9147 100644 --- a/test/mosq_test.py +++ b/test/mosq_test.py @@ -498,9 +498,16 @@ def gen_unsubscribe_multiple(mid, topics, proto_ver=4): return struct.pack("!BBH", 162, remaining_length, mid) + packet -def gen_unsuback(mid, proto_ver=4): +def gen_unsuback(mid, proto_ver=4, reason_code=0): if proto_ver == 5: - return struct.pack('!BBHB', 176, 3, mid, 0) + if isinstance(reason_code, list): + reason_code_count = len(reason_code) + p = struct.pack('!BBHB', 176, 3+reason_code_count, mid, 0) + for r in reason_code: + p += struct.pack('B', r) + return p + else: + return struct.pack('!BBHBB', 176, 4, mid, 0, reason_code) else: return struct.pack('!BBH', 176, 2, mid)