Fix `message_size_limit` not applying to the Will payload.

Closes #2022. Thanks to Umberto Morelli.

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

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);

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){

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);

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{

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)

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)

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)

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

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'),

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)