Merge branch 'fix-esp-mqtt-client-set-uri' into 'master'

MR: fix: Fixed uri inconsistency when updated
See merge request espressif/esp-mqtt!306

mqtt_client.c

@@ -1075,38 +1075,35 @@ esp_err_t esp_mqtt_client_set_uri(esp_mqtt_client_handle_t client, const char *u
         return ESP_FAIL;
     }
 
-    // This API could be also executed when client is active (need to protect config fields)
-    MQTT_API_LOCK(client);
-    // set uri overrides actual scheme, host, path if configured previously
-// False-positive leak detection. TODO: GCC-366
-#pragma GCC diagnostic push  // TODO: IDF-10105
-#if __clang__
-#pragma clang diagnostic ignored "-Wunknown-warning-option"
-#else
-#pragma GCC diagnostic ignored "-Wpragmas"
-#endif
-#pragma GCC diagnostic ignored "-Wanalyzer-malloc-leak"
-    free(client->config->scheme);
-    free(client->config->host);
-    free(client->config->path);
-    client->config->scheme = mqtt_create_string(uri + puri.field_data[UF_SCHEMA].off, puri.field_data[UF_SCHEMA].len);
-    client->config->host = mqtt_create_string(uri + puri.field_data[UF_HOST].off, puri.field_data[UF_HOST].len);
-    client->config->path = NULL;
-#pragma GCC diagnostic pop
+    char *new_scheme = mqtt_create_string(uri + puri.field_data[UF_SCHEMA].off, puri.field_data[UF_SCHEMA].len);
+    char *new_host = mqtt_create_string(uri + puri.field_data[UF_HOST].off, puri.field_data[UF_HOST].len);
+    char *new_path = NULL;
+    char *new_password = NULL;
+    char *new_username = NULL;
+    char *user_info = NULL;
+
+    if (!new_scheme || !new_host) {
+        ESP_LOGE(TAG, "%s(%d): %s", __FUNCTION__, __LINE__, "Memory exhausted");
+        free(new_scheme);
+        free(new_host);
+        return ESP_ERR_NO_MEM;
+    }
+
+    esp_err_t ret = ESP_OK;
 
     if (puri.field_data[UF_PATH].len || puri.field_data[UF_QUERY].len) {
         int asprintf_ret_value;
 
         if (puri.field_data[UF_QUERY].len == 0) {
-            asprintf_ret_value = asprintf(&client->config->path,
+            asprintf_ret_value = asprintf(&new_path,
                                           "%.*s",
                                           puri.field_data[UF_PATH].len, uri + puri.field_data[UF_PATH].off);
         } else if (puri.field_data[UF_PATH].len == 0)  {
-            asprintf_ret_value = asprintf(&client->config->path,
+            asprintf_ret_value = asprintf(&new_path,
                                           "/?%.*s",
                                           puri.field_data[UF_QUERY].len, uri + puri.field_data[UF_QUERY].off);
         } else {
-            asprintf_ret_value = asprintf(&client->config->path,
+            asprintf_ret_value = asprintf(&new_path,
                                           "%.*s?%.*s",
                                           puri.field_data[UF_PATH].len, uri + puri.field_data[UF_PATH].off,
                                           puri.field_data[UF_QUERY].len, uri + puri.field_data[UF_QUERY].off);
@@ -1114,44 +1111,92 @@ esp_err_t esp_mqtt_client_set_uri(esp_mqtt_client_handle_t client, const char *u
 
         if (asprintf_ret_value == -1) {
             ESP_LOGE(TAG, "%s(%d): %s", __FUNCTION__, __LINE__, "Memory exhausted");
-            MQTT_API_UNLOCK(client);
-            return ESP_ERR_NO_MEM;
+            ret = ESP_ERR_NO_MEM;
+            goto cleanup;
         }
     }
 
-    if (puri.field_data[UF_PORT].len) {
-        client->config->port = strtol((const char *)(uri + puri.field_data[UF_PORT].off), NULL, 10);
-    }
-
-    char *user_info = mqtt_create_string(uri + puri.field_data[UF_USERINFO].off, puri.field_data[UF_USERINFO].len);
+    user_info = mqtt_create_string(uri + puri.field_data[UF_USERINFO].off, puri.field_data[UF_USERINFO].len);
 
     if (user_info) {
         char *pass = strchr(user_info, ':');
 
         if (pass) {
-            pass[0] = 0; //terminal username
+            pass[0] = 0;
             pass++;
 
-            // parse %-encoded symbols such as %40 = @ in the password
             if (esp_mqtt_decode_percent_encoded_string(pass) < 0) {
                 ESP_LOGE(TAG, "Error parse uri (non-hexadecimal data after %%) = %s", uri);
-                return ESP_FAIL;
+                ret = ESP_FAIL;
+                goto cleanup;
             }
 
-            client->mqtt_state.connection.information.password = strdup(pass);
+            new_password = strdup(pass);
         }
 
         if (esp_mqtt_decode_percent_encoded_string(user_info) < 0) {
             ESP_LOGE(TAG, "Error parse uri (non-hexadecimal data after %%) = %s", uri);
-            return ESP_FAIL;
+            ret = ESP_FAIL;
+            goto cleanup;
         }
 
-        client->mqtt_state.connection.information.username = strdup(user_info);
-        free(user_info);
+        new_username = strdup(user_info);
     }
 
+    MQTT_API_LOCK(client);
+
+    // uri may alias client->config->uri (internal call from esp_mqtt_set_config).
+    // In that case skip the redundant strdup; otherwise copy before freeing.
+    if (uri != client->config->uri) {
+        char *new_uri = strdup(uri);
+
+        if (!new_uri) {
+            ESP_LOGE(TAG, "%s(%d): %s", __FUNCTION__, __LINE__, "Memory exhausted");
+            ret = ESP_ERR_NO_MEM;
             MQTT_API_UNLOCK(client);
-    return ESP_OK;
+            goto cleanup;
+        }
+
+        free(client->config->uri);
+        client->config->uri = new_uri;
+    }
+
+    free(client->config->scheme);
+    client->config->scheme = new_scheme;
+    new_scheme = NULL;
+    free(client->config->host);
+    client->config->host = new_host;
+    new_host = NULL;
+    free(client->config->path);
+    client->config->path = new_path;
+    new_path = NULL;
+
+    if (puri.field_data[UF_PORT].len) {
+        client->config->port = strtol((const char *)(uri + puri.field_data[UF_PORT].off), NULL, 10);
+    }
+
+    if (new_username) {
+        free(client->mqtt_state.connection.information.username);
+        client->mqtt_state.connection.information.username = new_username;
+        new_username = NULL;
+    }
+
+    if (new_password) {
+        free(client->mqtt_state.connection.information.password);
+        client->mqtt_state.connection.information.password = new_password;
+        new_password = NULL;
+    }
+
+    ESP_LOGD(TAG, "config_uri=%s", client->config->uri);
+    MQTT_API_UNLOCK(client);
+cleanup:
+    free(new_scheme);
+    free(new_host);
+    free(new_path);
+    free(new_username);
+    free(new_password);
+    free(user_info);
+    return ret;
 }
 
 static esp_err_t esp_mqtt_dispatch_event_with_msgid(esp_mqtt_client_handle_t client)

test/host/main/CMakeLists.txt

@@ -2,14 +2,16 @@ idf_component_register(SRCS  "test_mqtt_client.cpp" "test_log_intercept.cpp" "te
                        REQUIRES cmock mqtt esp_timer esp_hw_support http_parser log
                        WHOLE_ARCHIVE)
 
-target_compile_options(${COMPONENT_LIB} PUBLIC -fsanitize=address -Wno-missing-field-initializers)
-target_link_options(${COMPONENT_LIB} PUBLIC -fsanitize=address)
+set(SANITIZER_FLAGS -fsanitize=address,undefined -fno-sanitize-recover=undefined)
+
+target_compile_options(${COMPONENT_LIB} PUBLIC ${SANITIZER_FLAGS} -Wno-missing-field-initializers)
+target_link_options(${COMPONENT_LIB} PUBLIC ${SANITIZER_FLAGS})
 target_link_libraries(${COMPONENT_LIB} PUBLIC Catch2::Catch2WithMain rapidcheck rapidcheck_catch)
 
 idf_component_get_property(mqtt mqtt COMPONENT_LIB)
 target_compile_definitions(${mqtt} PRIVATE SOC_WIFI_SUPPORTED=1)
-target_compile_options(${mqtt} PUBLIC -fsanitize=address)
-target_link_options(${mqtt} PUBLIC -fsanitize=address)
+target_compile_options(${mqtt} PUBLIC ${SANITIZER_FLAGS})
+target_link_options(${mqtt} PUBLIC ${SANITIZER_FLAGS})
 
 if(CONFIG_GCOV_ENABLED)
     target_compile_options(${COMPONENT_LIB} PUBLIC --coverage -fprofile-arcs -ftest-coverage)

test/host/main/test_log_matchers.cpp

@@ -9,6 +9,7 @@
  */
 
 #include "test_log_matchers.hpp"
+#include <algorithm>
 #include <format>
 #include <span>
 #include <string_view>
@@ -17,6 +18,23 @@
 namespace test::esp_log::matchers
 {
 
+static const char *level_name(esp_log_level_t level)
+{
+    switch (level) {
+    case ESP_LOG_ERROR: return "ERROR";
+
+    case ESP_LOG_WARN:  return "WARN";
+
+    case ESP_LOG_INFO:  return "INFO";
+
+    case ESP_LOG_DEBUG: return "DEBUG";
+
+    case ESP_LOG_VERBOSE: return "VERBOSE";
+
+    default: return "?";
+    }
+}
+
 std::string ContainsMessage::describe() const
 {
     return std::format(R"(contains message "{}")", expected_message);
@@ -28,6 +46,22 @@ std::string ContainsMessageWithTag::describe() const
     expected_message, expected_tag);
 }
 
+bool ContainsMessageAtLevel::match(const Capture &captured_log) const
+{
+    return std::any_of(captured_log.entries().begin(), captured_log.entries().end(),
+    [&](const Entry & e) {
+        return e.level == expected_level &&
+               e.tag == expected_tag &&
+               e.message.find(expected_message) != std::string::npos;
+    });
+}
+
+std::string ContainsMessageAtLevel::describe() const
+{
+    return std::format(R"(contains {} message "{}" with tag "{}")",
+    level_name(expected_level), expected_message, expected_tag);
+}
+
 bool LogsInOrder::match(const Capture &captured_log) const
 {
     std::vector<std::string_view> views;

test/host/main/test_log_matchers.hpp

@@ -57,6 +57,24 @@ private:
     std::string expected_message;
 };
 
+class ContainsMessageAtLevel : public Catch::Matchers::MatcherGenericBase
+{
+public:
+    ContainsMessageAtLevel(esp_log_level_t expected_level, std::string_view expected_tag,
+                           std::string_view expected_message)
+        : expected_level(expected_level), expected_tag(expected_tag),
+          expected_message(expected_message) {}
+
+    bool match(const Capture &captured_log) const;
+
+    std::string describe() const override;
+
+private:
+    esp_log_level_t expected_level;
+    std::string expected_tag;
+    std::string expected_message;
+};
+
 class LogsInOrder : public Catch::Matchers::MatcherGenericBase
 {
 public:
@@ -87,6 +105,18 @@ inline ContainsMessageWithTag HasMessageIn(std::string_view expected_tag,
     return ContainsMessageWithTag{expected_tag, expected_message};
 }
 
+inline ContainsMessageAtLevel HasErrorIn(std::string_view expected_tag,
+                                         std::string_view expected_message)
+{
+    return ContainsMessageAtLevel{ESP_LOG_ERROR, expected_tag, expected_message};
+}
+
+inline ContainsMessageAtLevel HasWarnIn(std::string_view expected_tag,
+                                        std::string_view expected_message)
+{
+    return ContainsMessageAtLevel{ESP_LOG_WARN, expected_tag, expected_message};
+}
+
 inline LogsInOrder LogsInOrderAny(std::initializer_list<std::string_view> expected_messages)
 {
     return LogsInOrder{expected_messages};

test/host/main/test_mqtt_client.cpp

@@ -3,24 +3,23 @@
  *
  * SPDX-License-Identifier: Apache-2.0
  */
-#include <algorithm>
+#include "esp_transport.h"
+#include <array>
+#include <catch2/catch_test_macros.hpp>
+#include <catch2/matchers/catch_matchers.hpp>
+#include <cstddef>
 #include <exception>
 #include <memory>
 #include <net/if.h>
-#include <random>
+#include <rapidcheck.h>
 #include <string>
 #include <string_view>
 #include <type_traits>
-#include "esp_transport.h"
-#include <catch2/catch_test_macros.hpp>
-#include <catch2/matchers/catch_matchers.hpp>
-#include <rapidcheck.h>
 
 #include "mqtt_client.h"
 #include "test_log_intercept.hpp"
 #include "test_log_matchers.hpp"
 extern "C" {
-#include "esp_log.h"
 #include "Mockesp_event.h"
 #include "Mockesp_transport.h"
 #include "Mockesp_transport_ssl.h"
@@ -29,6 +28,7 @@ extern "C" {
 #include "Mockevent_groups.h"
 #include "Mockqueue.h"
 #include "Mocktask.h"
+#include "esp_log.h"
 #if __has_include("Mockidf_additions.h")
     /* Some functions were moved from "task.h" to "idf_additions.h" */
 #include "Mockidf_additions.h"
@@ -38,22 +38,49 @@ extern "C" {
      * The following functions are not directly called but the generation of them
      * from cmock is broken, so we need to define them here.
      */
-    esp_err_t esp_tls_get_and_clear_last_error(esp_tls_error_handle_t h, int *esp_tls_code, int *esp_tls_flags)
+    esp_err_t esp_tls_get_and_clear_last_error(esp_tls_error_handle_t h,
+                                               int *esp_tls_code,
+                                               int *esp_tls_flags)
     {
         return ESP_OK;
     }
 }
 
-auto random_string(std::size_t n)
+static std::string build_uri(std::string_view scheme, std::string_view host,
+                             const uint16_t *port, std::string_view path = {},
+                             std::string_view query = {},
+                             std::string_view user_info = {})
 {
-    static constexpr std::string_view char_set = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ123456790";
-    std::string str;
-    std::sample(char_set.begin(), char_set.end(), std::back_inserter(str), n,
-                std::mt19937 {std::random_device{}()});
-    return str;
+    std::string uri{scheme};
+    uri += "://";
+
+    if (!user_info.empty()) {
+        uri += user_info;
+        uri += "@";
     }
 
-using unique_mqtt_client = std::unique_ptr < std::remove_pointer_t<esp_mqtt_client_handle_t>,
+    uri += host;
+
+    if (port) {
+        uri += ":";
+        uri += std::to_string(*port);
+    }
+
+    if (!path.empty()) {
+        uri += "/";
+        uri += path;
+    }
+
+    if (!query.empty()) {
+        uri += "?";
+        uri += query;
+    }
+
+    return uri;
+}
+
+using unique_mqtt_client =
+    std::unique_ptr < std::remove_pointer_t<esp_mqtt_client_handle_t>,
     decltype([](esp_mqtt_client_handle_t client)
 {
     esp_mqtt_client_destroy(client);
@@ -71,11 +98,16 @@ SCENARIO("MQTT Client Operation")
     xQueueGiveMutexRecursive_IgnoreAndReturn(true);
     xQueueCreateMutex_ExpectAnyArgsAndReturn(
         reinterpret_cast<QueueHandle_t>(&mtx));
-    xEventGroupCreate_IgnoreAndReturn(reinterpret_cast<EventGroupHandle_t>(&event_group));
-    esp_transport_list_init_IgnoreAndReturn(reinterpret_cast<esp_transport_list_handle_t>(&transport_list));
-    esp_transport_tcp_init_IgnoreAndReturn(reinterpret_cast<esp_transport_handle_t>(&transport));
-    esp_transport_ssl_init_IgnoreAndReturn(reinterpret_cast<esp_transport_handle_t>(&transport));
-    esp_transport_ws_init_IgnoreAndReturn(reinterpret_cast<esp_transport_handle_t>(&transport));
+    xEventGroupCreate_IgnoreAndReturn(
+        reinterpret_cast<EventGroupHandle_t>(&event_group));
+    esp_transport_list_init_IgnoreAndReturn(
+        reinterpret_cast<esp_transport_list_handle_t>(&transport_list));
+    esp_transport_tcp_init_IgnoreAndReturn(
+        reinterpret_cast<esp_transport_handle_t>(&transport));
+    esp_transport_ssl_init_IgnoreAndReturn(
+        reinterpret_cast<esp_transport_handle_t>(&transport));
+    esp_transport_ws_init_IgnoreAndReturn(
+        reinterpret_cast<esp_transport_handle_t>(&transport));
     esp_transport_ws_set_subprotocol_IgnoreAndReturn(ESP_OK);
     esp_transport_list_add_IgnoreAndReturn(ESP_OK);
     esp_transport_set_default_port_IgnoreAndReturn(ESP_OK);
@@ -85,6 +117,11 @@ SCENARIO("MQTT Client Operation")
     vEventGroupDelete_Ignore();
     vQueueDelete_Ignore();
     GIVEN("An a minimal config") {
+        static constexpr std::array<char, 36> char_set = {
+            'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n',
+            'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', '0', '1',
+            '2', '3', '4', '5', '6', '7', '8', '9',
+        };
         esp_mqtt_client_config_t config{};
         config.broker.address.uri = "mqtt://1.1.1.1";
         xTaskCreatePinnedToCore_ExpectAnyArgsAndReturn(pdTRUE);
@@ -92,53 +129,182 @@ SCENARIO("MQTT Client Operation")
             auto client = unique_mqtt_client{esp_mqtt_client_init(&config)};
             REQUIRE(client != nullptr);
             SECTION("User will set a new uri") {
+                using namespace test::esp_log::matchers;
+                esp_log_level_set("mqtt_client", ESP_LOG_DEBUG);
+                test::esp_log::Capture log;
                 SECTION("User set a correct URI") {
-                    auto res = esp_mqtt_client_set_uri(client.get(), "mqtt://example.com/valid");
-                    REQUIRE(res == ESP_OK);
+                    REQUIRE(esp_mqtt_client_set_uri(
+                                client.get(), "mqtt://example.com/valid") == ESP_OK);
+                    REQUIRE_THAT(log, HasMessageIn("mqtt_client",
+                                                   "config_uri=mqtt://example.com/valid"));
                 }
-                SECTION("Incorrect URI from user") {
-                    auto res = esp_mqtt_client_set_uri(client.get(), "invalid string that is not a url");
-                    REQUIRE(res == ESP_FAIL);
+                SECTION("Incorrect URI from user leaves config unchanged") {
+                    REQUIRE(esp_mqtt_client_set_uri(
+                                client.get(),
+                                "invalid string that is not a url") == ESP_FAIL);
+                    REQUIRE_THAT(log, HasErrorIn("mqtt_client",
+                                                 "Error parse uri"));
+                    REQUIRE(esp_mqtt_client_set_uri(client.get(),
+                                                    "mqtt://1.1.1.1") == ESP_OK);
+                    REQUIRE_THAT(log, HasMessageIn("mqtt_client",
+                                                   "config_uri=mqtt://1.1.1.1"));
+                }
+                SECTION("set_uri stores complete URI with all components") {
+                    const char *full_uri =
+                        "mqtts://user:pass@broker.local:8883/topic?opt=1";
+                    REQUIRE(esp_mqtt_client_set_uri(client.get(),
+                                                    full_uri) == ESP_OK);
+                    REQUIRE_THAT(log, HasMessageIn("mqtt_client",
+                                                   std::string{"config_uri="} + full_uri));
+                }
+                SECTION("set_uri value survives round-trip through set_config "
+                        "without uri") {
+                    static constexpr std::array schemes = {"mqtt", "mqtts",
+                                                           "ws", "wss"
+                                                          };
+                    rc::check("set_uri then set_config(no uri) preserves URI",
+                    [&] {
+                        auto char_gen = rc::gen::elementOf(char_set);
+                        auto scheme = *rc::gen::elementOf(schemes);
+                        auto host = *rc::gen::container<std::string>(
+                            *rc::gen::inRange<int>(1, 20), char_gen).as("host");
+                        auto path = *rc::gen::container<std::string>(
+                            *rc::gen::inRange<int>(0, 10), char_gen).as("path");
+                        auto port = *rc::gen::maybe(
+                            rc::gen::inRange<uint16_t>(1, 65535)).as("port");
+                        std::string uri = build_uri(scheme, host,
+                                                    port ? &*port : nullptr, path);
+
+                        log.clear();
+                        RC_ASSERT(esp_mqtt_client_set_uri(client.get(),
+                                                          uri.c_str()) == ESP_OK);
+                        RC_ASSERT(log.contains("mqtt_client",
+                        std::string{"config_uri="} + uri));
+
+                        // set_config without a uri must not revert the stored URI
+                        esp_mqtt_client_config_t no_uri_cfg{};
+                        no_uri_cfg.credentials.client_id = "round-trip-id";
+                        RC_ASSERT(esp_mqtt_set_config(client.get(),
+                                                      &no_uri_cfg) == ESP_OK);
+
+                        log.clear();
+                        RC_ASSERT(esp_mqtt_client_set_uri(client.get(),
+                                                          uri.c_str()) == ESP_OK);
+                        RC_ASSERT(log.contains("mqtt_client",
+                                               std::string{"config_uri="} + uri));
+                    });
+                }
+                SECTION("set_config with new uri replaces the previous set_uri "
+                        "value") {
+                    static constexpr std::array schemes = {"mqtt", "mqtts",
+                                                           "ws", "wss"
+                                                          };
+                    rc::check("set_config with URI overrides previous set_uri",
+                    [&] {
+                        auto char_gen = rc::gen::elementOf(char_set);
+                        auto scheme1 = *rc::gen::elementOf(schemes);
+                        auto host1 = *rc::gen::container<std::string>(
+                            *rc::gen::inRange<int>(1, 16), char_gen).as("host1");
+                        auto scheme2 = *rc::gen::elementOf(schemes);
+                        auto host2 = *rc::gen::container<std::string>(
+                            *rc::gen::inRange<int>(1, 16), char_gen).as("host2");
+                        std::string first = build_uri(scheme1, host1, nullptr);
+                        std::string second = build_uri(scheme2, host2, nullptr);
+                        RC_PRE(first != second);
+
+                        RC_ASSERT(esp_mqtt_client_set_uri(client.get(),
+                                                          first.c_str()) == ESP_OK);
+                        log.clear();
+                        esp_mqtt_client_config_t new_cfg{};
+                        new_cfg.broker.address.uri = second.c_str();
+                        RC_ASSERT(esp_mqtt_set_config(client.get(),
+                                                      &new_cfg) == ESP_OK);
+                        RC_ASSERT(log.contains("mqtt_client",
+                                               std::string{"config_uri="} + second));
+                        RC_ASSERT(!log.contains("mqtt_client",
+                                                std::string{"config_uri="} + first));
+                    });
+                }
+                SECTION("set_config called repeatedly is safe") {
+                    REQUIRE(esp_mqtt_set_config(client.get(), &config) == ESP_OK);
+                    REQUIRE(esp_mqtt_set_config(client.get(), &config) == ESP_OK);
+                    REQUIRE_THAT(log, HasMessageIn("mqtt_client",
+                                                   "config_uri=mqtt://1.1.1.1"));
+                }
+                SECTION("URI with percent-encoded user_info is accepted") {
+                    REQUIRE(esp_mqtt_client_set_uri(
+                                client.get(),
+                                "mqtt://user:p%40ss@broker.local") == ESP_OK);
+                }
+                SECTION("URI with invalid percent-encoded password returns "
+                        "ESP_FAIL and leaves config unchanged") {
+                    REQUIRE(esp_mqtt_client_set_uri(client.get(),
+                                                    "mqtt://changed.host") == ESP_OK);
+                    log.clear();
+                    REQUIRE(esp_mqtt_client_set_uri(
+                                client.get(),
+                                "mqtt://user:p%ZZ@broker.local") == ESP_FAIL);
+                    // The failed call must not have modified config
+                    REQUIRE_FALSE(log.contains("mqtt_client",
+                                               "config_uri=mqtt://user"));
+                    // Client remains usable (lock not leaked)
+                    REQUIRE(esp_mqtt_client_set_uri(
+                                client.get(),
+                                "mqtt://recovery.broker") == ESP_OK);
+                    REQUIRE_THAT(log, HasMessageIn("mqtt_client",
+                                                   "config_uri=mqtt://recovery.broker"));
+                }
+                SECTION("URI with invalid percent-encoded username returns "
+                        "ESP_FAIL and leaves config unchanged") {
+                    REQUIRE(esp_mqtt_client_set_uri(client.get(),
+                                                    "mqtt://changed.host") == ESP_OK);
+                    log.clear();
+                    REQUIRE(esp_mqtt_client_set_uri(
+                                client.get(),
+                                "mqtt://us%XYer:pass@broker.local") == ESP_FAIL);
+                    REQUIRE_FALSE(log.contains("mqtt_client",
+                                               "config_uri=mqtt://us"));
+                    REQUIRE(esp_mqtt_client_set_uri(
+                                client.get(),
+                                "mqtt://recovery2.broker") == ESP_OK);
+                    REQUIRE_THAT(log, HasMessageIn("mqtt_client",
+                                                   "config_uri=mqtt://recovery2.broker"));
                 }
             }
             SECTION("Any well-formed URI is accepted") {
-                static constexpr std::array<const char *, 4> schemes = {
-                    "mqtt", "mqtts", "ws", "wss"
+                esp_log_level_set("mqtt_client", ESP_LOG_DEBUG);
+                static constexpr std::array schemes = {"mqtt", "mqtts",
+                                                       "ws", "wss"
                                                       };
-                auto host_char = rc::gen::element('a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i',
-                                                  'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r',
-                                                  's', 't', 'u', 'v', 'w', 'x', 'y', 'z',
-                                                  '0', '1', '2', '3', '4', '5', '6', '7', '8', '9');
-                rc::check("esp_mqtt_client_set_uri accepts well-formed URIs",
-                [&] {
+                test::esp_log::Capture log;
+                rc::check("esp_mqtt_client_set_uri accepts well-formed URIs", [&] {
+                    auto char_gen = rc::gen::elementOf(char_set);
                     auto scheme = *rc::gen::elementOf(schemes);
                     auto host = *rc::gen::container<std::string>(
-                        *rc::gen::inRange<int>(1, 33), host_char).as("host");
+                        *rc::gen::inRange<int>(1, 33), char_gen)
+                    .as("host");
                     auto path = *rc::gen::container<std::string>(
-                        *rc::gen::inRange<int>(0, 17), host_char).as("path");
-                    auto port = *rc::gen::maybe(rc::gen::inRange<uint16_t>(1, 65535)).as("port");
-                    std::string uri = scheme;
-                    uri += "://";
-                    uri += host;
+                        *rc::gen::inRange<int>(0, 17), char_gen)
+                    .as("path");
+                    auto query = *rc::gen::container<std::string>(
+                        *rc::gen::inRange<int>(0, 17), char_gen)
+                    .as("query");
+                    auto port =
+                    *rc::gen::maybe(rc::gen::inRange<uint16_t>(1, 65535)).as("port");
+                    std::string uri =
+                    build_uri(scheme, host, port ? &*port : nullptr, path, query);
 
-                    if (port) {
-                        uri += ":";
-                        uri += std::to_string(*port);
-                    }
-
-                    if (!path.empty())
-                    {
-                        uri += "/";
-                        uri += path;
-                    }
-
-                    RC_ASSERT(esp_mqtt_client_set_uri(client.get(), uri.c_str()) == ESP_OK);
+                    log.clear();
+                    RC_ASSERT(esp_mqtt_client_set_uri(client.get(), uri.c_str()) ==
+                              ESP_OK);
+                    RC_ASSERT(
+                    log.contains("mqtt_client", std::string{"config_uri="} + uri));
                 });
             }
             SECTION("User set interface to use") {
                 struct ifreq if_name = {};
                 strncpy(if_name.ifr_name, "custom", IFNAMSIZ - 1);
-                if_name.ifr_name[IFNAMSIZ - 1] = '\0';;
+                if_name.ifr_name[IFNAMSIZ - 1] = '\0';
                 config.network.if_name = &if_name;
                 SECTION("Client is not started") {
                     REQUIRE(esp_mqtt_set_config(client.get(), &config) == ESP_OK);
@@ -150,38 +316,47 @@ SCENARIO("MQTT Client Operation")
                 REQUIRE(esp_mqtt_client_start(client.get()) == ESP_OK);
                 using namespace test::esp_log::matchers;
                 REQUIRE_THAT(log, HasMessageIn("mqtt_client", "Core selection"));
-                // Only need to start the client, destroy is called automatically at the end of
-                // scope
+                // Only need to start the client, destroy is called automatically at the
+                // end of scope
             }
         }
         SECTION("Client with all allocating configuration set") {
-            auto host = random_string(20);
-            auto path = random_string(10);
-            auto username = random_string(10);
-            auto client_id = random_string(10);
-            auto password = random_string(10);
-            auto lw_topic = random_string(10);
-            auto lw_msg = random_string(10);
-            config.broker = {.address = {
-                    .hostname = host.data(),
-                                    .path = path.data()
-                }
+            xQueueCreateMutex_IgnoreAndReturn(
+                reinterpret_cast<QueueHandle_t>(&mtx));
+            rc::check("esp_mqtt_client_init succeeds with arbitrary config fields",
+            [&] {
+                auto char_gen = rc::gen::elementOf(char_set);
+                auto host = *rc::gen::container<std::string>(
+                    *rc::gen::inRange<int>(1, 20), char_gen).as("host");
+                auto path = *rc::gen::container<std::string>(
+                    *rc::gen::inRange<int>(1, 10), char_gen).as("path");
+                auto username = *rc::gen::container<std::string>(
+                    *rc::gen::inRange<int>(1, 10), char_gen).as("username");
+                auto client_id = *rc::gen::container<std::string>(
+                    *rc::gen::inRange<int>(1, 10), char_gen).as("client_id");
+                auto password = *rc::gen::container<std::string>(
+                    *rc::gen::inRange<int>(1, 10), char_gen).as("password");
+                auto lw_topic = *rc::gen::container<std::string>(
+                    *rc::gen::inRange<int>(1, 10), char_gen).as("lw_topic");
+                auto lw_msg = *rc::gen::container<std::string>(
+                    *rc::gen::inRange<int>(1, 10), char_gen).as("lw_msg");
+
+                esp_mqtt_client_config_t alloc_cfg{};
+                alloc_cfg.broker = {
+                    .address = {.hostname = host.data(), .path = path.data()}
                 };
-            config.credentials = {
+                alloc_cfg.credentials = {
                     .username = username.data(),
                                         .client_id = client_id.data(),
-                .authentication = {
-                    .password = password.data()
-                }
+                    .authentication = {.password = password.data()}
                 };
-            config.session = {
-                .last_will {
-                    .topic = lw_topic.data(),
-                                     .msg = lw_msg.data()
-                }
+                alloc_cfg.session = {
+                    .last_will{.topic = lw_topic.data(), .msg = lw_msg.data()}
                 };
-            auto client = unique_mqtt_client{esp_mqtt_client_init(&config)};
-            REQUIRE(client != nullptr);
+                auto client = unique_mqtt_client{esp_mqtt_client_init(&alloc_cfg)};
+                const bool client_is_valid = client != nullptr;
+                RC_ASSERT(client_is_valid);
+            });
         }
     }
 }

test/mqtt_utils_host_test/main/test_cases.cpp

@@ -1,13 +1,15 @@
 /*
- * SPDX-FileCopyrightText: 2025 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2025-2026 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Unlicense OR CC0-1.0
  */
 #include <catch2/catch_test_macros.hpp>
 #include <cstring>
+#include <exception>
 #include <iostream>
 #include <iomanip>
 #include <format>
+#include <string_view>
 #include "rapidcheck.h"
 #include "mqtt_utils.h"
 
@@ -75,6 +77,18 @@ std::string percent_encode(std::string input, std::string filter = "`~!@#$%^&*()
     return encoded_str;
 }
 
+static std::string build_uri_authority(std::string_view username, std::string_view password, std::string_view host)
+{
+    std::string uri;
+    uri.reserve(username.size() + password.size() + host.size() + 2);
+    uri += username;
+    uri += ":";
+    uri += password;
+    uri += "@";
+    uri += host;
+    return uri;
+}
+
 TEST_CASE("Parse percent-encoded data")
 {
     SECTION("Zero-length string as an input") {
@@ -120,8 +134,10 @@ TEST_CASE("Parse percent-encoded data")
             RC_PRE(uri.host.value.length() > 0);
             RC_PRE(uri.username.value.length() > 0);
             RC_PRE(uri.password.value.length() > 0);
-            std::string complete_uri_raw = uri.username.value + ":" + uri.password.value + "@" + uri.host.value;
-            std::string complete_uri_enc = percent_encode(uri.username.value) + ":" + percent_encode(uri.password.value) + "@" + percent_encode(uri.host.value);
+            const std::string complete_uri_raw = build_uri_authority(uri.username.value, uri.password.value, uri.host.value);
+            const std::string complete_uri_enc = build_uri_authority(percent_encode(uri.username.value),
+                                                                     percent_encode(uri.password.value),
+                                                                     percent_encode(uri.host.value));
             // Verify that there are no prohibited characters in the encoded username
             RC_PRE([complete_uri_enc]() -> bool {
                 // I have removed /, :, and @ as they are permitted symbols in URI