From 91f5f4392ecca07b71f774e4dbe1093fa70369b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Wed, 27 Apr 2022 17:43:24 +0200 Subject: [PATCH 1/5] Properly handle 'EINTR' for signals for RESTAPI scripts #3838 --- lib/proxysql_utils.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/proxysql_utils.cpp b/lib/proxysql_utils.cpp index 64da63c2e..567435562 100644 --- a/lib/proxysql_utils.cpp +++ b/lib/proxysql_utils.cpp @@ -222,7 +222,7 @@ int wexecvp( int select_err = select(maxfd + 1, &read_fds, NULL, NULL, &select_to); // Unexpected error while executing 'select' - if (select_err < 0) { + if (select_err < 0 && errno != EINTR) { pipe_err = -5; // Backup read errno errno_cpy = errno; From 074e4c0595abff0b176b12b5d82a491a0adfe510 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Wed, 27 Apr 2022 17:47:25 +0200 Subject: [PATCH 2/5] Improve error report for RESTAPI scripts #3838 Previously any error that wasn't ETIME (timeout) was reported as 'Internal error' and 'errno' was reported. This didn't cover reporting terminations of the child processes by signals. --- lib/ProxySQL_RESTAPI_Server.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/ProxySQL_RESTAPI_Server.cpp b/lib/ProxySQL_RESTAPI_Server.cpp index 974d5d62a..58fde5aeb 100644 --- a/lib/ProxySQL_RESTAPI_Server.cpp +++ b/lib/ProxySQL_RESTAPI_Server.cpp @@ -136,7 +136,7 @@ private: + "\"}"; proxy_error("Request to execute script '%s' timed out.\n", script.c_str()); - } else { + } else if (script_err < 0) { // there was an internal error unrelated to script execution internal_error = true; @@ -171,6 +171,13 @@ private: failed_syscall.c_str(), script_errno ); + } else { + str_response_err = + "{\"type\":\"out\", \"error\":\"Terminated without exit code. Child exit status reported in" + " 'error_code'\", \"error_code\":\"" + std::to_string(script_err) + "\"}"; + proxy_error( + "Error while executing script '%s'. Child exit status: '%d'.\n", script.c_str(), script_err + ); } } // script returned and empty output, invalid output, no need to parse it. From 3dc0ba4744740a9f03ebf742387c6129383c95fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Wed, 27 Apr 2022 18:25:00 +0200 Subject: [PATCH 3/5] Add regression test for RESTAPI signal handling #3838 --- .../tests/reg_test_3838-restapi_eintr-t.cpp | 229 ++++++++++++++++++ .../reg_test_3838_scripts/simple_sleep.sh | 10 + 2 files changed, 239 insertions(+) create mode 100644 test/tap/tests/reg_test_3838-restapi_eintr-t.cpp create mode 100755 test/tap/tests/reg_test_3838_scripts/simple_sleep.sh diff --git a/test/tap/tests/reg_test_3838-restapi_eintr-t.cpp b/test/tap/tests/reg_test_3838-restapi_eintr-t.cpp new file mode 100644 index 000000000..ce1a76bc8 --- /dev/null +++ b/test/tap/tests/reg_test_3838-restapi_eintr-t.cpp @@ -0,0 +1,229 @@ +/** + * @file reg_test_3838-restapi_eintr-t.cpp + * @brief This is a regression test for issue #3838. Test checks that scripts executed via RESTAPI doesn't get + * unproperly interrupted by signals. + * @details The test register a simple waiting script into the RESTAPI, for latter issuing multiple different + * signals to it and checks that: + * - ProxySQL properly handles the signals being set to the executed script. + * - Timeouts work properly no matter the signaling. + * - ProxySQL correctly reports the child termination exit status. E.g. If terminated by a signal. + * @date 2022-04-27 + */ + +#include +#include +#include +#include +#include +#include + +#include +#include + +#include + +#include +#include + +#include "json.hpp" +#include "tap.h" +#include "proxysql_utils.h" +#include "command_line.h" +#include "utils.h" + +using std::string; +using std::vector; + +const int SIGNAL_NUM = 5; +const string base_address { "http://localhost:6070/sync/" }; + +using params = std::string; +using signal_t = int; +using rescode_t = long; + +vector> endpoint_requests { + std::make_tuple("simple_sleep", "1", 200, 18, 0), + std::make_tuple("simple_sleep_timeout", "4", 424, 18, ETIME), + std::make_tuple("simple_sleep_timeout", "4", 424, 19, ETIME), + std::make_tuple("simple_sleep", "2", 424, 15, 15), +}; + +int main(int argc, char** argv) { + CommandLine cl; + + if (cl.getEnv()) { + diag("Failed to get the required environmental variables."); + return EXIT_FAILURE; + } + + plan(endpoint_requests.size()); + + MYSQL* proxysql_admin = mysql_init(NULL); + + // Initialize connections + if (!proxysql_admin) { + fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql_admin)); + return EXIT_FAILURE; + } + if (!mysql_real_connect(proxysql_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(proxysql_admin)); + return EXIT_FAILURE; + } + + // Enable 'RESTAPI' + MYSQL_QUERY(proxysql_admin, "SET admin-restapi_enabled='true'"); + MYSQL_QUERY(proxysql_admin, "SET admin-restapi_port=6070"); + + MYSQL_QUERY(proxysql_admin, "LOAD ADMIN VARIABLES TO RUNTIME"); + + // Clean current 'restapi_routes' if any + MYSQL_QUERY(proxysql_admin, "DELETE FROM restapi_routes"); + + // Configure restapi_routes to be used + string test_script_base_path { string { cl.workdir } + "reg_test_3838_scripts" }; + + vector t_valid_scripts_inserts { + "INSERT INTO restapi_routes (active, timeout_ms, method, uri, script, comment)" + " VALUES (1,3000,'POST','simple_sleep','%s/simple_sleep.sh','simple_sleep_comment')", + "INSERT INTO restapi_routes (active, timeout_ms, method, uri, script, comment)" + " VALUES (1,3000,'POST','simple_sleep_timeout','%s/simple_sleep.sh','simple_sleep_to_comment')", + }; + vector valid_scripts_inserts {}; + + for (const auto& t_valid_script_insert : t_valid_scripts_inserts) { + string valid_script_insert {}; + string_format(t_valid_script_insert, valid_script_insert, test_script_base_path.c_str()); + valid_scripts_inserts.push_back(valid_script_insert); + } + + // Configure routes for valid scripts + for (const auto& valid_script_insert : valid_scripts_inserts) { + MYSQL_QUERY(proxysql_admin, valid_script_insert.c_str()); + } + + // Load RESTAPI + MYSQL_QUERY(proxysql_admin, "LOAD RESTAPI TO RUNTIME"); + + // Sensible wait until the new configured enpoints are ready. Use the first enpoint for the check + const auto& first_request_tuple { endpoint_requests.front() }; + const string full_endpoint { + base_address + std::get<0>(first_request_tuple) + "/" + }; + int endpoint_timeout = wait_until_enpoint_ready(full_endpoint, std::get<1>(first_request_tuple), 10, 500); + + if (endpoint_timeout) { + diag( + "Timeout while trying to reach first valid enpoint. Test failed, skipping endpoint testing..." + ); + goto skip_endpoints_testing; + } + + for (const auto& request : endpoint_requests) { + const string endpoint { base_address + std::get<0>(request) + "/"}; + const string params { std::get<1>(request) }; + const long exp_rc = std::get<2>(request); + const int signal = std::get<3>(request); + const int exp_child_exit_st = std::get<4>(request); + + string post_out_err { "" }; + uint64_t curl_res_code = 0; + + CURLcode post_err = CURLE_HTTP_POST_ERROR; + + // 1. Perform the POST operation + std::thread post_op_th([&] () -> void { + post_err = perform_simple_post(endpoint, params, curl_res_code, post_out_err); + }); + + // 2. Find the child process + string s_pid {}; + + int timeout = 1000; + int waited = 0; + int e_res= 0; + + while (waited < timeout) { + e_res = exec("ps aux | grep -e \"[/]bin/sh.*.simple_sleep.sh\" | awk '{print $2}'", s_pid); + + if (e_res == 0 && s_pid.empty()) { + usleep(200 * 1000); + waited += 200; + } else { + break; + } + } + + if (e_res != EXIT_SUCCESS || s_pid.empty()) { + if (e_res != EXIT_SUCCESS) { + fprintf(stderr, "File %s, line %d, 'exec' failed with error: '%d'\n", __FILE__, __LINE__, e_res); + } else { + const string err_msg {"Invalid command executed or faulty test logic" }; + fprintf(stderr, "File %s, line %d, Error: '%s'\n", __FILE__, __LINE__, err_msg.c_str()); + } + } else { + // 3. Send multiple signals to the child process + int pid = std::stol(s_pid); + int k_res = 0; + + if (signal == 18) { + for (int i = 0; i < SIGNAL_NUM; i++) { + k_res = kill(pid, 19); + if (k_res != 0) { break; } + + usleep(100*1000); + + k_res = kill(pid, 18); + if (k_res != 0) { break; } + } + } else { + for (int i = 0; i < SIGNAL_NUM; i++) { + k_res = kill(pid, signal); + } + } + + if (k_res != 0) { + fprintf(stderr, "File %s, line %d, 'kill' failed with error: '%d'\n", __FILE__, __LINE__, errno); + } + } + + post_op_th.join(); + + try { + int child_exit_st = 0; + int signaled = 0; + int exp_signaled = 0; + + if (post_out_err.empty() == false) { + nlohmann::json j_curl_err = nlohmann::json::parse(post_out_err); + child_exit_st = std::stol(j_curl_err["error_code"].get()); + } + + // NOTE: This is pointless because the value doesn't change, but it's a demonstration on how to + // recover child process exit statuses for debugging purposes. + if (exp_child_exit_st == 15) { + exp_signaled = 1; + signaled = WIFSIGNALED(child_exit_st); + child_exit_st = WTERMSIG(child_exit_st); + } + + bool ok_check = + post_err == CURLE_OK && curl_res_code == exp_rc && + exp_signaled == signaled && exp_child_exit_st == child_exit_st; + + ok( + ok_check, + "Performing a POST over endpoint '%s' should result into a '%ld' response:" + " (curl_err: '%d', response_errcode: '%ld', signaled: '%d', child_exit_st: '%d', curlerr: '%s')", + endpoint.c_str(), exp_rc, post_err, curl_res_code, signaled, child_exit_st, post_out_err.c_str() + ); + } catch (const std::exception& ex) { + diag("Invalid error kind returned by ProxySQL, JSON parsing failed with error: %s", ex.what()); + } + } + +skip_endpoints_testing: + + mysql_close(proxysql_admin); + + return exit_status(); +} diff --git a/test/tap/tests/reg_test_3838_scripts/simple_sleep.sh b/test/tap/tests/reg_test_3838_scripts/simple_sleep.sh new file mode 100755 index 000000000..cf056e47e --- /dev/null +++ b/test/tap/tests/reg_test_3838_scripts/simple_sleep.sh @@ -0,0 +1,10 @@ +#!/bin/sh + +if [ ! "$1" ]; then + echo { \"err\": \"Missing required operand\" } + exit 1 +fi + +sleep $1 + +echo { \"param\": $1 } From 4a3cca28c9d05ec375b22195e6a21ad6e90690df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Wed, 27 Apr 2022 20:06:19 +0200 Subject: [PATCH 4/5] Extract POST response also for failing requests in 'perform_simple_post' #3838 --- test/tap/tap/utils.cpp | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/test/tap/tap/utils.cpp b/test/tap/tap/utils.cpp index b36732226..bd87faf24 100644 --- a/test/tap/tap/utils.cpp +++ b/test/tap/tap/utils.cpp @@ -22,6 +22,8 @@ #include "proxysql_utils.h" +using std::string; + int show_variable(MYSQL *mysql, const std::string& var_name, std::string& var_value) { char query[128]; @@ -313,8 +315,26 @@ std::vector extract_mysql_rows(MYSQL_RES* my_res) { return result; }; -size_t my_dummy_write(char*, size_t size, size_t nmemb, void*) { - return size * nmemb; +struct memory { + char* data; + size_t size; +}; + +static size_t write_callback(void *data, size_t size, size_t nmemb, void *userp) { + size_t realsize = size * nmemb; + struct memory *mem = (struct memory *)userp; + + char* ptr = static_cast(realloc(mem->data, mem->size + realsize + 1)); + if(ptr == NULL) { + return 0; + } + + mem->data = ptr; + memcpy(&(mem->data[mem->size]), data, realsize); + mem->size += realsize; + mem->data[mem->size] = 0; + + return realsize; } CURLcode perform_simple_post( @@ -330,7 +350,9 @@ CURLcode perform_simple_post( if(curl) { curl_easy_setopt(curl, CURLOPT_URL, endpoint.c_str()); curl_easy_setopt(curl, CURLOPT_POSTFIELDS, post_params.c_str()); - curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, &my_dummy_write); + struct memory response = { 0 }; + curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_callback); + curl_easy_setopt(curl, CURLOPT_WRITEDATA, (void *)&response); res = curl_easy_perform(curl); @@ -338,6 +360,10 @@ CURLcode perform_simple_post( curl_out_err = std::string { curl_easy_strerror(res) }; } else { curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &curl_res_code); + + if (curl_res_code != 200) { + curl_out_err = string(response.data, response.size); + } } curl_easy_cleanup(curl); From 4799ae3a228a4a2c1dfe0e9a4257559123ffeec6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Wed, 27 Apr 2022 20:40:20 +0200 Subject: [PATCH 5/5] Fix leak at 'wait_for_proxysql' test helper function #3838 --- test/tap/tap/utils.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/test/tap/tap/utils.cpp b/test/tap/tap/utils.cpp index bd87faf24..a2b60ed6a 100644 --- a/test/tap/tap/utils.cpp +++ b/test/tap/tap/utils.cpp @@ -686,6 +686,7 @@ MYSQL* wait_for_proxysql(const conn_opts_t& opts, int timeout) { } if (con_waited >= timeout) { + mysql_close(admin); return nullptr; } else { return admin;