diff --git a/BROKER.md b/BROKER.md index c35cd15..1b4312e 100644 --- a/BROKER.md +++ b/BROKER.md @@ -111,6 +111,17 @@ When built with `WOLFMQTT_STATIC_MEMORY`, the broker uses fixed-size arrays inst | `BROKER_TIMEOUT_MS` | 1000 | `select()` timeout | | `BROKER_LISTEN_BACKLOG` | 128 | Listen queue depth | +Retaining a message is best-effort. A `RETAIN=1` PUBLISH whose retained copy +cannot be stored (the retained table is full at `BROKER_MAX_RETAINED`, the +payload exceeds `BROKER_MAX_PAYLOAD_LEN`, or an allocation fails) is still +delivered to every current subscriber and acknowledged with success at any QoS +or protocol level. Only the retained copy is skipped, so a later subscriber will +not receive that message until the topic is published again with room to store +it. The skipped retained copy is logged broker-side. This matches how servers +such as Mosquitto treat a full retained store: live delivery is never sacrificed +to retention. Raise `BROKER_MAX_RETAINED` (or `BROKER_MAX_PAYLOAD_LEN`) if +retained-topic capacity matters for your deployment. + The static offline queue is broker-owned fixed storage. Its dominant RAM cost is approximately `sessions * messages * (topic length + data length)` bytes, plus queue metadata. A new persistent CONNECT is refused when all session slots diff --git a/ChangeLog.md b/ChangeLog.md index f1b1c6a..248c6f5 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -29,6 +29,20 @@ previous manual keep-alive loop under `WOLFMQTT_NO_TIME` (#501) * API / Behavior Changes + - The broker now treats retaining a message as best-effort. When a + `RETAIN=1` PUBLISH cannot be stored (retained table full, oversized + payload, or allocation failure), the message is still delivered to all + current subscribers and acknowledged with success at every QoS and + protocol level; only the retained copy is skipped, and the skip is logged. + This matches Mosquitto and replaces the previous inconsistent handling + that could drop delivery on a retained-store failure. See `BROKER.md`. + - The client rejects an inbound v5 PUBLISH that carries a Topic Alias and no + longer advertises a nonzero Topic Alias Maximum in `CONNECT`. Inbound + alias resolution is not implemented, so the client advertises Topic Alias + Maximum 0 and a server that sends an alias anyway is treated as a protocol + error. Outbound Topic Alias (client to server) is unchanged. An + application that supplies a nonzero `MQTT_PROP_TOPIC_ALIAS_MAX` now gets + `MQTT_CODE_ERROR_PROPERTY` from `MqttClient_Connect`. - A v5 `CONNECT` now advertises `Receive Maximum` set to `MQTT_MAX_RECV_QOS2` (16 by default) unless the application supplied its own `MQTT_PROP_RECEIVE_MAX`. This bounds the QoS 1 and QoS 2 PUBLISH diff --git a/README.md b/README.md index 055ad64..77d0fec 100644 --- a/README.md +++ b/README.md @@ -276,7 +276,7 @@ The following v5.0 specification features are supported by the wolfMQTT client: * Maximum packet size * Server assigned client identifier * Subscription ID -* Topic Alias +* Topic Alias (outbound PUBLISH only; inbound aliases are rejected because the client advertises a Topic Alias Maximum of 0) The v5 enabled wolfMQTT client was tested with the following MQTT v5 brokers: * Mosquitto diff --git a/examples/mqttclient/mqttclient.c b/examples/mqttclient/mqttclient.c index 0037d82..fde90c0 100644 --- a/examples/mqttclient/mqttclient.c +++ b/examples/mqttclient/mqttclient.c @@ -361,10 +361,11 @@ int mqttclient_test(MQTTCtx *mqttCtx) prop->data_int = (word32)mqttCtx->max_packet_size; } { - /* Topic Alias Maximum */ + /* Topic Alias Maximum. Advertise 0: the client does not resolve inbound + * Topic Aliases, so a conforming server must not send any. */ MqttProp* prop = MqttClient_PropsAdd(&mqttCtx->connect.props); prop->type = MQTT_PROP_TOPIC_ALIAS_MAX; - prop->data_short = mqttCtx->topic_alias_max; + prop->data_short = 0; } if (mqttCtx->clean_session == 0) { /* Session expiry interval */ diff --git a/examples/pub-sub/mqtt-pub.c b/examples/pub-sub/mqtt-pub.c index abf3130..86f730e 100644 --- a/examples/pub-sub/mqtt-pub.c +++ b/examples/pub-sub/mqtt-pub.c @@ -297,10 +297,11 @@ int pub_client(MQTTCtx *mqttCtx) prop->data_int = (word32)mqttCtx->max_packet_size; } { - /* Topic Alias Maximum */ + /* Topic Alias Maximum. Advertise 0: the client does not resolve inbound + * Topic Aliases, so a conforming server must not send any. */ MqttProp* prop = MqttClient_PropsAdd(&mqttCtx->connect.props); prop->type = MQTT_PROP_TOPIC_ALIAS_MAX; - prop->data_short = mqttCtx->topic_alias_max; + prop->data_short = 0; } if (mqttCtx->clean_session == 0) { /* Session expiry interval */ diff --git a/examples/pub-sub/mqtt-sub.c b/examples/pub-sub/mqtt-sub.c index c4e7497..a8109c3 100644 --- a/examples/pub-sub/mqtt-sub.c +++ b/examples/pub-sub/mqtt-sub.c @@ -364,7 +364,7 @@ int sub_client(MQTTCtx *mqttCtx) /* Topic Alias Maximum */ MqttProp* prop = MqttClient_PropsAdd(&mqttCtx->connect.props); prop->type = MQTT_PROP_TOPIC_ALIAS_MAX; - prop->data_short = mqttCtx->topic_alias_max; + prop->data_short = 0; } if (mqttCtx->clean_session == 0) { /* Session expiry interval */ diff --git a/src/mqtt_broker.c b/src/mqtt_broker.c index ae9c6d1..b065993 100644 --- a/src/mqtt_broker.c +++ b/src/mqtt_broker.c @@ -7114,6 +7114,19 @@ static int BrokerHandle_Connect(BrokerClient* bc, int rx_len, ack.return_code = MQTT_CONNECT_ACK_CODE_ACCEPTED; #ifdef WOLFMQTT_V5 ack.props = NULL; + + /* Release the decoded CONNECT and Will properties before building the + * CONNACK: they share the fixed property pool, and a CONNECT carrying many + * User Properties would otherwise exhaust it and drop the mandatory + * Assigned Client Identifier added below. */ + if (mc.props != NULL) { + (void)MqttProps_Free(mc.props); + mc.props = NULL; + } + if (lwt.props != NULL) { + (void)MqttProps_Free(lwt.props); + lwt.props = NULL; + } #endif #ifdef WOLFMQTT_V5 @@ -7132,6 +7145,17 @@ static int BrokerHandle_Connect(BrokerClient* bc, int rx_len, prop->data_str.str = bc->client_id; prop->data_str.len = (word16)XSTRLEN(bc->client_id); } + else { + /* [MQTT-3.1.3-6] An empty-ClientId client must be told its + * assigned id; refuse rather than accept an unusable connection + * when the property pool cannot hold it. Clear the effective + * session expiry first so the refused connection tears its + * tentative session down instead of orphaning an unreachable + * one that could evict a valid session at capacity. */ + bc->session_expiry_sec = 0; + ack.return_code = MQTT_REASON_SERVER_UNAVAILABLE; + goto send_connack; + } } /* Advertise feature availability */ @@ -7576,9 +7600,6 @@ static int BrokerHandle_Publish(BrokerClient* bc, int rx_len, byte* payload = NULL; char* topic = NULL; MqttQoS eff_qos; -#if defined(WOLFMQTT_V5) && defined(WOLFMQTT_BROKER_RETAINED) - int retain_rc = MQTT_CODE_SUCCESS; -#endif #if WOLFMQTT_MAX_QOS >= 2 int qos2_duplicate = 0; #endif @@ -7777,19 +7798,20 @@ static int BrokerHandle_Publish(BrokerClient* bc, int rx_len, int ret_rc = BrokerRetained_Store(broker, topic, payload, pub.total_len, pub.qos, expiry); if (ret_rc != MQTT_CODE_SUCCESS) { + /* Retaining is best-effort: a store failure (table full, + * oversized payload, transient alloc) does not reject the + * PUBLISH. The message is still delivered live and + * acknowledged; only the retained copy is not kept. */ WBLOG_ERR(broker, "Retained store failed: %s", MqttClient_ReturnCodeToString(ret_rc)); } -#ifdef WOLFMQTT_V5 - retain_rc = ret_rc; -#endif } } } #endif /* WOLFMQTT_BROKER_RETAINED */ - /* Fan-out is skipped for QoS 2 duplicates: subscribers already received - * the application message from the original PUBLISH ([MQTT-4.3.3]). */ + /* Fan-out is skipped for QoS 2 duplicates: subscribers already received the + * application message from the original PUBLISH ([MQTT-4.3.3]). */ if ( #if WOLFMQTT_MAX_QOS >= 2 !qos2_duplicate && @@ -8105,13 +8127,6 @@ static int BrokerHandle_Publish(BrokerClient* bc, int rx_len, #ifdef WOLFMQTT_V5 resp.protocol_level = bc->protocol_level; resp.reason_code = MQTT_REASON_SUCCESS; - /* A retained-store failure must not be ACKed as success: tell the - * publisher the quota was exceeded [MQTT-3.4.2]. */ -#ifdef WOLFMQTT_BROKER_RETAINED - if (retain_rc != MQTT_CODE_SUCCESS) { - resp.reason_code = MQTT_REASON_QUOTA_EXCEEDED; - } -#endif resp.props = NULL; #endif rc = MqttEncode_PublishResp(bc->tx_buf, BROKER_CLIENT_TX_SZ(bc), diff --git a/src/mqtt_client.c b/src/mqtt_client.c index 1f92715..1ce10bb 100644 --- a/src/mqtt_client.c +++ b/src/mqtt_client.c @@ -1404,7 +1404,24 @@ static int MqttClient_DecodePacket(MqttClient* client, byte* rx_buf, if (rc >= 0) { packet_id = p_publish->packet_id; #ifdef WOLFMQTT_V5 - if (doProps) { + if (client->protocol_level >= MQTT_CONNECT_PROTOCOL_LEVEL_5) { + MqttProp* prop; + for (prop = p_publish->props; prop != NULL; + prop = prop->next) { + /* The client advertises Topic Alias Maximum 0 and keeps + * no inbound alias table, so it can neither resolve nor + * record an alias; reject rather than deliver an + * unresolved topic. */ + if (prop->type == MQTT_PROP_TOPIC_ALIAS) { + MqttProps_Free(p_publish->props); + p_publish->props = NULL; + rc = MQTT_TRACE_ERROR( + MQTT_CODE_ERROR_MALFORMED_DATA); + break; + } + } + } + if (rc >= 0 && doProps) { /* Retain returned properties until the message callback. */ int tmp = Handle_Props(client, p_publish->props, (packet_obj != NULL), @@ -3099,12 +3116,37 @@ int MqttClient_Connect(MqttClient *client, MqttConnect *mc_connect) MqttProp* app_props = NULL; int recv_max_added = 0; #endif +#ifdef WOLFMQTT_V5 + MqttProp* ta_prop; + int ta_count; +#endif /* Validate required arguments */ if (client == NULL || mc_connect == NULL) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } +#ifdef WOLFMQTT_V5 + /* The client does not resolve inbound Topic Aliases, so it must not + * advertise the capability. Reject a nonzero caller-supplied Topic Alias + * Maximum before sending CONNECT rather than advertising support it cannot + * honor and then rejecting the server's aliased PUBLISH. The scan is + * bounded like MqttEncode_Props so a cyclic list cannot spin. */ + if (client->protocol_level >= MQTT_CONNECT_PROTOCOL_LEVEL_5) { + ta_count = 0; + for (ta_prop = mc_connect->props; ta_prop != NULL; + ta_prop = ta_prop->next) { + if (++ta_count > MQTT_MAX_PROPS) { + break; + } + if (ta_prop->type == MQTT_PROP_TOPIC_ALIAS_MAX && + ta_prop->data_short != 0) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PROPERTY); + } + } + } +#endif + #ifndef WOLFMQTT_NO_SESSION_REPLAY if (mc_connect->stat.write == MQTT_MSG_PAYLOAD) { /* MQTT_MSG_PAYLOAD is not part of the CONNECT write sequence diff --git a/src/mqtt_packet.c b/src/mqtt_packet.c index 91af980..d3edade 100644 --- a/src/mqtt_packet.c +++ b/src/mqtt_packet.c @@ -127,10 +127,6 @@ static const struct MqttPropMatrix gPropMatrix[] = { { MQTT_PROP_TYPE_MAX, MQTT_DATA_TYPE_NONE, 0 } }; -/* Maximum number of active properties - overridable */ -#ifndef MQTT_MAX_PROPS -#define MQTT_MAX_PROPS 30 -#endif /* WOLFMQTT_DYN_PROP allows property allocation using malloc */ #ifndef WOLFMQTT_DYN_PROP @@ -916,6 +912,12 @@ int MqttEncode_Props(MqttPacketType packet, MqttProp* props, byte* buf) { case MQTT_DATA_TYPE_BYTE: { + /* Every MQTT 5 Byte property is Boolean-valued (0 or 1); + * Maximum QoS shares the same {0,1} domain. Reject any other + * value so the encoder never emits a Protocol Error. */ + if (cur_prop->data_byte > 1) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PROPERTY); + } if (buf != NULL) { *(buf++) = cur_prop->data_byte; } @@ -1165,11 +1167,11 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, tmp++; total++; prop_len--; - /* [MQTT-3.1.2-28/29] Request Response/Problem Information - * MUST be 0 or 1; any other value is a Protocol Error. */ - if ((cur_prop->type == MQTT_PROP_REQ_RESP_INFO || - cur_prop->type == MQTT_PROP_REQ_PROB_INFO) && - cur_prop->data_byte > 1) { + /* Every MQTT 5 Byte property is Boolean-valued (0 or 1), and + * Maximum QoS shares the same {0,1} domain; any other value is + * a Protocol Error. Mirrors MqttEncode_Props so a decoded + * property always re-encodes. */ + if (cur_prop->data_byte > 1) { rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PROPERTY); } break; diff --git a/tests/test_broker_connect.c b/tests/test_broker_connect.c index d3a48e9..1c06c3b 100644 --- a/tests/test_broker_connect.c +++ b/tests/test_broker_connect.c @@ -1400,6 +1400,87 @@ TEST(connect_v5_emptyid_assigned_id_emitted) MqttBroker_Free(&broker); } +/* Build a v5 CONNECT with an empty ClientId and n_props minimal User + * Properties. Returns the packet length, or 0 if it would not fit in out_sz. */ +static size_t build_v5_connect_emptyid_userprops(byte* out, size_t out_sz, + int n_props) +{ + static const byte one_prop[] = { + 0x26, 0x00, 0x01, 'k', 0x00, 0x01, 'v' /* User Property "k"="v" */ + }; + word32 props_len = (word32)n_props * (word32)sizeof(one_prop); + byte props_len_vbi[5]; + int props_len_sz; + word32 remain; + byte remain_vbi[5]; + int remain_sz; + size_t pos = 0; + int i; + + props_len_sz = MqttEncode_Vbi(props_len_vbi, props_len); + remain = 6 + 1 + 1 + 2 + (word32)props_len_sz + props_len + 2; + remain_sz = MqttEncode_Vbi(remain_vbi, remain); + if ((size_t)(1 + remain_sz) + remain > out_sz) { + return 0; + } + + out[pos++] = 0x10; /* CONNECT */ + XMEMCPY(out + pos, remain_vbi, (size_t)remain_sz); + pos += (size_t)remain_sz; + out[pos++] = 0x00; out[pos++] = 0x04; + out[pos++] = 'M'; out[pos++] = 'Q'; out[pos++] = 'T'; out[pos++] = 'T'; + out[pos++] = 0x05; /* protocol level 5 */ + out[pos++] = 0x02; /* Clean Start */ + out[pos++] = 0x00; out[pos++] = 0x3C; /* keepalive 60 */ + XMEMCPY(out + pos, props_len_vbi, (size_t)props_len_sz); + pos += (size_t)props_len_sz; + for (i = 0; i < n_props; i++) { + XMEMCPY(out + pos, one_prop, sizeof(one_prop)); + pos += sizeof(one_prop); + } + out[pos++] = 0x00; out[pos++] = 0x00; /* empty ClientId */ + return pos; +} + +/* A v5 CONNECT with an empty ClientId that also fills the shared property pool + * with User Properties must still receive its mandatory Assigned Client + * Identifier in CONNACK: the decoded CONNECT properties are released before the + * CONNACK properties are built. */ +TEST(connect_v5_emptyid_assigned_id_survives_prop_pool_pressure) +{ + MqttBroker broker; + MqttBrokerNet net; + byte connect[MQTT_MAX_PROPS * 7 + 64]; + size_t connect_len; + word16 assigned_id_len; + + 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)); + + connect_len = build_v5_connect_emptyid_userprops(connect, sizeof(connect), + MQTT_MAX_PROPS); + ASSERT_TRUE(connect_len > 0); + reset_mock_state(connect, connect_len); + run_broker_one_connect(&broker); + + /* CONNACK must be accepted and carry the Assigned Client Identifier as its + * first property (see connect_v5_emptyid_assigned_id_emitted for layout). */ + ASSERT_TRUE(g_out_len >= 8); + ASSERT_EQ(0x20, g_out_buf[0]); + ASSERT_EQ(MQTT_REASON_SUCCESS, g_out_buf[3]); + ASSERT_EQ(MQTT_PROP_ASSIGNED_CLIENT_ID, g_out_buf[5]); + assigned_id_len = (word16)((g_out_buf[6] << 8) | g_out_buf[7]); + ASSERT_TRUE(assigned_id_len > 5); + ASSERT_TRUE((size_t)8 + assigned_id_len <= g_out_len); + ASSERT_EQ(0, XMEMCMP(&g_out_buf[8], "auto-", 5)); + ASSERT_FALSE(g_client_closed); + + MqttBroker_Stop(&broker); + MqttBroker_Free(&broker); +} + /* v5: empty ClientId + Clean Start = 0 must also be accepted. v5 dropped * [MQTT-3.1.3-8]; the protocol_level<5 predicate in the broker's rejection * gate must keep this case out of the refuse path. Pins that the v5 escape @@ -4155,6 +4236,383 @@ TEST(broker_retained_clock_rollback_not_expired) MqttBroker_Stop(&broker); MqttBroker_Free(&broker); } + +#if defined(WOLFMQTT_V5) && WOLFMQTT_MAX_QOS >= 1 +/* Return the Reason Code of the first PUBACK/PUBREC in a captured stream. A v5 + * response that omits the reason byte (remain <= 2) means Success; -1 means no + * such packet was found. */ +static int first_publish_resp_reason(const byte* buf, size_t len) +{ + size_t pos = 0; + while (pos < len) { + byte type = (byte)((buf[pos] >> 4) & 0x0F); + size_t remain = 0; + size_t mult = 1; + size_t hdr_len = 1; + int vbi_complete = 0; + while (pos + hdr_len < len && hdr_len <= 5) { + byte b = buf[pos + hdr_len]; + remain += (size_t)(b & 0x7F) * mult; + hdr_len++; + if ((b & 0x80) == 0) { vbi_complete = 1; break; } + mult *= 128; + } + if (!vbi_complete) { + break; + } + if (type == MQTT_PACKET_TYPE_PUBLISH_ACK || + type == MQTT_PACKET_TYPE_PUBLISH_REC) { + if (remain <= 2) { + return MQTT_REASON_SUCCESS; + } + if (pos + hdr_len + 2 < len) { + return buf[pos + hdr_len + 2]; + } + return -1; + } + if (remain > len - pos - hdr_len) { + break; + } + pos += hdr_len + remain; + } + return -1; +} +#endif /* WOLFMQTT_V5 && WOLFMQTT_MAX_QOS >= 1 */ + +#if defined(WOLFMQTT_V5) && WOLFMQTT_MAX_QOS >= 2 +/* Best-effort retain: a v5 QoS 2 PUBLISH whose retained store is full is still + * delivered live to subscribers and answered with a success PUBREC. Only the + * retained copy is skipped; the store stays at capacity. */ +TEST(qos2_retained_store_full_delivers_live) +{ + MqttBroker broker; + MqttBrokerNet net; + int i; + byte fill[8]; + int sub_pubs; + int pub_pubrecs; + + /* v3.1.1 subscriber "A" on "z". */ + static const byte connect_sub_z[] = { + 0x10, 0x0D, 0x00, 0x04, 'M', 'Q', 'T', 'T', + 0x04, 0x02, 0x00, 0x3C, 0x00, 0x01, 'A' + }; + static const byte subscribe_z[] = { + 0x82, 0x06, 0x00, 0x01, 0x00, 0x01, 'z', 0x02 + }; + /* v5 publisher "B". */ + static const byte connect_pub[] = { + 0x10, 0x0E, 0x00, 0x04, 'M', 'Q', 'T', 'T', + 0x05, 0x02, 0x00, 0x3C, 0x00, 0x00, 0x01, 'B' + }; + /* v3.1.1 filler "F" used to exhaust the retained store. */ + static const byte connect_fill[] = { + 0x10, 0x0D, 0x00, 0x04, 'M', 'Q', 'T', 'T', + 0x04, 0x02, 0x00, 0x3C, 0x00, 0x01, 'F' + }; + /* v5 QoS 2 retained PUBLISH, packet_id=7, topic "z", payload "first". + * remain = topic(3) + id(2) + props(1) + payload(5) = 11 */ + static const byte publish_retain[] = { + 0x35, 0x0B, 0x00, 0x01, 'z', 0x00, 0x07, 0x00, + 'f', 'i', 'r', 's', 't' + }; + + 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(3); + mock_client_input_append(0, connect_sub_z, sizeof(connect_sub_z)); + mock_client_input_append(0, subscribe_z, sizeof(subscribe_z)); + mock_client_input_append(1, connect_pub, sizeof(connect_pub)); + mock_client_input_append(2, connect_fill, sizeof(connect_fill)); + for (i = 0; i < BROKER_MAX_RETAINED; i++) { + fill[0] = 0x31; /* PUBLISH, retain=1, QoS 0 */ + fill[1] = 0x06; /* remain = 6 */ + fill[2] = 0x00; fill[3] = 0x03; /* topic len 3 */ + fill[4] = 'r'; + fill[5] = (byte)('0' + (i / 10)); + fill[6] = (byte)('0' + (i % 10)); + fill[7] = 'x'; /* payload */ + mock_client_input_append(2, fill, sizeof(fill)); + } + for (i = 0; i < BROKER_MAX_RETAINED + 32; i++) { + MqttBroker_Step(&broker); + } + ASSERT_EQ(BROKER_MAX_RETAINED, broker.retained_count); + + /* Table is full, so the retained copy is skipped, but the message is still + * delivered live and answered with a success PUBREC. */ + mock_client_input_append(1, publish_retain, sizeof(publish_retain)); + for (i = 0; i < 16; i++) { + MqttBroker_Step(&broker); + } + ASSERT_EQ(BROKER_MAX_RETAINED, broker.retained_count); + + sub_pubs = count_packets_of_type(g_clients[0].out_buf, + g_clients[0].out_len, MQTT_PACKET_TYPE_PUBLISH); + pub_pubrecs = count_packets_of_type(g_clients[1].out_buf, + g_clients[1].out_len, MQTT_PACKET_TYPE_PUBLISH_REC); + + ASSERT_EQ(1, sub_pubs); /* delivered live despite full table */ + ASSERT_EQ(1, pub_pubrecs); + ASSERT_EQ(MQTT_REASON_SUCCESS, + first_publish_resp_reason(g_clients[1].out_buf, + g_clients[1].out_len)); + + MqttBroker_Stop(&broker); + MqttBroker_Free(&broker); +} +#endif /* WOLFMQTT_V5 && WOLFMQTT_MAX_QOS >= 2 */ + +#if WOLFMQTT_MAX_QOS >= 2 +/* Best-effort retain: a v3.1.1 QoS 2 PUBLISH whose retained store is full is + * still delivered live to subscribers, and its handshake completes normally. + * Only the retained copy is skipped. */ +TEST(qos2_retained_store_full_v311_delivers_live) +{ + MqttBroker broker; + MqttBrokerNet net; + int i; + byte fill[8]; + int sub_pubs; + int pub_pubrecs; + int pub_pubcomps; + + /* v3.1.1 subscriber "A", subscribes to "x" at QoS 2. */ + static const byte connect_sub[] = { + 0x10, 0x0D, 0x00, 0x04, 'M', 'Q', 'T', 'T', + 0x04, 0x02, 0x00, 0x3C, 0x00, 0x01, 'A' + }; + static const byte subscribe_x[] = { + 0x82, 0x06, 0x00, 0x01, 0x00, 0x01, 'x', 0x02 + }; + /* v3.1.1 publisher "B". */ + static const byte connect_pub[] = { + 0x10, 0x0D, 0x00, 0x04, 'M', 'Q', 'T', 'T', + 0x04, 0x02, 0x00, 0x3C, 0x00, 0x01, 'B' + }; + /* v3.1.1 filler "F" used to exhaust the retained store. */ + static const byte connect_fill[] = { + 0x10, 0x0D, 0x00, 0x04, 'M', 'Q', 'T', 'T', + 0x04, 0x02, 0x00, 0x3C, 0x00, 0x01, 'F' + }; + /* v3.1.1 QoS 2 retained PUBLISH, packet_id=7, topic "x", payload "live". + * remain = topic(3) + id(2) + payload(4) = 9 */ + static const byte publish_retain[] = { + 0x35, 0x09, 0x00, 0x01, 'x', 0x00, 0x07, + 'l', 'i', 'v', 'e' + }; + static const byte pubrel[] = { + 0x62, 0x02, 0x00, 0x07 + }; + + 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(3); + mock_client_input_append(0, connect_sub, sizeof(connect_sub)); + mock_client_input_append(0, subscribe_x, sizeof(subscribe_x)); + mock_client_input_append(1, connect_pub, sizeof(connect_pub)); + mock_client_input_append(2, connect_fill, sizeof(connect_fill)); + for (i = 0; i < BROKER_MAX_RETAINED; i++) { + fill[0] = 0x31; /* PUBLISH, retain=1, QoS 0 */ + fill[1] = 0x06; /* remain = 6 */ + fill[2] = 0x00; fill[3] = 0x03; /* topic len 3 */ + fill[4] = 'r'; + fill[5] = (byte)('0' + (i / 10)); + fill[6] = (byte)('0' + (i % 10)); + fill[7] = 'x'; /* payload */ + mock_client_input_append(2, fill, sizeof(fill)); + } + for (i = 0; i < BROKER_MAX_RETAINED + 32; i++) { + MqttBroker_Step(&broker); + } + ASSERT_EQ(BROKER_MAX_RETAINED, broker.retained_count); + + /* Table is full, so the retained copy is skipped, but the message is still + * delivered live and the v3.1.1 handshake completes normally. */ + mock_client_input_append(1, publish_retain, sizeof(publish_retain)); + mock_client_input_append(1, pubrel, sizeof(pubrel)); + for (i = 0; i < 16; i++) { + MqttBroker_Step(&broker); + } + + sub_pubs = count_packets_of_type(g_clients[0].out_buf, + g_clients[0].out_len, MQTT_PACKET_TYPE_PUBLISH); + pub_pubrecs = count_packets_of_type(g_clients[1].out_buf, + g_clients[1].out_len, MQTT_PACKET_TYPE_PUBLISH_REC); + pub_pubcomps = count_packets_of_type(g_clients[1].out_buf, + g_clients[1].out_len, MQTT_PACKET_TYPE_PUBLISH_COMP); + + ASSERT_EQ(1, sub_pubs); /* delivered live despite full table */ + ASSERT_EQ(1, pub_pubrecs); + ASSERT_EQ(1, pub_pubcomps); /* handshake completes normally */ + + MqttBroker_Stop(&broker); + MqttBroker_Free(&broker); +} +#endif /* WOLFMQTT_MAX_QOS >= 2 */ + +#if defined(WOLFMQTT_V5) && WOLFMQTT_MAX_QOS >= 1 +/* Best-effort retain on the QoS 1 path most applications hit: a v5 QoS 1 + * PUBLISH whose retained store is full is still delivered live and answered + * with a success PUBACK. Only the retained copy is skipped. */ +TEST(qos1_retained_store_full_delivers_live) +{ + MqttBroker broker; + MqttBrokerNet net; + int i; + byte fill[8]; + int sub_pubs; + int pub_pubacks; + + /* v3.1.1 subscriber "A" on "x". */ + static const byte connect_sub[] = { + 0x10, 0x0D, 0x00, 0x04, 'M', 'Q', 'T', 'T', + 0x04, 0x02, 0x00, 0x3C, 0x00, 0x01, 'A' + }; + static const byte subscribe_x[] = { + 0x82, 0x06, 0x00, 0x01, 0x00, 0x01, 'x', 0x01 + }; + /* v5 publisher "B". */ + static const byte connect_pub[] = { + 0x10, 0x0E, 0x00, 0x04, 'M', 'Q', 'T', 'T', + 0x05, 0x02, 0x00, 0x3C, 0x00, 0x00, 0x01, 'B' + }; + /* v3.1.1 filler "F" used to exhaust the retained store. */ + static const byte connect_fill[] = { + 0x10, 0x0D, 0x00, 0x04, 'M', 'Q', 'T', 'T', + 0x04, 0x02, 0x00, 0x3C, 0x00, 0x01, 'F' + }; + /* v5 QoS 1 retained PUBLISH, packet_id=7, topic "x", payload "live". + * remain = topic(3) + id(2) + props(1) + payload(4) = 10 */ + static const byte publish_retain[] = { + 0x33, 0x0A, 0x00, 0x01, 'x', 0x00, 0x07, 0x00, + 'l', 'i', 'v', 'e' + }; + + 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(3); + mock_client_input_append(0, connect_sub, sizeof(connect_sub)); + mock_client_input_append(0, subscribe_x, sizeof(subscribe_x)); + mock_client_input_append(1, connect_pub, sizeof(connect_pub)); + mock_client_input_append(2, connect_fill, sizeof(connect_fill)); + for (i = 0; i < BROKER_MAX_RETAINED; i++) { + fill[0] = 0x31; /* PUBLISH, retain=1, QoS 0 */ + fill[1] = 0x06; /* remain = 6 */ + fill[2] = 0x00; fill[3] = 0x03; /* topic len 3 */ + fill[4] = 'r'; + fill[5] = (byte)('0' + (i / 10)); + fill[6] = (byte)('0' + (i % 10)); + fill[7] = 'x'; /* payload */ + mock_client_input_append(2, fill, sizeof(fill)); + } + for (i = 0; i < BROKER_MAX_RETAINED + 32; i++) { + MqttBroker_Step(&broker); + } + ASSERT_EQ(BROKER_MAX_RETAINED, broker.retained_count); + + mock_client_input_append(1, publish_retain, sizeof(publish_retain)); + for (i = 0; i < 16; i++) { + MqttBroker_Step(&broker); + } + ASSERT_EQ(BROKER_MAX_RETAINED, broker.retained_count); + + sub_pubs = count_packets_of_type(g_clients[0].out_buf, + g_clients[0].out_len, MQTT_PACKET_TYPE_PUBLISH); + pub_pubacks = count_packets_of_type(g_clients[1].out_buf, + g_clients[1].out_len, MQTT_PACKET_TYPE_PUBLISH_ACK); + + ASSERT_EQ(1, sub_pubs); /* delivered live despite full table */ + ASSERT_EQ(1, pub_pubacks); + ASSERT_EQ(MQTT_REASON_SUCCESS, + first_publish_resp_reason(g_clients[1].out_buf, + g_clients[1].out_len)); + MqttBroker_Stop(&broker); + MqttBroker_Free(&broker); +} + +#ifndef WOLFMQTT_STATIC_MEMORY +/* Best-effort retain also covers a transient store failure: an allocation + * failure in the retained store still delivers the message live and + * acknowledges success, and stores nothing. */ +TEST(qos1_retained_store_alloc_failure_delivers_live) +{ + MqttBroker broker; + MqttBrokerNet net; + int i; + int sub_pubs; + int pub_pubacks; + + /* v3.1.1 subscriber "A" on "x". */ + static const byte connect_sub[] = { + 0x10, 0x0D, 0x00, 0x04, 'M', 'Q', 'T', 'T', + 0x04, 0x02, 0x00, 0x3C, 0x00, 0x01, 'A' + }; + static const byte subscribe_x[] = { + 0x82, 0x06, 0x00, 0x01, 0x00, 0x01, 'x', 0x01 + }; + /* v5 publisher "B". */ + static const byte connect_pub[] = { + 0x10, 0x0E, 0x00, 0x04, 'M', 'Q', 'T', 'T', + 0x05, 0x02, 0x00, 0x3C, 0x00, 0x00, 0x01, 'B' + }; + /* v5 QoS 1 retained PUBLISH, packet_id=7, topic "x", payload "live". + * remain = topic(3) + id(2) + props(1) + payload(4) = 10 */ + static const byte publish_retain[] = { + 0x33, 0x0A, 0x00, 0x01, 'x', 0x00, 0x07, 0x00, + 'l', 'i', 'v', 'e' + }; + + 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(2); + mock_client_input_append(0, connect_sub, sizeof(connect_sub)); + mock_client_input_append(0, subscribe_x, sizeof(subscribe_x)); + mock_client_input_append(1, connect_pub, sizeof(connect_pub)); + for (i = 0; i < 16; i++) { + MqttBroker_Step(&broker); + } + + /* The retained table is far below capacity, so the store fails only because + * the injected allocation fails. The topic copy allocates first and + * succeeds; the next allocation is the retained store's, which fails. */ + broker_test_fail_alloc_after(1); + mock_client_input_append(1, publish_retain, sizeof(publish_retain)); + for (i = 0; i < 16; i++) { + MqttBroker_Step(&broker); + } + broker_test_disable_alloc_failure(); + + sub_pubs = count_packets_of_type(g_clients[0].out_buf, + g_clients[0].out_len, MQTT_PACKET_TYPE_PUBLISH); + pub_pubacks = count_packets_of_type(g_clients[1].out_buf, + g_clients[1].out_len, MQTT_PACKET_TYPE_PUBLISH_ACK); + + ASSERT_EQ(1, g_alloc_failure_count); /* the retained store did fail */ + ASSERT_EQ(0, broker.retained_count); /* nothing was stored */ + ASSERT_EQ(1, sub_pubs); /* but still delivered live */ + ASSERT_EQ(1, pub_pubacks); + ASSERT_EQ(MQTT_REASON_SUCCESS, + first_publish_resp_reason(g_clients[1].out_buf, + g_clients[1].out_len)); /* acked success, not Quota */ + + MqttBroker_Stop(&broker); + MqttBroker_Free(&broker); +} +#endif /* !WOLFMQTT_STATIC_MEMORY */ +#endif /* WOLFMQTT_V5 && WOLFMQTT_MAX_QOS >= 1 */ #endif /* WOLFMQTT_BROKER_RETAINED && !WOLFMQTT_STATIC_MEMORY */ #ifndef WOLFMQTT_STATIC_MEMORY @@ -9266,6 +9724,7 @@ int main(int argc, char** argv) #endif #ifdef WOLFMQTT_V5 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); #endif #ifndef WOLFMQTT_STATIC_MEMORY @@ -9340,6 +9799,16 @@ int main(int argc, char** argv) #if defined(WOLFMQTT_BROKER_RETAINED) && !defined(WOLFMQTT_STATIC_MEMORY) RUN_TEST(broker_retained_list_capped); RUN_TEST(broker_retained_clock_rollback_not_expired); +#if defined(WOLFMQTT_V5) && WOLFMQTT_MAX_QOS >= 2 + RUN_TEST(qos2_retained_store_full_delivers_live); +#endif +#if WOLFMQTT_MAX_QOS >= 2 + RUN_TEST(qos2_retained_store_full_v311_delivers_live); +#endif +#if defined(WOLFMQTT_V5) && WOLFMQTT_MAX_QOS >= 1 + RUN_TEST(qos1_retained_store_full_delivers_live); + RUN_TEST(qos1_retained_store_alloc_failure_delivers_live); +#endif RUN_TEST(broker_retained_scrub_after_completed_write); #ifdef WOLFMQTT_NONBLOCK RUN_TEST(retained_short_write_preserves_following_delivery); diff --git a/tests/test_mqtt_client.c b/tests/test_mqtt_client.c index d984be3..1cea250 100644 --- a/tests/test_mqtt_client.c +++ b/tests/test_mqtt_client.c @@ -534,6 +534,49 @@ TEST(connect_clears_tx_buf_credentials) } } +#ifdef WOLFMQTT_V5 +/* The client cannot resolve inbound Topic Aliases, so MqttClient_Connect must + * not advertise the capability: a caller-supplied nonzero Topic Alias Maximum + * is rejected before any CONNECT is sent, rather than advertised and then + * contradicted when the server's aliased PUBLISH is refused. */ +TEST(connect_rejects_nonzero_inbound_topic_alias_max) +{ + int rc; + MqttConnect connect; + MqttProp ta_max_prop; + + rc = test_init_client(); + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + test_client.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; + + connect_mock_xfer = 0; + XMEMSET(connect_mock_sent, 0, sizeof(connect_mock_sent)); + test_net.write = mock_net_write_accept; + + XMEMSET(&connect, 0, sizeof(connect)); + connect.keep_alive_sec = 60; + connect.clean_session = 1; + connect.client_id = "test_client"; + XMEMSET(&ta_max_prop, 0, sizeof(ta_max_prop)); + ta_max_prop.type = MQTT_PROP_TOPIC_ALIAS_MAX; + ta_max_prop.data_short = 5; + ta_max_prop.next = NULL; + connect.props = &ta_max_prop; + + rc = MqttClient_Connect(&test_client, &connect); + ASSERT_EQ(MQTT_CODE_ERROR_PROPERTY, rc); + /* Rejected before the transport was touched. */ + ASSERT_EQ(0, connect_mock_xfer); + + /* A zero (or absent) Topic Alias Maximum is accepted and does reach the + * write path. */ + ta_max_prop.data_short = 0; + rc = MqttClient_Connect(&test_client, &connect); + ASSERT_NE(MQTT_CODE_ERROR_PROPERTY, rc); + ASSERT_TRUE(connect_mock_xfer > 0); +} +#endif /* WOLFMQTT_V5 */ + /* Serves a pre-staged response packet (e.g. a SUBACK) one chunk per read so a * full client request/response round-trip can run against the mock net. */ static byte g_canned_buf[64]; @@ -2174,7 +2217,9 @@ TEST(publish_v5_within_max_packet_size_allowed) /* MQTT 5.0 section 3.2.2.3.4: Maximum QoS can only be 0 or 1. Feed an * independently constructed CONNACK containing 2 and require the client to - * reject the connection instead of normalizing the invalid wire value. */ + * reject the connection instead of normalizing the invalid wire value. The + * out-of-range Byte value is now caught in MqttDecode_Props at the wire + * boundary (MQTT_CODE_ERROR_PROPERTY). */ TEST(connect_accepted_connack_rejects_illegal_max_qos) { int rc; @@ -2207,14 +2252,14 @@ TEST(connect_accepted_connack_rejects_illegal_max_qos) rc = MqttClient_Connect(&test_client, &connect); } - ASSERT_EQ(MQTT_CODE_ERROR_SERVER_PROP, rc); - ASSERT_EQ(MQTT_CONNECT_ACK_CODE_ACCEPTED, connect.ack.return_code); + ASSERT_EQ(MQTT_CODE_ERROR_PROPERTY, rc); ASSERT_EQ(WOLFMQTT_MAX_QOS, test_client.max_qos); } /* MQTT 5.0 section 3.2.2.3.5: Retain Available can only be 0 or 1. Feed an * independently constructed CONNACK containing 2 and require a protocol - * failure rather than accepting it as Retain Available=1. */ + * failure rather than accepting it as Retain Available=1. The out-of-range + * Byte value is now caught in MqttDecode_Props (MQTT_CODE_ERROR_PROPERTY). */ TEST(connect_accepted_connack_rejects_illegal_retain_available) { int rc; @@ -2248,8 +2293,7 @@ TEST(connect_accepted_connack_rejects_illegal_retain_available) rc = MqttClient_Connect(&test_client, &connect); } - ASSERT_EQ(MQTT_CODE_ERROR_SERVER_PROP, rc); - ASSERT_EQ(MQTT_CONNECT_ACK_CODE_ACCEPTED, connect.ack.return_code); + ASSERT_EQ(MQTT_CODE_ERROR_PROPERTY, rc); ASSERT_EQ(1, test_client.retain_avail); } #endif /* WOLFMQTT_V5 */ @@ -6410,10 +6454,12 @@ TEST(wait_message_timeout_preserves_partial_vbi) #endif /* WOLFMQTT_NONBLOCK */ #ifdef WOLFMQTT_V5 -/* MQTT v5 section 3.3.2.3.4 permits an empty Topic Name when a nonzero Topic - * Alias property supplies the topic. This fixed wire fixture reaches the +/* The client advertises Topic Alias Maximum 0 and keeps no inbound alias table, + * so an empty Topic Name paired with a Topic Alias is a non-conforming server + * PUBLISH that cannot be resolved; it must be rejected, not delivered to the + * callback with a zero-length topic. This fixed wire fixture reaches the * preliminary packet decode used by MqttClient_WaitMessage. */ -TEST(wait_message_v5_empty_topic_with_alias_delivered) +TEST(wait_message_v5_empty_topic_with_alias_rejected) { int rc; int i; @@ -6438,8 +6484,42 @@ TEST(wait_message_v5_empty_topic_with_alias_delivered) rc = MqttClient_WaitMessage(&test_client, TEST_CMD_TIMEOUT_MS); } + ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); + ASSERT_EQ(0, g_msg_cb_calls); +} + +/* The client advertises Topic Alias Maximum 0, so any inbound Topic Alias, even + * on a non-empty Topic Name, is a non-conforming server PUBLISH. The client + * keeps no alias table and must reject it rather than accept the alias. Wire: + * PUBLISH QoS 0, remain=9, topic "ta", props_len=3, TOPIC_ALIAS(35)=1, + * payload "x". */ +TEST(wait_message_v5_topic_alias_with_topic_rejected) +{ + int rc; + int i; + static const byte publish_v5[] = { + 0x30, 0x09, 0x00, 0x02, 't', 'a', 0x03, 0x23, 0x00, 0x01, 'x' + }; + + rc = test_init_client(); ASSERT_EQ(MQTT_CODE_SUCCESS, rc); - ASSERT_TRUE(g_msg_cb_calls > 0); + test_client.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; + test_client.msg_cb = test_accept_message_cb; + g_msg_cb_calls = 0; + + test_net.write = mock_net_write_accept; + test_net.read = mock_net_read_canned; + XMEMCPY(g_canned_buf, publish_v5, sizeof(publish_v5)); + g_canned_len = (int)sizeof(publish_v5); + g_canned_pos = 0; + + rc = MQTT_CODE_CONTINUE; + for (i = 0; i < 20 && rc == MQTT_CODE_CONTINUE; i++) { + rc = MqttClient_WaitMessage(&test_client, TEST_CMD_TIMEOUT_MS); + } + + ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); + ASSERT_EQ(0, g_msg_cb_calls); } #endif /* WOLFMQTT_V5 */ @@ -7499,6 +7579,9 @@ void run_mqtt_client_tests(void) RUN_TEST(publish_after_connect_allowed); RUN_TEST(second_connect_on_same_network_connection_rejected); RUN_TEST(connect_clears_tx_buf_credentials); +#ifdef WOLFMQTT_V5 + RUN_TEST(connect_rejects_nonzero_inbound_topic_alias_max); +#endif RUN_TEST(connect_accepted_connack_returns_success); RUN_TEST(connect_clean_session_present_mismatch_refused); RUN_TEST(connect_resume_session_present_accepted); @@ -7703,7 +7786,8 @@ void run_mqtt_client_tests(void) #endif #ifdef WOLFMQTT_V5 RUN_TEST(wait_message_v5_props_null_msg_cb_frees_props); - RUN_TEST(wait_message_v5_empty_topic_with_alias_delivered); + RUN_TEST(wait_message_v5_empty_topic_with_alias_rejected); + RUN_TEST(wait_message_v5_topic_alias_with_topic_rejected); #endif RUN_TEST(wait_message_qos1_with_msg_cb_delivers_and_acks); #ifdef WOLFMQTT_NONBLOCK diff --git a/tests/test_mqtt_packet.c b/tests/test_mqtt_packet.c index 33f51a5..b27cea9 100644 --- a/tests/test_mqtt_packet.c +++ b/tests/test_mqtt_packet.c @@ -1394,9 +1394,10 @@ TEST(decode_publish_topic_contains_u0000_rejected) } #ifdef WOLFMQTT_V5 -/* MQTT v5 section 3.3.2.3.4: a zero-length Topic Name is permitted only when - * paired with a Topic Alias property. Wire: PUBLISH QoS 0, remain=7, - * topic_len=0, props_len=3, TOPIC_ALIAS(35)=1, payload "x". */ +/* MQTT v5 section 3.3.2.3.4: a zero-length Topic Name is valid at the wire + * level when paired with a Topic Alias. The decoder accepts it; the client + * layer decides whether it can resolve the alias. Wire: PUBLISH QoS 0, + * remain=7, topic_len=0, props_len=3, TOPIC_ALIAS(35)=1, payload "x". */ TEST(decode_publish_v5_empty_topic_with_alias_accepted) { byte buf[] = { 0x30, 0x07, 0x00, 0x00, 0x03, 0x23, 0x00, 0x01, 'x' }; @@ -1411,6 +1412,26 @@ TEST(decode_publish_v5_empty_topic_with_alias_accepted) MqttProps_Free(pub.props); } +/* A non-empty Topic Name paired with a Topic Alias is a valid v5 PUBLISH that + * establishes or updates the alias mapping; the decoder accepts it at the wire + * level. Wire: PUBLISH QoS 0, remain=9, topic "ta", props_len=3, + * TOPIC_ALIAS(35)=1, payload "x". */ +TEST(decode_publish_v5_topic_alias_with_topic_accepted) +{ + byte buf[] = { 0x30, 0x09, 0x00, 0x02, 't', 'a', 0x03, + 0x23, 0x00, 0x01, 'x' }; + MqttPublish pub; + int rc; + + XMEMSET(&pub, 0, sizeof(pub)); + pub.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; + rc = MqttDecode_Publish(buf, (int)sizeof(buf), &pub); + ASSERT_TRUE(rc > 0); + ASSERT_EQ(2, pub.topic_name_len); + ASSERT_TRUE(pub.props != NULL); + MqttProps_Free(pub.props); +} + /* [MQTT-3.3.2-8] A zero-length Topic Name with no Topic Alias property is a * Protocol Error. Wire: PUBLISH QoS 0, remain=4, topic_len=0, props_len=0. */ TEST(decode_publish_v5_empty_topic_no_alias_rejected) @@ -1456,6 +1477,23 @@ TEST(decode_publish_v5_topic_alias_zero_rejected) ASSERT_NULL(pub.props); } +/* A Byte property value other than 0 or 1 is a Protocol Error and must be + * rejected at decode, symmetric with MqttEncode_Props, so a decoded property + * always re-encodes. Wire: PUBLISH QoS 0, topic "t", props_len=2, + * PAYLOAD_FORMAT_IND(1)=2. */ +TEST(decode_publish_v5_byte_property_out_of_range_rejected) +{ + byte buf[] = { 0x30, 0x07, 0x00, 0x01, 't', 0x02, 0x01, 0x02, 'x' }; + MqttPublish pub; + int rc; + + XMEMSET(&pub, 0, sizeof(pub)); + pub.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; + rc = MqttDecode_Publish(buf, (int)sizeof(buf), &pub); + ASSERT_EQ(MQTT_CODE_ERROR_PROPERTY, rc); + ASSERT_NULL(pub.props); +} + /* [MQTT-3.3.2-14] A Response Topic is a Topic Name and MUST NOT contain * wildcards. Wire: PUBLISH QoS 0, topic "t", props_len=6, RESP_TOPIC(8)="a/#". */ TEST(decode_publish_v5_response_topic_wildcard_rejected) @@ -6395,6 +6433,45 @@ TEST(encode_props_duplicate_repeatability) ASSERT_TRUE(rc > 0); } +/* Every MQTT 5 Byte property is Boolean-valued: its only legal values are 0 and + * 1 (Maximum QoS uses the same {0,1} domain, absence signals QoS 2). The + * encoder must reject any other value so it cannot emit a property a peer + * treats as a Protocol Error. Exercised in the length pass (buf == NULL). */ +TEST(encode_props_boolean_byte_out_of_range_rejected) +{ + MqttProp prop; + int rc; + + XMEMSET(&prop, 0, sizeof(prop)); + prop.type = MQTT_PROP_REQ_RESP_INFO; + prop.data_byte = 2; + prop.next = NULL; + rc = MqttEncode_Props(MQTT_PACKET_TYPE_CONNECT, &prop, NULL); + ASSERT_EQ(MQTT_CODE_ERROR_PROPERTY, rc); + + XMEMSET(&prop, 0, sizeof(prop)); + prop.type = MQTT_PROP_RETAIN_AVAIL; + prop.data_byte = 2; + prop.next = NULL; + rc = MqttEncode_Props(MQTT_PACKET_TYPE_CONNECT_ACK, &prop, NULL); + ASSERT_EQ(MQTT_CODE_ERROR_PROPERTY, rc); + + XMEMSET(&prop, 0, sizeof(prop)); + prop.type = MQTT_PROP_MAX_QOS; + prop.data_byte = 2; + prop.next = NULL; + rc = MqttEncode_Props(MQTT_PACKET_TYPE_CONNECT_ACK, &prop, NULL); + ASSERT_EQ(MQTT_CODE_ERROR_PROPERTY, rc); + + /* A legal value still encodes. */ + XMEMSET(&prop, 0, sizeof(prop)); + prop.type = MQTT_PROP_RETAIN_AVAIL; + prop.data_byte = 1; + prop.next = NULL; + rc = MqttEncode_Props(MQTT_PACKET_TYPE_CONNECT_ACK, &prop, NULL); + ASSERT_TRUE(rc > 0); +} + /* ============================================================================ * MqttEncode/Decode_Auth roundtrip * @@ -6902,9 +6979,11 @@ void run_mqtt_packet_tests(void) RUN_TEST(decode_publish_topic_contains_u0000_rejected); #ifdef WOLFMQTT_V5 RUN_TEST(decode_publish_v5_empty_topic_with_alias_accepted); + RUN_TEST(decode_publish_v5_topic_alias_with_topic_accepted); RUN_TEST(decode_publish_v5_empty_topic_no_alias_rejected); RUN_TEST(decode_publish_v5_subscription_id_zero_rejected); RUN_TEST(decode_publish_v5_topic_alias_zero_rejected); + RUN_TEST(decode_publish_v5_byte_property_out_of_range_rejected); RUN_TEST(decode_publish_v5_response_topic_wildcard_rejected); RUN_TEST(encode_publish_v5_response_topic_wildcard_rejected); RUN_TEST(decode_publish_v5_property_count_capped); @@ -7221,6 +7300,7 @@ void run_mqtt_packet_tests(void) RUN_TEST(encode_props_string_invalid_utf8_rejected); RUN_TEST(encode_props_user_prop_invalid_utf8_rejected); RUN_TEST(encode_props_duplicate_repeatability); + RUN_TEST(encode_props_boolean_byte_out_of_range_rejected); RUN_TEST(auth_v5_cont_auth_roundtrip); RUN_TEST(auth_v5_reauth_roundtrip); RUN_TEST(auth_v5_reauth_decodes_without_error); diff --git a/wolfmqtt/mqtt_packet.h b/wolfmqtt/mqtt_packet.h index a86475f..25efb2b 100644 --- a/wolfmqtt/mqtt_packet.h +++ b/wolfmqtt/mqtt_packet.h @@ -48,6 +48,16 @@ #define MAX_MQTT_TOPICS 12 #endif +/* Maximum number of MQTT v5 properties in one packet, and the size of the + * shared property pool. Also bounds property-list traversal outside the pool + * allocator. Override in user_settings.h to trade memory for a larger set. */ +#ifndef MQTT_MAX_PROPS +#define MQTT_MAX_PROPS 30 +#endif +#if (MQTT_MAX_PROPS < 1) || (MQTT_MAX_PROPS > 65535) + #error "MQTT_MAX_PROPS must be between 1 and 65535" +#endif + /* WOLFMQTT_NO_UTF8_VALIDATION * Define to disable RFC 3629 UTF-8 well-formedness validation on the * encode side (MqttEncode_Utf8Ok). Decode-side validation in