From 3c58ac9308ccbb6af5248c2c9291f33f4dac3c1d Mon Sep 17 00:00:00 2001 From: "Roger A. Light" Date: Tue, 19 Jan 2021 11:51:00 +0000 Subject: [PATCH] Fix `message_size_limit` not applying to the Will payload. Closes #2022. Thanks to Umberto Morelli. --- ChangeLog.txt | 1 + src/conf.c | 1 + src/handle_connect.c | 15 +++- src/handle_publish.c | 2 +- src/loop.c | 5 +- .../broker/02-subpub-qos0-oversize-payload.py | 75 +++++++++++++++++ .../broker/02-subpub-qos1-oversize-payload.py | 81 +++++++++++++++++++ test/broker/07-will-oversize-payload.py | 71 ++++++++++++++++ test/broker/Makefile | 3 + test/broker/test.py | 3 + test/mosq_test.py | 24 ++++-- 11 files changed, 271 insertions(+), 10 deletions(-) create mode 100755 test/broker/02-subpub-qos0-oversize-payload.py create mode 100755 test/broker/02-subpub-qos1-oversize-payload.py create mode 100755 test/broker/07-will-oversize-payload.py diff --git a/ChangeLog.txt b/ChangeLog.txt index 02ceae9a..e725203d 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -9,6 +9,7 @@ Broker: - Improve logging in obscure cases when a client disconnects. Closes #2017. - Fix reloading of listeners where multiple listeners have been defined with the same port but different bind addresses. Closes #2029. +- Fix `message_size_limit` not applying to the Will payload. Closes #2022. Apps: - Allow command line arguments to override config file options in diff --git a/src/conf.c b/src/conf.c index 9c9de4f4..ba65fc00 100644 --- a/src/conf.c +++ b/src/conf.c @@ -1696,6 +1696,7 @@ int config__read_file_core(struct mosquitto__config *config, bool reload, struct } memory__set_limit((size_t)lim); }else if(!strcmp(token, "message_size_limit")){ + log__printf(NULL, MOSQ_LOG_NOTICE, "Note: It is recommended to replace `message_size_limit` with `max_packet_size`."); if(conf__parse_int(&token, "message_size_limit", (int *)&config->message_size_limit, saveptr)) return MOSQ_ERR_INVAL; if(config->message_size_limit > MQTT_MAX_PAYLOAD){ log__printf(NULL, MOSQ_LOG_ERR, "Error: Invalid message_size_limit value (%u).", config->message_size_limit); diff --git a/src/handle_connect.c b/src/handle_connect.c index 5a33fdde..fc6ebbd8 100644 --- a/src/handle_connect.c +++ b/src/handle_connect.c @@ -291,7 +291,7 @@ error: } -static int will__read(struct mosquitto *context, struct mosquitto_message_all **will, uint8_t will_qos, int will_retain) +static int will__read(struct mosquitto *context, const char *client_id, struct mosquitto_message_all **will, uint8_t will_qos, int will_retain) { int rc = MOSQ_ERR_SUCCESS; size_t slen; @@ -344,6 +344,17 @@ static int will__read(struct mosquitto *context, struct mosquitto_message_all ** will_struct->msg.payloadlen = payloadlen; if(will_struct->msg.payloadlen > 0){ + if(db.config->message_size_limit && will_struct->msg.payloadlen > db.config->message_size_limit){ + log__printf(NULL, MOSQ_LOG_DEBUG, "Client %s connected with too large Will payload", client_id); + if(context->protocol == mosq_p_mqtt5){ + send__connack(context, 0, MQTT_RC_PACKET_TOO_LARGE, NULL); + }else{ + send__connack(context, 0, CONNACK_REFUSED_NOT_AUTHORIZED, NULL); + } + context__disconnect(context); + rc = MOSQ_ERR_PAYLOAD_SIZE; + goto error_cleanup; + } will_struct->msg.payload = mosquitto__malloc((size_t)will_struct->msg.payloadlen); if(!will_struct->msg.payload){ rc = MOSQ_ERR_NOMEM; @@ -602,7 +613,7 @@ int handle__connect(struct mosquitto *context) } if(will){ - rc = will__read(context, &will_struct, will_qos, will_retain); + rc = will__read(context, client_id, &will_struct, will_qos, will_retain); if(rc) goto handle_connect_error; }else{ if(context->protocol == mosq_p_mqtt311 || context->protocol == mosq_p_mqtt5){ diff --git a/src/handle_publish.c b/src/handle_publish.c index c5843832..3f9a1627 100644 --- a/src/handle_publish.c +++ b/src/handle_publish.c @@ -224,7 +224,7 @@ int handle__publish(struct mosquitto *context) if(msg->payloadlen){ if(db.config->message_size_limit && msg->payloadlen > db.config->message_size_limit){ log__printf(NULL, MOSQ_LOG_DEBUG, "Dropped too large PUBLISH from %s (d%d, q%d, r%d, m%d, '%s', ... (%ld bytes))", context->id, dup, msg->qos, msg->retain, msg->source_mid, msg->topic, (long)msg->payloadlen); - reason_code = MQTT_RC_IMPLEMENTATION_SPECIFIC; + reason_code = MQTT_RC_PACKET_TOO_LARGE; goto process_bad_message; } msg->payload = mosquitto__malloc(msg->payloadlen+1); diff --git a/src/loop.c b/src/loop.c index ba146c25..ec8b80f8 100644 --- a/src/loop.c +++ b/src/loop.c @@ -344,6 +344,9 @@ void do_disconnect(struct mosquitto *context, int reason) case MOSQ_ERR_OVERSIZE_PACKET: log__printf(NULL, MOSQ_LOG_NOTICE, "Client %s disconnected due to oversize packet.", id); break; + case MOSQ_ERR_PAYLOAD_SIZE: + log__printf(NULL, MOSQ_LOG_NOTICE, "Client %s disconnected due to oversize payload.", id); + break; case MOSQ_ERR_NOMEM: log__printf(NULL, MOSQ_LOG_NOTICE, "Client %s disconnected due to out of memory.", id); break; @@ -357,7 +360,7 @@ void do_disconnect(struct mosquitto *context, int reason) log__printf(NULL, MOSQ_LOG_NOTICE, "Client %s disconnected: %s.", id, strerror(errno)); break; default: - log__printf(NULL, MOSQ_LOG_NOTICE, "Bad socket read/write on client %s: %s.", id, mosquitto_strerror(reason)); + log__printf(NULL, MOSQ_LOG_NOTICE, "Bad socket read/write on client %s: %s", id, mosquitto_strerror(reason)); break; } }else{ diff --git a/test/broker/02-subpub-qos0-oversize-payload.py b/test/broker/02-subpub-qos0-oversize-payload.py new file mode 100755 index 00000000..4e78dacf --- /dev/null +++ b/test/broker/02-subpub-qos0-oversize-payload.py @@ -0,0 +1,75 @@ +#!/usr/bin/env python3 + +# Test whether message size limits apply. + +from mosq_test_helper import * + +def write_config(filename, port): + with open(filename, 'w') as f: + f.write("listener %d\n" % (port)) + f.write("allow_anonymous true\n") + f.write("message_size_limit 1\n") + +def do_test(proto_ver): + rc = 1 + mid = 53 + keepalive = 60 + connect_packet = mosq_test.gen_connect("subpub-qos0-test", keepalive=keepalive, proto_ver=proto_ver) + connack_packet = mosq_test.gen_connack(rc=0, proto_ver=proto_ver) + + subscribe_packet = mosq_test.gen_subscribe(mid, "subpub/qos0", 0, proto_ver=proto_ver) + suback_packet = mosq_test.gen_suback(mid, 0, proto_ver=proto_ver) + + connect2_packet = mosq_test.gen_connect("subpub-qos0-helper", keepalive=keepalive, proto_ver=proto_ver) + connack2_packet = mosq_test.gen_connack(rc=0, proto_ver=proto_ver) + + publish_packet_ok = mosq_test.gen_publish("subpub/qos0", qos=0, payload="A", proto_ver=proto_ver) + publish_packet_bad = mosq_test.gen_publish("subpub/qos0", qos=0, payload="AB", proto_ver=proto_ver) + + port = mosq_test.get_port() + conf_file = os.path.basename(__file__).replace('.py', '.conf') + write_config(conf_file, port) + + broker = mosq_test.start_broker(filename=os.path.basename(__file__), use_conf=True, port=port) + + try: + sock = mosq_test.do_client_connect(connect_packet, connack_packet, timeout=20, port=port) + mosq_test.do_send_receive(sock, subscribe_packet, suback_packet, "suback") + + sock2 = mosq_test.do_client_connect(connect2_packet, connack2_packet, timeout=20, port=port) + sock2.send(publish_packet_ok) + mosq_test.expect_packet(sock, "publish 1", publish_packet_ok) + + # Check all is still well on the publishing client + mosq_test.do_ping(sock2) + + sock2.send(publish_packet_bad) + + # Check all is still well on the publishing client + mosq_test.do_ping(sock2) + + # The subscribing client shouldn't have received a PUBLISH + mosq_test.do_ping(sock) + rc = 0 + + sock.close() + except SyntaxError: + raise + except TypeError: + raise + except mosq_test.TestError: + pass + finally: + os.remove(conf_file) + broker.terminate() + broker.wait() + (stdo, stde) = broker.communicate() + if rc: + print(stde.decode('utf-8')) + print("proto_ver=%d" % (proto_ver)) + exit(rc) + + +do_test(proto_ver=4) +do_test(proto_ver=5) +exit(0) diff --git a/test/broker/02-subpub-qos1-oversize-payload.py b/test/broker/02-subpub-qos1-oversize-payload.py new file mode 100755 index 00000000..17878004 --- /dev/null +++ b/test/broker/02-subpub-qos1-oversize-payload.py @@ -0,0 +1,81 @@ +#!/usr/bin/env python3 + +# Test whether message size limits apply. + +from mosq_test_helper import * + +def write_config(filename, port): + with open(filename, 'w') as f: + f.write("listener %d\n" % (port)) + f.write("allow_anonymous true\n") + f.write("message_size_limit 1\n") + +def do_test(proto_ver): + rc = 1 + mid = 53 + keepalive = 60 + connect_packet = mosq_test.gen_connect("subpub-qos1-test", keepalive=keepalive, proto_ver=proto_ver) + connack_packet = mosq_test.gen_connack(rc=0, proto_ver=proto_ver) + + subscribe_packet = mosq_test.gen_subscribe(mid, "subpub/qos1", 1, proto_ver=proto_ver) + suback_packet = mosq_test.gen_suback(mid, 1, proto_ver=proto_ver) + + connect2_packet = mosq_test.gen_connect("subpub-qos1-helper", keepalive=keepalive, proto_ver=proto_ver) + connack2_packet = mosq_test.gen_connack(rc=0, proto_ver=proto_ver) + + mid = 1 + publish_packet_ok = mosq_test.gen_publish("subpub/qos1", mid=mid, qos=1, payload="A", proto_ver=proto_ver) + puback_packet_ok = mosq_test.gen_puback(mid=mid, proto_ver=proto_ver) + + mid = 2 + publish_packet_bad = mosq_test.gen_publish("subpub/qos1", mid=mid, qos=1, payload="AB", proto_ver=proto_ver) + if proto_ver == 5: + puback_packet_bad = mosq_test.gen_puback(reason_code=mqtt5_rc.MQTT_RC_PACKET_TOO_LARGE, mid=mid, proto_ver=proto_ver) + else: + puback_packet_bad = mosq_test.gen_puback(mid=mid, proto_ver=proto_ver) + + port = mosq_test.get_port() + conf_file = os.path.basename(__file__).replace('.py', '.conf') + write_config(conf_file, port) + + broker = mosq_test.start_broker(filename=os.path.basename(__file__), use_conf=True, port=port) + + try: + sock = mosq_test.do_client_connect(connect_packet, connack_packet, timeout=20, port=port) + mosq_test.do_send_receive(sock, subscribe_packet, suback_packet, "suback") + + sock2 = mosq_test.do_client_connect(connect2_packet, connack2_packet, timeout=20, port=port) + mosq_test.do_send_receive(sock2, publish_packet_ok, puback_packet_ok, "puback 1") + mosq_test.expect_packet(sock, "publish 1", publish_packet_ok) + sock.send(puback_packet_ok) + + # Check all is still well on the publishing client + mosq_test.do_ping(sock2) + + mosq_test.do_send_receive(sock2, publish_packet_bad, puback_packet_bad, "puback 2") + + # The subscribing client shouldn't have received a PUBLISH + mosq_test.do_ping(sock) + rc = 0 + + sock.close() + except SyntaxError: + raise + except TypeError: + raise + except mosq_test.TestError: + pass + finally: + os.remove(conf_file) + broker.terminate() + broker.wait() + (stdo, stde) = broker.communicate() + if rc: + print(stde.decode('utf-8')) + print("proto_ver=%d" % (proto_ver)) + exit(rc) + + +do_test(proto_ver=4) +do_test(proto_ver=5) +exit(0) diff --git a/test/broker/07-will-oversize-payload.py b/test/broker/07-will-oversize-payload.py new file mode 100755 index 00000000..319ec509 --- /dev/null +++ b/test/broker/07-will-oversize-payload.py @@ -0,0 +1,71 @@ +#!/usr/bin/env python3 + +# Test whether a client will that is too large is handled + +from mosq_test_helper import * + +def write_config(filename, port): + with open(filename, 'w') as f: + f.write("listener %d\n" % (port)) + f.write("allow_anonymous true\n") + f.write("message_size_limit 1\n") + +def do_test(proto_ver, clean_session): + rc = 1 + mid = 53 + keepalive = 60 + connect_packet = mosq_test.gen_connect("will-test", keepalive=keepalive, proto_ver=proto_ver) + connack_packet = mosq_test.gen_connack(rc=0, proto_ver=proto_ver) + + connect_packet_ok = mosq_test.gen_connect("test-helper", keepalive=keepalive, will_topic="will/qos0/test", will_payload=b"A", clean_session=clean_session, proto_ver=proto_ver, session_expiry=60) + connack_packet_ok = mosq_test.gen_connack(rc=0, proto_ver=proto_ver) + + connect_packet_bad = mosq_test.gen_connect("test-helper", keepalive=keepalive, will_topic="will/qos0/test", will_payload=b"AB", clean_session=clean_session, proto_ver=proto_ver, session_expiry=60) + if proto_ver == 5: + connack_packet_bad = mosq_test.gen_connack(rc=mqtt5_rc.MQTT_RC_PACKET_TOO_LARGE, proto_ver=proto_ver, property_helper=False) + else: + connack_packet_bad = mosq_test.gen_connack(rc=5, proto_ver=proto_ver) + + subscribe_packet = mosq_test.gen_subscribe(mid, "will/qos0/test", 0, proto_ver=proto_ver) + suback_packet = mosq_test.gen_suback(mid, 0, proto_ver=proto_ver) + + publish_packet = mosq_test.gen_publish("will/qos0/test", qos=0, payload="A", proto_ver=proto_ver) + + port = mosq_test.get_port() + conf_file = os.path.basename(__file__).replace('.py', '.conf') + write_config(conf_file, port) + + broker = mosq_test.start_broker(filename=os.path.basename(__file__), use_conf=True, port=port) + + try: + sock = mosq_test.do_client_connect(connect_packet, connack_packet, timeout=5, port=port) + mosq_test.do_send_receive(sock, subscribe_packet, suback_packet, "suback") + + sock2 = mosq_test.do_client_connect(connect_packet_bad, connack_packet_bad, port=port, timeout=5) + sock2.close() + + sock2 = mosq_test.do_client_connect(connect_packet_ok, connack_packet_ok, port=port, timeout=5) + sock2.close() + + mosq_test.expect_packet(sock, "publish", publish_packet) + # Check there are no more messages + mosq_test.do_ping(sock) + rc = 0 + + sock.close() + except mosq_test.TestError: + pass + finally: + os.remove(conf_file) + broker.terminate() + broker.wait() + (stdo, stde) = broker.communicate() + if rc: + print(stde.decode('utf-8')) + exit(rc) + +do_test(4, True) +do_test(4, False) +do_test(5, True) +do_test(5, False) +exit(0) diff --git a/test/broker/Makefile b/test/broker/Makefile index c139e1be..fcb6839d 100644 --- a/test/broker/Makefile +++ b/test/broker/Makefile @@ -44,6 +44,7 @@ test : test-compile 01 02 03 04 05 06 07 08 09 10 11 12 13 14 ./02-shared-qos0-v5.py ./02-subhier-crash.py ./02-subpub-qos0-long-topic.py + ./02-subpub-qos0-oversize-payload.py ./02-subpub-qos0-retain-as-publish.py ./02-subpub-qos0-send-retain.py ./02-subpub-qos0-subscription-id.py @@ -56,6 +57,7 @@ test : test-compile 01 02 03 04 05 06 07 08 09 10 11 12 13 14 ./02-subpub-qos1-message-expiry-will.py ./02-subpub-qos1-message-expiry.py ./02-subpub-qos1-nolocal.py + ./02-subpub-qos1-oversize-payload.py ./02-subpub-qos1.py ./02-subpub-qos2-1322.py ./02-subpub-qos2-bad-puback-1.py @@ -141,6 +143,7 @@ test : test-compile 01 02 03 04 05 06 07 08 09 10 11 12 13 14 ./07-will-no-flag.py ./07-will-null-topic.py ./07-will-null.py + ./07-will-oversize-payload.py ./07-will-properties.py ./07-will-qos0.py ./07-will-reconnect-1273.py diff --git a/test/broker/test.py b/test/broker/test.py index 28ecc92d..0514359a 100755 --- a/test/broker/test.py +++ b/test/broker/test.py @@ -27,6 +27,7 @@ tests = [ (1, './02-shared-qos0-v5.py'), (1, './02-subhier-crash.py'), (1, './02-subpub-qos0-long-topic.py'), + (1, './02-subpub-qos0-oversize-payload.py'), (1, './02-subpub-qos0-retain-as-publish.py'), (1, './02-subpub-qos0-send-retain.py'), (1, './02-subpub-qos0-subscription-id.py'), @@ -39,6 +40,7 @@ tests = [ (1, './02-subpub-qos1-message-expiry-will.py'), (1, './02-subpub-qos1-message-expiry.py'), (1, './02-subpub-qos1-nolocal.py'), + (1, './02-subpub-qos1-oversize-payload.py'), (1, './02-subpub-qos1.py'), (1, './02-subpub-qos2-1322.py'), (1, './02-subpub-qos2-bad-puback-1.py'), @@ -120,6 +122,7 @@ tests = [ (1, './07-will-no-flag.py'), (1, './07-will-null-topic.py'), (1, './07-will-null.py'), + (1, './07-will-oversize-payload.py'), (1, './07-will-properties.py'), (1, './07-will-qos0.py'), (1, './07-will-reconnect-1273.py'), diff --git a/test/mosq_test.py b/test/mosq_test.py index 901ece55..52dcca0c 100644 --- a/test/mosq_test.py +++ b/test/mosq_test.py @@ -284,12 +284,20 @@ def to_string(packet): return s elif cmd == 0x40: # PUBACK - (cmd, rl, mid) = struct.unpack('!BBH', packet) - return "PUBACK, rl="+str(rl)+", mid="+str(mid) + if len(packet) == 5: + (cmd, rl, mid, reason_code) = struct.unpack('!BBHB', packet) + return "PUBACK, rl="+str(rl)+", mid="+str(mid)+", reason_code="+str(reason_code) + else: + (cmd, rl, mid) = struct.unpack('!BBH', packet) + return "PUBACK, rl="+str(rl)+", mid="+str(mid) elif cmd == 0x50: # PUBREC - (cmd, rl, mid) = struct.unpack('!BBH', packet) - return "PUBREC, rl="+str(rl)+", mid="+str(mid) + if len(packet) == 5: + (cmd, rl, mid, reason_code) = struct.unpack('!BBHB', packet) + return "PUBREC, rl="+str(rl)+", mid="+str(mid)+", reason_code="+str(reason_code) + else: + (cmd, rl, mid) = struct.unpack('!BBH', packet) + return "PUBREC, rl="+str(rl)+", mid="+str(mid) elif cmd == 0x60: # PUBREL dup = (packet0 & 0x08)>>3 @@ -353,8 +361,12 @@ def to_string(packet): return "PINGRESP, rl="+str(rl) elif cmd == 0xE0: # DISCONNECT - (cmd, rl) = struct.unpack('!BB', packet) - return "DISCONNECT, rl="+str(rl) + if len(packet) == 3: + (cmd, rl, reason_code) = struct.unpack('!BBB', packet) + return "DISCONNECT, rl="+str(rl)+", reason_code="+str(reason_code) + else: + (cmd, rl) = struct.unpack('!BB', packet) + return "DISCONNECT, rl="+str(rl) elif cmd == 0xF0: # AUTH (cmd, rl) = struct.unpack('!BB', packet)