Fixes from review

Author: embhorn

src/mqtt_broker.c

@@ -3393,11 +3393,6 @@ static int BrokerHandle_PublishRel(BrokerClient* bc, int rx_len)
             (int)bc->sock, resp.packet_id);
         rc = MqttPacket_Write(&bc->client, bc->tx_buf, rc);
     }
-#ifdef WOLFMQTT_V5
-    if (resp.props) {
-        (void)MqttProps_Free(resp.props);
-    }
-#endif
     return rc;
 }
 
@@ -3434,11 +3429,6 @@ static int BrokerHandle_PublishRec(BrokerClient* bc, int rx_len)
             (int)bc->sock, resp.packet_id);
         rc = MqttPacket_Write(&bc->client, bc->tx_buf, rc);
     }
-#ifdef WOLFMQTT_V5
-    if (resp.props) {
-        (void)MqttProps_Free(resp.props);
-    }
-#endif
     return rc;
 }
 
@@ -3875,6 +3865,25 @@ int MqttBroker_Start(MqttBroker* broker)
 
 #ifdef WOLFMQTT_BROKER_AUTH
     if (broker->auth_user || broker->auth_pass) {
+        /* Reject configured credentials that would be silently rejected
+         * by BrokerStrCompare's cmp_len guard. Catching this at startup
+         * avoids a confusing state where every client auth fails. */
+        if (broker->auth_user &&
+                XSTRLEN(broker->auth_user) >= BROKER_MAX_USERNAME_LEN) {
+            WBLOG_ERR(broker,
+                "broker: auth_user length %u >= BROKER_MAX_USERNAME_LEN (%d)",
+                (unsigned)XSTRLEN(broker->auth_user),
+                BROKER_MAX_USERNAME_LEN);
+            return MQTT_CODE_ERROR_BAD_ARG;
+        }
+        if (broker->auth_pass &&
+                XSTRLEN(broker->auth_pass) >= BROKER_MAX_PASSWORD_LEN) {
+            WBLOG_ERR(broker,
+                "broker: auth_pass length %u >= BROKER_MAX_PASSWORD_LEN (%d)",
+                (unsigned)XSTRLEN(broker->auth_pass),
+                BROKER_MAX_PASSWORD_LEN);
+            return MQTT_CODE_ERROR_BAD_ARG;
+        }
         WBLOG_INFO(broker, "broker: auth enabled user=%s",
             broker->auth_user ? broker->auth_user : "(null)");
     #ifdef ENABLE_MQTT_TLS
@@ -4187,7 +4196,7 @@ int wolfmqtt_broker(int argc, char** argv)
             if (rc == MQTT_CODE_CONTINUE) {
                 BROKER_SLEEP_MS(10);
             }
-            else if (rc < 0 && rc != MQTT_CODE_CONTINUE) {
+            else if (rc < 0) {
                 break;
             }
         }

src/mqtt_client.c

@@ -1725,6 +1725,10 @@ int MqttClient_Connect(MqttClient *client, MqttConnect *mc_connect)
             MQTT_PACKET_TYPE_CONNECT, 0, 0);
     #endif
         if (rc <= 0) {
+            /* Encode failed: tx_buf may hold partial plaintext credentials.
+             * Zero the full buffer before MqttWriteStop releases lockSend
+             * so no other thread can see residual data. */
+            CLIENT_FORCE_ZERO(client->tx_buf, client->tx_buf_len);
             MqttWriteStop(client, &mc_connect->stat);
             return rc;
         }
@@ -1741,9 +1745,11 @@ int MqttClient_Connect(MqttClient *client, MqttConnect *mc_connect)
         if (rc != 0) {
             /* Save write.len before MqttWriteStop zeroes client->write */
             int xfer = client->write.len;
-            MqttWriteStop(client, &mc_connect->stat);
-            /* Clear tx_buf to remove plaintext credentials before returning */
+            /* Clear tx_buf to remove plaintext credentials BEFORE
+             * MqttWriteStop releases lockSend, so another thread cannot
+             * race in and repopulate tx_buf before it is scrubbed. */
             CLIENT_FORCE_ZERO(client->tx_buf, xfer);
+            MqttWriteStop(client, &mc_connect->stat);
             return rc; /* Error locking client */
         }
     #endif
@@ -1766,11 +1772,12 @@ int MqttClient_Connect(MqttClient *client, MqttConnect *mc_connect)
             return rc;
         }
     #endif
-        MqttWriteStop(client, &mc_connect->stat);
-
-        /* Clear tx_buf to remove any plaintext credentials from memory.
-         * Use xfer (saved before MqttWriteStop zeroes client->write) */
+        /* Clear tx_buf to remove any plaintext credentials from memory
+         * BEFORE MqttWriteStop releases lockSend, so another thread cannot
+         * race in and populate tx_buf before it is scrubbed.
+         * Use xfer (saved before MqttWriteStop zeroes client->write). */
         CLIENT_FORCE_ZERO(client->tx_buf, xfer);
+        MqttWriteStop(client, &mc_connect->stat);
 
         if (rc != xfer) {
             MqttClient_CancelMessage(client, (MqttObject*)mc_connect);
@@ -2750,6 +2757,10 @@ int MqttClient_Auth(MqttClient *client, MqttAuth* auth)
             MQTT_PACKET_TYPE_AUTH, 0, 0);
     #endif
         if (rc <= 0) {
+            /* Encode failed: tx_buf may hold partial SASL auth data.
+             * Zero the full buffer before MqttWriteStop releases lockSend
+             * so no other thread can see residual data. */
+            CLIENT_FORCE_ZERO(client->tx_buf, client->tx_buf_len);
             MqttWriteStop(client, &auth->stat);
             return rc;
         }
@@ -2766,9 +2777,11 @@ int MqttClient_Auth(MqttClient *client, MqttAuth* auth)
         if (rc != 0) {
             /* Save write.len before MqttWriteStop zeroes client->write */
             int xfer = client->write.len;
-            MqttWriteStop(client, &auth->stat);
-            /* Clear tx_buf to remove SASL auth data before returning */
+            /* Clear tx_buf to remove SASL auth data BEFORE MqttWriteStop
+             * releases lockSend, to prevent a racing thread from
+             * repopulating tx_buf before it is scrubbed. */
             CLIENT_FORCE_ZERO(client->tx_buf, xfer);
+            MqttWriteStop(client, &auth->stat);
             return rc; /* Error locking client */
         }
     #endif
@@ -2790,11 +2803,12 @@ int MqttClient_Auth(MqttClient *client, MqttAuth* auth)
             return rc;
         }
     #endif
-        MqttWriteStop(client, &auth->stat);
-
-        /* Clear tx_buf to remove any SASL auth data from memory.
-         * Use xfer (saved before MqttWriteStop zeroes client->write) */
+        /* Clear tx_buf to remove any SASL auth data from memory BEFORE
+         * MqttWriteStop releases lockSend, to prevent a racing thread
+         * from populating tx_buf before it is scrubbed.
+         * Use xfer (saved before MqttWriteStop zeroes client->write). */
         CLIENT_FORCE_ZERO(client->tx_buf, xfer);
+        MqttWriteStop(client, &auth->stat);
 
         if (rc != xfer) {
             MqttClient_CancelMessage(client, (MqttObject*)auth);