From 736d4403616da2f99377c1747429e231aea8ffb0 Mon Sep 17 00:00:00 2001 From: Euripedes Rocha Filho Date: Wed, 29 Apr 2026 14:53:44 +0200 Subject: [PATCH] fix: Fixed uri inconsistency when updated --- mqtt_client.c | 121 ++++-- test/host/main/CMakeLists.txt | 10 +- test/host/main/test_log_matchers.cpp | 34 ++ test/host/main/test_log_matchers.hpp | 30 ++ test/host/main/test_mqtt_client.cpp | 343 +++++++++++++----- test/mqtt_utils_host_test/main/test_cases.cpp | 22 +- 6 files changed, 431 insertions(+), 129 deletions(-) diff --git a/mqtt_client.c b/mqtt_client.c index aab7cad..6db0d29 100644 --- a/mqtt_client.c +++ b/mqtt_client.c @@ -1075,83 +1075,128 @@ 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); } if (asprintf_ret_value == -1) { - ESP_LOGE(TAG, "%s(%d): %s", __FUNCTION__, __LINE__, "Memory exhausted"); - MQTT_API_UNLOCK(client); - return ESP_ERR_NO_MEM; + ESP_LOGE(TAG, "%s(%d): %s", __FUNCTION__, __LINE__, "Memory exhausted"); + 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 ++; + 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); + 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); - return ESP_OK; +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) diff --git a/test/host/main/CMakeLists.txt b/test/host/main/CMakeLists.txt index ef32740..be8879b 100644 --- a/test/host/main/CMakeLists.txt +++ b/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) diff --git a/test/host/main/test_log_matchers.cpp b/test/host/main/test_log_matchers.cpp index 7235f2b..566f19e 100644 --- a/test/host/main/test_log_matchers.cpp +++ b/test/host/main/test_log_matchers.cpp @@ -9,6 +9,7 @@ */ #include "test_log_matchers.hpp" +#include #include #include #include @@ -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 views; diff --git a/test/host/main/test_log_matchers.hpp b/test/host/main/test_log_matchers.hpp index 5693ca0..6d48214 100644 --- a/test/host/main/test_log_matchers.hpp +++ b/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 expected_messages) { return LogsInOrder{expected_messages}; diff --git a/test/host/main/test_mqtt_client.cpp b/test/host/main/test_mqtt_client.cpp index 49a5600..10229e2 100644 --- a/test/host/main/test_mqtt_client.cpp +++ b/test/host/main/test_mqtt_client.cpp @@ -3,24 +3,23 @@ * * SPDX-License-Identifier: Apache-2.0 */ -#include +#include "esp_transport.h" +#include +#include +#include +#include #include #include #include -#include +#include #include #include #include -#include "esp_transport.h" -#include -#include -#include #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,7 +28,8 @@ extern "C" { #include "Mockevent_groups.h" #include "Mockqueue.h" #include "Mocktask.h" -#if __has_include ("Mockidf_additions.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" #endif @@ -38,23 +38,50 @@ 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 += "@"; + } + + 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, -decltype([](esp_mqtt_client_handle_t client) +using unique_mqtt_client = + std::unique_ptr < std::remove_pointer_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(&mtx)); - xEventGroupCreate_IgnoreAndReturn(reinterpret_cast(&event_group)); - esp_transport_list_init_IgnoreAndReturn(reinterpret_cast(&transport_list)); - esp_transport_tcp_init_IgnoreAndReturn(reinterpret_cast(&transport)); - esp_transport_ssl_init_IgnoreAndReturn(reinterpret_cast(&transport)); - esp_transport_ws_init_IgnoreAndReturn(reinterpret_cast(&transport)); + xEventGroupCreate_IgnoreAndReturn( + reinterpret_cast(&event_group)); + esp_transport_list_init_IgnoreAndReturn( + reinterpret_cast(&transport_list)); + esp_transport_tcp_init_IgnoreAndReturn( + reinterpret_cast(&transport)); + esp_transport_ssl_init_IgnoreAndReturn( + reinterpret_cast(&transport)); + esp_transport_ws_init_IgnoreAndReturn( + reinterpret_cast(&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_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( + *rc::gen::inRange(1, 20), char_gen).as("host"); + auto path = *rc::gen::container( + *rc::gen::inRange(0, 10), char_gen).as("path"); + auto port = *rc::gen::maybe( + rc::gen::inRange(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( + *rc::gen::inRange(1, 16), char_gen).as("host1"); + auto scheme2 = *rc::gen::elementOf(schemes); + auto host2 = *rc::gen::container( + *rc::gen::inRange(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 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", - [&] { + esp_log_level_set("mqtt_client", ESP_LOG_DEBUG); + static constexpr std::array schemes = {"mqtt", "mqtts", + "ws", "wss" + }; + 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( - *rc::gen::inRange(1, 33), host_char).as("host"); + *rc::gen::inRange(1, 33), char_gen) + .as("host"); auto path = *rc::gen::container( - *rc::gen::inRange(0, 17), host_char).as("path"); - auto port = *rc::gen::maybe(rc::gen::inRange(1, 65535)).as("port"); - std::string uri = scheme; - uri += "://"; - uri += host; + *rc::gen::inRange(0, 17), char_gen) + .as("path"); + auto query = *rc::gen::container( + *rc::gen::inRange(0, 17), char_gen) + .as("query"); + auto port = + *rc::gen::maybe(rc::gen::inRange(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() - } - }; - config.credentials = { - .username = username.data(), - .client_id = client_id.data(), - .authentication = { - .password = password.data() - } - }; - config.session = { - .last_will { - .topic = lw_topic.data(), - .msg = lw_msg.data() - } - }; - auto client = unique_mqtt_client{esp_mqtt_client_init(&config)}; - REQUIRE(client != nullptr); + xQueueCreateMutex_IgnoreAndReturn( + reinterpret_cast(&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( + *rc::gen::inRange(1, 20), char_gen).as("host"); + auto path = *rc::gen::container( + *rc::gen::inRange(1, 10), char_gen).as("path"); + auto username = *rc::gen::container( + *rc::gen::inRange(1, 10), char_gen).as("username"); + auto client_id = *rc::gen::container( + *rc::gen::inRange(1, 10), char_gen).as("client_id"); + auto password = *rc::gen::container( + *rc::gen::inRange(1, 10), char_gen).as("password"); + auto lw_topic = *rc::gen::container( + *rc::gen::inRange(1, 10), char_gen).as("lw_topic"); + auto lw_msg = *rc::gen::container( + *rc::gen::inRange(1, 10), char_gen).as("lw_msg"); + + esp_mqtt_client_config_t alloc_cfg{}; + alloc_cfg.broker = { + .address = {.hostname = host.data(), .path = path.data()} + }; + alloc_cfg.credentials = { + .username = username.data(), + .client_id = client_id.data(), + .authentication = {.password = password.data()} + }; + alloc_cfg.session = { + .last_will{.topic = lw_topic.data(), .msg = lw_msg.data()} + }; + auto client = unique_mqtt_client{esp_mqtt_client_init(&alloc_cfg)}; + const bool client_is_valid = client != nullptr; + RC_ASSERT(client_is_valid); + }); } } } diff --git a/test/mqtt_utils_host_test/main/test_cases.cpp b/test/mqtt_utils_host_test/main/test_cases.cpp index e88b391..fc335d4 100644 --- a/test/mqtt_utils_host_test/main/test_cases.cpp +++ b/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 #include +#include #include #include #include +#include #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