From ae8ec8da3ad86a563ba9cfd4dc1b5b7c344c9f24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Thu, 15 Dec 2022 13:55:28 +0100 Subject: [PATCH 1/2] Fix crash on RESTAPI when malformed request is received #4055 This commits packs two fixes: - Patch to 'libhttpserver' to properly handle empty URI. - Improve compiler version detection for applying required 'libhttpserver' patches. --- deps/Makefile | 10 ++++++---- deps/libhttpserver/empty_uri_log_crash.patch | 13 +++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) create mode 100644 deps/libhttpserver/empty_uri_log_crash.patch diff --git a/deps/Makefile b/deps/Makefile index b25e31b76..8e2438ebc 100644 --- a/deps/Makefile +++ b/deps/Makefile @@ -88,11 +88,12 @@ libssl/openssl/libssl.a: cd libssl/openssl && ln -s . lib # curl wants this path libssl: libssl/openssl/libssl.a -GCC_VERSION := $(shell gcc -dumpversion) MIN_VERSION := 4.9.0 -REQUIRE_PATCH = false +GCC_VERSION := $(shell gcc -dumpversion) +SORTED_VERSIONS := $(shell echo -e "$(GCC_VERSION)\n$(MIN_VERSION)" | sort -V) -ifeq ($(MIN_VERSION),$(lastword $(sort $(GCC_VERSION) $(MIN_VERSION)))) +REQUIRE_PATCH = false +ifeq ($(MIN_VERSION),$(lastword $(SORTED_VERSIONS))) REQUIRE_PATCH = true endif @@ -110,8 +111,9 @@ ifeq ($(REQUIRE_PATCH), true) cd libhttpserver/libhttpserver && patch src/httpserver/http_response.hpp < ../http_response.hpp.patch cd libhttpserver/libhttpserver && patch src/httpserver/string_response.hpp < ../string_response.hpp.patch cd libhttpserver/libhttpserver && patch -p0 < ../re2_regex.patch - cd libhttpserver/libhttpserver && patch -p0 < ../final_val_post_process.patch endif + cd libhttpserver/libhttpserver && patch -p0 < ../final_val_post_process.patch + cd libhttpserver/libhttpserver && patch -p0 < ../empty_uri_log_crash.patch ifeq ($(UNAME_S),FreeBSD) sed -i -e 's/\/bin\/bash/\/usr\/local\/bin\/bash/' libhttpserver/libhttpserver/bootstrap endif diff --git a/deps/libhttpserver/empty_uri_log_crash.patch b/deps/libhttpserver/empty_uri_log_crash.patch new file mode 100644 index 000000000..3970a7f88 --- /dev/null +++ b/deps/libhttpserver/empty_uri_log_crash.patch @@ -0,0 +1,13 @@ +diff --git src/webserver.cpp src/webserver.cpp +index 5ae7381..04a5a28 100644 +--- src/webserver.cpp ++++ src/webserver.cpp +@@ -443,7 +443,7 @@ int policy_callback (void *cls, const struct sockaddr* addr, socklen_t addrlen) + void* uri_log(void* cls, const char* uri) + { + struct details::modded_request* mr = new details::modded_request(); +- mr->complete_uri = new string(uri); ++ mr->complete_uri = new string(uri == NULL ? "" : uri); + mr->second = false; + return ((void*)mr); + } From 910b225f005f11dd50a980aa59d6f1a3d9d4acef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Thu, 15 Dec 2022 14:00:32 +0100 Subject: [PATCH 2/2] Add regression test for malformed request on RESTAPI #4055 --- test/tap/tap/utils.cpp | 12 ++ test/tap/tap/utils.h | 10 ++ test/tap/tests/reg_test_4055_restapi-t.cpp | 123 ++++++++++++++++++ ...metheus_metrics_dump-t_disabled_gh3571.cpp | 12 -- 4 files changed, 145 insertions(+), 12 deletions(-) create mode 100644 test/tap/tests/reg_test_4055_restapi-t.cpp diff --git a/test/tap/tap/utils.cpp b/test/tap/tap/utils.cpp index c4a26c24e..ec5be5fad 100644 --- a/test/tap/tap/utils.cpp +++ b/test/tap/tap/utils.cpp @@ -25,6 +25,18 @@ using std::string; using std::vector; +std::size_t count_matches(const string& str, const string& substr) { + std::size_t result = 0; + std::size_t pos = 0; + + while ((pos = str.find(substr, pos)) != string::npos) { + result += 1; + pos += substr.length(); + } + + return result; +} + int mysql_query_t(MYSQL* mysql, const char* query) { diag("%s: Issuing query '%s' to ('%s':%d)", get_formatted_time().c_str(), query, mysql->host, mysql->port); return mysql_query(mysql, query); diff --git a/test/tap/tap/utils.h b/test/tap/tap/utils.h index 4b286459e..f9761bf12 100644 --- a/test/tap/tap/utils.h +++ b/test/tap/tap/utils.h @@ -431,4 +431,14 @@ int configure_endpoints( bool prevent_dups = true ); +/** + * @brief Returns the matches found of the 'substr' provided in the provided string. + * + * @param str String from which to count the matches. + * @param substr The substring which matches needs to be counted. + * + * @return Number of matches of the 'substr' in the provided string. + */ +std::size_t count_matches(const std::string& str, const std::string& substr); + #endif // #define UTILS_H diff --git a/test/tap/tests/reg_test_4055_restapi-t.cpp b/test/tap/tests/reg_test_4055_restapi-t.cpp new file mode 100644 index 000000000..73564c73b --- /dev/null +++ b/test/tap/tests/reg_test_4055_restapi-t.cpp @@ -0,0 +1,123 @@ +/** + * @file reg_test_4055_restapi-t.cpp + * @brief Simple regression test sending malformed query to RESTAPI. + * @details This test performs the following actions: + * - Issue a malformed request to the RESTAPI. + * - Checks that Admin interface is still responsive. + * - Checks that the 'metrics' endpoint from the RESTAPI is still responsive. + * - Perform minor correctness check on the 'metrics' endpoint response. + * @date 2022-12-15 + */ + +#include +#include +#include +#include +#include + +#include +#include + +#include "mysql.h" + +#include "tap.h" +#include "command_line.h" +#include "utils.h" + +using std::string; + +/* This is an estimation of the supported number of metrics as for '2022-12-15' */ +uint32_t SUPPORTED_METRICS = 148; + +int main(int argc, char** argv) { + plan(5); + + CommandLine cl; + + if (cl.getEnv()) { + diag("Failed to get the required environmental variables."); + return EXIT_FAILURE; + } + + MYSQL* admin = mysql_init(NULL); + + if (!mysql_real_connect(admin, cl.host, cl.admin_username, cl.admin_password, NULL, cl.admin_port, NULL, 0)) { + fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); + return EXIT_FAILURE; + } + + // Enable 'RESTAPI' + MYSQL_QUERY(admin, "SET admin-restapi_enabled='true'"); + MYSQL_QUERY(admin, "SET admin-restapi_port=6070"); + MYSQL_QUERY(admin, "LOAD ADMIN VARIABLES TO RUNTIME"); + + int socket_desc; + struct sockaddr_in server; + + socket_desc = socket(AF_INET , SOCK_STREAM , 0); + if (socket_desc == -1) { + return errno; + } + + server.sin_addr.s_addr = inet_addr("127.0.0.1"); + server.sin_family = AF_INET; + server.sin_port = htons(6070); + + if (connect(socket_desc , (struct sockaddr *)&server , sizeof(server)) < 0) { + return errno; + } + + // Perform the invalid request, and add a sleep to let ProxySQL process the data + { + ssize_t n = write(socket_desc, static_cast(" \n"), strlen(" \n")); + diag("Written '%lu' bytes into socket", n); + sleep(1); + } + + int myrc = mysql_query(admin, "SELECT version()"); + ok(myrc == EXIT_SUCCESS, "ProxySQL is still up and reachable"); + + MYSQL_RES* myres = mysql_store_result(admin); + MYSQL_ROW myrow = mysql_fetch_row(myres); + + string recv_version {}; + if (myrow && myrow[0]) { recv_version = myrow[0]; } + mysql_free_result(myres); + + ok(recv_version.empty() == false, "Received non empty ProxySQL version '%s'", recv_version.c_str()); + + uint64_t curl_res_code = 0; + string curl_res_data {}; + + CURLcode code = perform_simple_get("http://localhost:6070/metrics/", curl_res_code, curl_res_data); + + ok( + code == CURLE_OK && curl_res_code == 200, + "RESTAPI still up and responding to requests - curl_code: %d, res_code: %lu", + code, curl_res_code + ); + + size_t matches = count_matches(curl_res_data, "# "); + const uint32_t min_exp_metrics = SUPPORTED_METRICS - 20; + + ok( + matches % 2 == 0, + "Response from endpoint is well-formed (even number of '# ' metrics descriptions) - matches: '%ld'", + matches + ); + + ok( + min_exp_metrics < (matches / 2), + "Response contains more than a minimum of expected metrics - min: %d, act: %lu", + min_exp_metrics, matches / 2 + ); + + if (tests_failed()) { + diag("Failed! Received GET response: \n\n%s", curl_res_data.c_str()); + } + + close(socket_desc); + mysql_close(admin); + + return exit_status(); +} diff --git a/test/tap/tests/test_admin_prometheus_metrics_dump-t_disabled_gh3571.cpp b/test/tap/tests/test_admin_prometheus_metrics_dump-t_disabled_gh3571.cpp index e3e269823..4b3ba5432 100644 --- a/test/tap/tests/test_admin_prometheus_metrics_dump-t_disabled_gh3571.cpp +++ b/test/tap/tests/test_admin_prometheus_metrics_dump-t_disabled_gh3571.cpp @@ -21,18 +21,6 @@ using std::string; std::size_t supported_metrics = 121; -std::size_t count_matches(const std::string& str, const std::string& substr) { - std::size_t result = 0; - std::size_t pos = 0; - - while ((pos = str.find(substr, pos)) != std::string::npos) { - result += 1; - pos += substr.length(); - } - - return result; -} - int main(int argc, char** argv) { CommandLine cl;