Merge pull request #484 from embhorn/fenrir-tests

aidangarske · web-flow · commit 03667b45666e · 2026-04-27T09:47:05.000-07:00
Add fenrir suggested test cases
diff --git a/AGENTS.md b/AGENTS.md
@@ -147,6 +147,50 @@ Uses `.clang-format` with LLVM base style:
 - Tab indentation (4-space tabs)
 - K&R inspired style
 
+## Test Integrity
+Never modify, delete, skip, or weaken tests to make them pass.
+Never fabricate, adjust, or derive expected values from the code under test just to force a pass; fixed expected values are acceptable when they come from an independent oracle, such as committed test vectors or other externally verified results.
+A passing test suite achieved by changing the tests (not the implementation) is not a passing result.
+Fix the code. If the code cannot be fixed within scope, escalate.
+
+Do not use the code under test as its only oracle where an independent oracle is required, especially for crypto, KDFs, canonical encodings, and other security-sensitive transformations. In those cases, tests should use known external test vectors, cross-validation against an independent implementation, or bit-exact comparison against a trusted reference path. For example, a test that only encrypts with function A and decrypts with function A is insufficient to validate the correctness of the cryptographic primitive.
+
+Roundtrip/property tests are still acceptable where they match the behavior being validated, such as encode/decode or serialize/parse flows already used elsewhere in this repository, but they should not be the sole oracle when stronger independent validation is needed.
+
+## No Fabrication
+Never report status, results, or completion that does not reflect work actually performed.
+If you are uncertain whether a step succeeded, say so explicitly. Do not paper over uncertainty with confident-sounding output.
+
+## Exit Code Discipline
+Every shell command's exit code must be checked.
+Never proceed after a silent failure.
+A command that failed and was ignored is not a completed step.
+
+## MQTT Specification Discipline
+Wire format and protocol behavior are governed by the published MQTT specifications. Treat the spec as the source of truth, not the code.
+
+Relevant specifications:
+- MQTT v3.1.1 — OASIS Standard (`mqtt-v3.1.1-os`)
+- MQTT v5.0 — OASIS Standard (`mqtt-v5.0`)
+- MQTT-SN v1.2 — OASIS
+
+When implementing or testing a normative requirement, cite it in a comment so reviewers can verify against the spec:
+- MQTT v3.1.1 / v5.0: bracketed conformance identifiers, e.g. `[MQTT-3.8.1-1]`, `[MQTT-2.3.1-1]`.
+- When a rule has no bracketed identifier, reference the section number, e.g. `MQTT 5.0 section 3.15.2`.
+- MQTT-SN v1.2: `MQTT-SN 1.2 section X.Y`.
+
+Match the version scope of the change. MQTT v5.0 adds packets and fields (AUTH, Reason Codes, Properties) that do not exist in v3.1.1; do not apply a v5-only rule to v3.1.1 code paths or vice versa. Guard v5-only logic with `WOLFMQTT_V5` and v3.1.1-only logic accordingly.
+
+## Test Oracle Discipline
+Do not use the code under test as its own oracle for wire-format behavior. For encoder or decoder tests, use one of:
+- A hand-constructed byte sequence that matches the spec wire format, built by reading the relevant section (not captured from encoder output). Comment the byte layout so the fixture is auditable.
+- Values from the spec's worked examples or conformance annex.
+- A cross-check against an independent implementation (e.g. mosquitto) captured once and committed as a fixed byte array.
+
+Roundtrip tests (encode then decode, or vice versa) are acceptable for regression and structural coverage, but they cannot be the sole oracle for a wire-format rule — a bug present in both encoder and decoder will still roundtrip. Pair roundtrip coverage with at least one independent fixture per rule.
+
+Tests must be fully offline and must not fetch vectors from the network at runtime.
+
 ## Dependencies
 
 - **wolfSSL** - Required for TLS support
diff --git a/scripts/broker.test b/scripts/broker.test
@@ -297,6 +297,93 @@ else
 fi
 fi # has_will
 
+# --- Test 6b: Keep-alive timeout window precision ---
+echo ""
+echo "--- Test 6b: Keep-alive timeout window ---"
+if [ "$has_will" = "no" ]; then
+    echo "SKIP: Keep-alive timeout window (not built with will support)"
+else
+# Validate that the broker's 1.5x keep_alive multiplier (mqtt_broker.c:
+# keep_alive_sec * 3 / 2) is honored. Test 6 above only checks that the LWT
+# is eventually delivered, so silent mutations (e.g. *1, *2, *3) would still
+# pass. This test measures wall-clock time from client freeze to LWT
+# delivery and asserts a tight bound that catches significant multiplier
+# changes.
+#
+# k=4 is used to widen the discrimination window. The broker checks timing
+# at integer-second resolution, so the keepalive fire happens at
+# last_rx_int + threshold + 1 wall seconds, where threshold = floor(k*mult).
+#
+# Expected fire window (relative to client freeze):
+#   mult  threshold  fire window (s)  +will_delay (v5)
+#   1.0       4           4-5              9-10
+#   1.5       6           6-7             11-12   <- spec compliant
+#   2.0       8           8-9             13-14
+#   3.0      12          12-13            17-18
+#   4.0      16          16-17            21-22
+v5_active=no
+if ./$sub_bin -h 2>&1 | grep -q "Max packet size"; then
+    v5_active=yes
+fi
+if [ "$v5_active" = "yes" ]; then
+    LO_BOUND=10
+    HI_BOUND=14
+else
+    LO_BOUND=5
+    HI_BOUND=9
+fi
+rm -f "${TMP_DIR}/t6b_watcher.ready" "${TMP_DIR}/t6b_client.ready"
+t6b_setup_ok=1
+timeout 30 ./$sub_bin -T -h 127.0.0.1 -p $port -n "wolfMQTT/example/lwttopic" \
+    -i "t6b_watcher" -R "${TMP_DIR}/t6b_watcher.ready" \
+    >"${TMP_DIR}/t6b_sub.log" 2>&1 &
+T6B_WATCHER_PID=$!
+TEST_PIDS+=($T6B_WATCHER_PID)
+if ! wait_for_file "${TMP_DIR}/t6b_watcher.ready" 5; then
+    echo "FAIL: Keep-alive timeout window (watcher did not become ready)"
+    FAIL=1
+    t6b_setup_ok=0
+fi
+./$sub_bin -T -h 127.0.0.1 -p $port -n "test/ka2_trigger" -i "ka2_client" -l -k 4 \
+    -R "${TMP_DIR}/t6b_client.ready" >"${TMP_DIR}/t6b_client.log" 2>&1 &
+T6B_CLIENT_PID=$!
+TEST_PIDS+=($T6B_CLIENT_PID)
+if ! wait_for_file "${TMP_DIR}/t6b_client.ready" 5; then
+    echo "FAIL: Keep-alive timeout window (client did not become ready)"
+    FAIL=1
+    t6b_setup_ok=0
+fi
+if [ $t6b_setup_ok -eq 1 ]; then
+    kill -STOP $T6B_CLIENT_PID 2>/dev/null
+    T6B_T_START=$(date +%s)
+    T6B_ELAPSED=-1
+    T6B_DEADLINE=$((T6B_T_START + HI_BOUND + 5))
+    while [ $(date +%s) -lt $T6B_DEADLINE ]; do
+        if grep -q "ka2_client" "${TMP_DIR}/t6b_sub.log" 2>/dev/null; then
+            T6B_ELAPSED=$(($(date +%s) - T6B_T_START))
+            break
+        fi
+        sleep 0.1
+    done
+fi
+kill -9 $T6B_CLIENT_PID 2>/dev/null
+wait $T6B_CLIENT_PID 2>/dev/null || true
+kill $T6B_WATCHER_PID 2>/dev/null
+wait $T6B_WATCHER_PID 2>/dev/null || true
+TEST_PIDS=()
+if [ $t6b_setup_ok -eq 1 ]; then
+    if [ $T6B_ELAPSED -lt 0 ]; then
+        echo "FAIL: Keep-alive timeout window (no LWT within $((HI_BOUND + 5))s)"
+        FAIL=1
+    elif [ $T6B_ELAPSED -lt $LO_BOUND ] || [ $T6B_ELAPSED -gt $HI_BOUND ]; then
+        echo "FAIL: Keep-alive timeout window (elapsed=${T6B_ELAPSED}s, expected ${LO_BOUND}-${HI_BOUND}s)"
+        FAIL=1
+    else
+        echo "PASS: Keep-alive timeout window (elapsed=${T6B_ELAPSED}s, expected ${LO_BOUND}-${HI_BOUND}s)"
+    fi
+fi
+fi # has_will
+
 fi # skip_plain (tests 1-6)
 
 # --- Test 7: Username/password auth ---
diff --git a/src/mqtt_broker.c b/src/mqtt_broker.c
@@ -35,7 +35,19 @@
 
 #ifdef WOLFMQTT_BROKER
 
-#define BROKER_FORCE_ZERO(mem, len) Mqtt_ForceZero(mem, (word32)(len))
+/* Secure memory zeroing - uses volatile pointer to prevent the compiler
+ * from optimizing away the stores (dead-store elimination). */
+static void MqttBroker_ForceZero(void* mem, word32 len)
+{
+    volatile byte* p = (volatile byte*)mem;
+    word32 i;
+    for (i = 0; i < len; i++) {
+        p[i] = 0;
+    }
+}
+
+#define BROKER_FORCE_ZERO(mem, len) \
+    MqttBroker_ForceZero(mem, (word32)(len))
 
 /* -------------------------------------------------------------------------- */
 /* Platform includes                                                           */
@@ -2701,6 +2713,13 @@ static int BrokerHandle_Connect(BrokerClient* bc, int rx_len,
         return rc;
     }
 
+#ifdef WOLFMQTT_V5
+    /* Initialize early so every `goto send_connack` below produces a CONNACK
+     * matching the client's protocol level (v5 CONNACK has a Properties
+     * length and uses v5 reason codes). */
+    ack.protocol_level = mc.protocol_level;
+#endif
+
     /* Store client ID */
 #ifdef WOLFMQTT_STATIC_MEMORY
     bc->client_id[0] = '\0';
@@ -2917,7 +2936,6 @@ static int BrokerHandle_Connect(BrokerClient* bc, int rx_len,
     ack.flags = 0;
     ack.return_code = MQTT_CONNECT_ACK_CODE_ACCEPTED;
 #ifdef WOLFMQTT_V5
-    ack.protocol_level = mc.protocol_level;
     ack.props = NULL;
 #endif
 
diff --git a/src/mqtt_client.c b/src/mqtt_client.c
@@ -26,7 +26,19 @@
 
 #include "wolfmqtt/mqtt_client.h"
 
-#define CLIENT_FORCE_ZERO(mem, len) Mqtt_ForceZero(mem, (word32)(len))
+/* Secure memory zeroing - uses volatile pointer to prevent the compiler
+ * from optimizing away the stores (dead-store elimination). */
+static void MqttClient_ForceZero(void* mem, word32 len)
+{
+    volatile byte* p = (volatile byte*)mem;
+    word32 i;
+    for (i = 0; i < len; i++) {
+        p[i] = 0;
+    }
+}
+
+#define CLIENT_FORCE_ZERO(mem, len) \
+    MqttClient_ForceZero(mem, (word32)(len))
 
 /* DOCUMENTED BUILD OPTIONS:
  *
@@ -3013,6 +3025,7 @@ int MqttClient_NetDisconnect(MqttClient *client)
 {
 #ifdef WOLFMQTT_MULTITHREAD
     MqttPendResp *tmpResp;
+    MqttPendResp *nextResp;
     int rc;
 #endif
 
@@ -3027,7 +3040,6 @@ int MqttClient_NetDisconnect(MqttClient *client)
     #ifdef WOLFMQTT_DEBUG_CLIENT
         PRINTF("Net Disconnect: Removing pending responses");
     #endif
-        MqttPendResp *nextResp;
         for (tmpResp = client->firstPendResp;
              tmpResp != NULL;
              tmpResp = nextResp) {
diff --git a/src/mqtt_packet.c b/src/mqtt_packet.c
@@ -801,8 +801,18 @@ int MqttEncode_Connect(byte *tx_buf, int tx_buf_len, MqttConnect *mc_connect)
     }
 #endif
 
-    remain_len += (int)XSTRLEN(mc_connect->client_id) + MQTT_DATA_LEN_SIZE;
+    /* MQTT UTF-8 strings are limited to 65535 bytes [MQTT-1.5.3]. Check here
+     * (before writing the fixed header) so a later MqttEncode_String failure
+     * cannot corrupt tx_payload via `tx_payload += -1`. */
+    {
+        size_t str_len = XSTRLEN(mc_connect->client_id);
+        if (str_len > (size_t)0xFFFF) {
+            return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
+        }
+        remain_len += (int)str_len + MQTT_DATA_LEN_SIZE;
+    }
     if (mc_connect->enable_lwt) {
+        size_t str_len;
         /* Verify all required fields are present */
         if (mc_connect->lwt_msg == NULL ||
             mc_connect->lwt_msg->topic_name == NULL ||
@@ -811,8 +821,12 @@ int MqttEncode_Connect(byte *tx_buf, int tx_buf_len, MqttConnect *mc_connect)
         {
             return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
         }
+        str_len = XSTRLEN(mc_connect->lwt_msg->topic_name);
+        if (str_len > (size_t)0xFFFF) {
+            return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
+        }
 
-        remain_len += (int)XSTRLEN(mc_connect->lwt_msg->topic_name);
+        remain_len += (int)str_len;
         remain_len += MQTT_DATA_LEN_SIZE;
         /* LWT payload uses word16 length prefix, validate it fits */
         if (mc_connect->lwt_msg->total_len > (word32)0xFFFF) {
@@ -841,10 +855,18 @@ int MqttEncode_Connect(byte *tx_buf, int tx_buf_len, MqttConnect *mc_connect)
 #endif
     }
     if (mc_connect->username) {
-        remain_len += (int)XSTRLEN(mc_connect->username) + MQTT_DATA_LEN_SIZE;
+        size_t str_len = XSTRLEN(mc_connect->username);
+        if (str_len > (size_t)0xFFFF) {
+            return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
+        }
+        remain_len += (int)str_len + MQTT_DATA_LEN_SIZE;
     }
     if (mc_connect->password) {
-        remain_len += (int)XSTRLEN(mc_connect->password) + MQTT_DATA_LEN_SIZE;
+        size_t str_len = XSTRLEN(mc_connect->password);
+        if (str_len > (size_t)0xFFFF) {
+            return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
+        }
+        remain_len += (int)str_len + MQTT_DATA_LEN_SIZE;
     }
 
     /* Encode fixed header */
@@ -1302,9 +1324,18 @@ int MqttEncode_Publish(byte *tx_buf, int tx_buf_len, MqttPublish *publish,
     if (tx_buf == NULL || publish == NULL) {
         return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
     }
+    /* MQTT UTF-8 strings are limited to 65535 bytes [MQTT-1.5.3]. Check here
+     * before writing the fixed header so a later MqttEncode_String failure
+     * cannot corrupt tx_payload via `tx_payload += -1`. */
+    {
+        size_t str_len = XSTRLEN(publish->topic_name);
+        if (str_len > (size_t)0xFFFF) {
+            return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
+        }
 
-    /* Determine packet length */
-    variable_len = (int)XSTRLEN(publish->topic_name) + MQTT_DATA_LEN_SIZE;
+        /* Determine packet length */
+        variable_len = (int)str_len + MQTT_DATA_LEN_SIZE;
+    }
     if (publish->qos > MQTT_QOS_0) {
         if (publish->packet_id == 0) {
             return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PACKET_ID);
@@ -1721,7 +1752,12 @@ int MqttEncode_Subscribe(byte *tx_buf, int tx_buf_len,
     for (i = 0; i < subscribe->topic_count; i++) {
         topic = &subscribe->topics[i];
         if ((topic != NULL) && (topic->topic_filter != NULL)) {
-            remain_len += (int)XSTRLEN(topic->topic_filter) + MQTT_DATA_LEN_SIZE;
+            /* MQTT UTF-8 strings are limited to 65535 bytes [MQTT-1.5.3] */
+            size_t str_len = XSTRLEN(topic->topic_filter);
+            if (str_len > (size_t)0xFFFF) {
+                return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
+            }
+            remain_len += (int)str_len + MQTT_DATA_LEN_SIZE;
             remain_len++; /* For QoS */
         }
         else {
@@ -2016,8 +2052,12 @@ int MqttEncode_Unsubscribe(byte *tx_buf, int tx_buf_len,
     for (i = 0; i < unsubscribe->topic_count; i++) {
         topic = &unsubscribe->topics[i];
         if ((topic != NULL) && (topic->topic_filter != NULL)) {
-            remain_len += (int)XSTRLEN(topic->topic_filter) +
-                                MQTT_DATA_LEN_SIZE;
+            /* MQTT UTF-8 strings are limited to 65535 bytes [MQTT-1.5.3] */
+            size_t str_len = XSTRLEN(topic->topic_filter);
+            if (str_len > (size_t)0xFFFF) {
+                return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
+            }
+            remain_len += (int)str_len + MQTT_DATA_LEN_SIZE;
         }
         else {
             /* Topic count is invalid */
@@ -2675,7 +2715,8 @@ int MqttDecode_Auth(byte *rx_buf, int rx_buf_len, MqttAuth *auth)
         /* Decode variable header */
         auth->reason_code = *rx_payload++;
         if ((auth->reason_code == MQTT_REASON_SUCCESS) ||
-            (auth->reason_code == MQTT_REASON_CONT_AUTH))
+            (auth->reason_code == MQTT_REASON_CONT_AUTH) ||
+            (auth->reason_code == MQTT_REASON_REAUTH))
         {
             /* Decode Length of Properties */
             if (rx_buf_len < (rx_payload - rx_buf)) {
@@ -2700,28 +2741,29 @@ int MqttDecode_Auth(byte *rx_buf, int rx_buf_len, MqttAuth *auth)
                     if (tmp < 0)
                         return tmp;
                     rx_payload += tmp;
-            }
-            else if (auth->reason_code != MQTT_REASON_SUCCESS) {
-                /* The Reason Code and Property Length can be omitted if the
-                   Reason Code is 0x00 (Success) and there are no Properties.
-                   In this case the AUTH has a Remaining Length of 0. */
-                return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA);
-            }
-            if (auth->props != NULL) {
-                /* Must have Authentication Method */
+                }
+                else if (auth->reason_code != MQTT_REASON_SUCCESS) {
+                    /* The Reason Code and Property Length can be omitted if
+                       the Reason Code is 0x00 (Success) and there are no
+                       Properties. In this case the AUTH has a Remaining
+                       Length of 0. */
+                    return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA);
+                }
+                if (auth->props != NULL) {
+                    /* Must have Authentication Method */
 
-                /* Must have Authentication Data */
+                    /* Must have Authentication Data */
 
-                /* May have zero or more User Property pairs */
+                    /* May have zero or more User Property pairs */
+                }
+                else {
+                    return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA);
+                }
             }
             else {
                 return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA);
             }
         }
-        else {
-            return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER);
-        }
-        }
         else {
             return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA);
         }
diff --git a/tests/fuzz/gen_corpus.py b/tests/fuzz/gen_corpus.py
@@ -180,6 +180,11 @@ def main():
                connect + gen_subscribe("test/#"))
     write_seed("seq_connect_subscribe_wildcard.bin",
                connect + gen_subscribe("+/data/#"))
+    # SUBSCRIBE options byte with QoS bits = 0b11 (reserved). Exercises the
+    # BrokerHandle_Subscribe cap that clamps surfaced QoS=3 down to QoS 2
+    # before BrokerSubs_Add / SUBACK.
+    write_seed("seq_connect_subscribe_qos3.bin",
+               connect + gen_subscribe("test/#", qos=3))
     write_seed("seq_connect_unsubscribe.bin",
                connect + gen_unsubscribe("test/#"))
     write_seed("seq_connect_pingreq.bin",
diff --git a/tests/test_mqtt_client.c b/tests/test_mqtt_client.c
diff --git a/tests/test_mqtt_packet.c b/tests/test_mqtt_packet.c
diff --git a/wolfmqtt/mqtt_types.h b/wolfmqtt/mqtt_types.h
