Merge pull request #3435 from sysown/v2.2.0-3434

Closes #3434: ProxySQL v2.1.X crashes due to invalid memory being accessed by mthd_my_read_metadata_ex
pull/3444/head
René Cannaò 5 years ago committed by GitHub
commit 8f243ab271
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

@ -12,7 +12,7 @@ index 62cf71c..c05e9a3 100644
COM_MULTI_OFF= 0,
COM_MULTI_CANCEL,
diff --git include/mariadb_com.h include/mariadb_com.h
index 7e722a0..dc121c7 100644
index 948d96e..225b2f5 100644
--- include/mariadb_com.h
+++ include/mariadb_com.h
@@ -160,6 +160,7 @@ enum enum_server_command
@ -65,7 +65,7 @@ index 15be4fc..a69e637 100644
See bug conc-57
*/
diff --git libmariadb/mariadb_lib.c libmariadb/mariadb_lib.c
index 8c2a99b..e849b76 100644
index e475e25..1a04b2f 100644
--- libmariadb/mariadb_lib.c
+++ libmariadb/mariadb_lib.c
@@ -144,6 +144,7 @@ struct st_mariadb_methods MARIADB_DEFAULT_METHODS;
@ -480,7 +480,7 @@ index 8c2a99b..e849b76 100644
/****************************************************************************
** Init MySQL structure or allocate one
****************************************************************************/
@@ -1491,7 +1647,7 @@ MYSQL *mthd_my_real_connect(MYSQL *mysql, const char *host, const char *user,
@@ -1494,7 +1650,7 @@ MYSQL *mthd_my_real_connect(MYSQL *mysql, const char *host, const char *user,
goto error;
}
*/
@ -489,7 +489,7 @@ index 8c2a99b..e849b76 100644
{
if (mysql->net.last_errno == CR_SERVER_LOST)
my_set_error(mysql, CR_SERVER_LOST, SQLSTATE_UNKNOWN,
@@ -1593,6 +1749,16 @@ MYSQL *mthd_my_real_connect(MYSQL *mysql, const char *host, const char *user,
@@ -1596,6 +1752,16 @@ MYSQL *mthd_my_real_connect(MYSQL *mysql, const char *host, const char *user,
}
}
@ -506,7 +506,7 @@ index 8c2a99b..e849b76 100644
/* pad 2 */
end+= 18;
@@ -2290,14 +2456,13 @@ int mthd_my_read_query_result(MYSQL *mysql)
@@ -2361,14 +2527,13 @@ int mthd_my_read_query_result(MYSQL *mysql)
{
uchar *pos;
ulong field_count;
@ -522,7 +522,7 @@ index 8c2a99b..e849b76 100644
{
return(1);
}
@@ -2310,7 +2475,7 @@ get_info:
@@ -2381,7 +2546,7 @@ get_info:
{
int error=mysql_handle_local_infile(mysql, (char *)pos, can_local_infile);
@ -531,7 +531,7 @@ index 8c2a99b..e849b76 100644
return(-1);
goto get_info; /* Get info packet */
}
@@ -2318,12 +2483,13 @@ get_info:
@@ -2389,12 +2554,13 @@ get_info:
mysql->server_status|= SERVER_STATUS_IN_TRANS;
mysql->extra_info= net_field_length_ll(&pos); /* Maybe number of rec */
@ -551,7 +551,7 @@ index 8c2a99b..e849b76 100644
mysql->status=MYSQL_STATUS_GET_RESULT;
mysql->field_count=field_count;
return(0);
@@ -2704,21 +2870,17 @@ mysql_list_fields(MYSQL *mysql, const char *table, const char *wild)
@@ -2775,21 +2941,17 @@ mysql_list_fields(MYSQL *mysql, const char *table, const char *wild)
MYSQL_RES * STDCALL
mysql_list_processes(MYSQL *mysql)
{
@ -576,7 +576,7 @@ index 8c2a99b..e849b76 100644
mysql->status=MYSQL_STATUS_GET_RESULT;
mysql->field_count=field_count;
return(mysql_store_result(mysql));
@@ -4164,7 +4326,7 @@ mysql_debug(const char *debug __attribute__((unused)))
@@ -4251,7 +4413,7 @@ mysql_debug(const char *debug __attribute__((unused)))
*********************************************************************/
ulong STDCALL mysql_net_read_packet(MYSQL *mysql)
{
@ -599,7 +599,7 @@ index 6a7e0cf..492727d 100644
if (pkt_len == packet_error)
{
diff --git libmariadb/mariadb_stmt.c libmariadb/mariadb_stmt.c
index 8f5bddc..5bd0ece 100644
index 0aaaf1a..229023b 100644
--- libmariadb/mariadb_stmt.c
+++ libmariadb/mariadb_stmt.c
@@ -78,6 +78,9 @@ typedef struct
@ -612,7 +612,7 @@ index 8f5bddc..5bd0ece 100644
static my_bool is_not_null= 0;
static my_bool is_null= 1;
@@ -152,8 +155,9 @@ static int stmt_unbuffered_eof(MYSQL_STMT *stmt __attribute__((unused)),
@@ -153,8 +156,9 @@ static int stmt_unbuffered_eof(MYSQL_STMT *stmt __attribute__((unused)),
static int stmt_unbuffered_fetch(MYSQL_STMT *stmt, uchar **row)
{
ulong pkt_len;
@ -623,7 +623,7 @@ index 8f5bddc..5bd0ece 100644
if (pkt_len == packet_error)
{
@@ -161,8 +165,10 @@ static int stmt_unbuffered_fetch(MYSQL_STMT *stmt, uchar **row)
@@ -162,8 +166,10 @@ static int stmt_unbuffered_fetch(MYSQL_STMT *stmt, uchar **row)
return(1);
}
@ -635,7 +635,7 @@ index 8f5bddc..5bd0ece 100644
*row = NULL;
stmt->fetch_row_func= stmt_unbuffered_eof;
return(MYSQL_NO_DATA);
@@ -194,13 +200,15 @@ int mthd_stmt_read_all_rows(MYSQL_STMT *stmt)
@@ -195,13 +201,15 @@ int mthd_stmt_read_all_rows(MYSQL_STMT *stmt)
MYSQL_ROWS *current, **pprevious;
ulong packet_len;
unsigned char *p;
@ -653,7 +653,7 @@ index 8f5bddc..5bd0ece 100644
{
/* allocate space for rows */
if (!(current= (MYSQL_ROWS *)ma_alloc_root(&result->alloc, sizeof(MYSQL_ROWS) + packet_len)))
@@ -275,10 +283,14 @@ int mthd_stmt_read_all_rows(MYSQL_STMT *stmt)
@@ -276,10 +284,14 @@ int mthd_stmt_read_all_rows(MYSQL_STMT *stmt)
{
*pprevious= 0;
/* sace status info */
@ -672,7 +672,7 @@ index 8f5bddc..5bd0ece 100644
stmt->result_cursor= result->data;
return(0);
}
@@ -335,30 +347,52 @@ static int stmt_cursor_fetch(MYSQL_STMT *stmt, uchar **row)
@@ -336,30 +348,52 @@ static int stmt_cursor_fetch(MYSQL_STMT *stmt, uchar **row)
void mthd_stmt_flush_unbuffered(MYSQL_STMT *stmt)
{
ulong packet_len;
@ -712,9 +712,7 @@ index 8f5bddc..5bd0ece 100644
{
- if (mariadb_connection(stmt->mysql))
+ if (!in_resultset && *pos == 0) /* OK */
{
- stmt->mysql->server_status= uint2korr(pos + 3);
- if (in_resultset)
+ {
+ pos++;
+ net_field_length(&pos);
+ net_field_length(&pos);
@ -722,7 +720,9 @@ index 8f5bddc..5bd0ece 100644
+ goto end;
+ }
+ if (packet_len < 8 && *pos == 254) /* EOF */
+ {
{
- stmt->mysql->server_status= uint2korr(pos + 3);
- if (in_resultset)
+ if (mariadb_connection(stmt->mysql))
+ {
+ stmt->mysql->server_status= uint2korr(pos + 3);
@ -739,7 +739,7 @@ index 8f5bddc..5bd0ece 100644
}
}
end:
@@ -1552,7 +1586,7 @@ my_bool mthd_stmt_read_prepare_response(MYSQL_STMT *stmt)
@@ -1554,7 +1588,7 @@ my_bool mthd_stmt_read_prepare_response(MYSQL_STMT *stmt)
ulong packet_length;
uchar *p;
@ -748,20 +748,37 @@ index 8f5bddc..5bd0ece 100644
return(1);
p= (uchar *)stmt->mysql->net.read_pos;
@@ -1579,26 +1613,22 @@ my_bool mthd_stmt_read_prepare_response(MYSQL_STMT *stmt)
@@ -1579,28 +1613,38 @@ my_bool mthd_stmt_read_prepare_response(MYSQL_STMT *stmt)
return(0);
}
my_bool mthd_stmt_get_param_metadata(MYSQL_STMT *stmt)
{
-my_bool mthd_stmt_get_param_metadata(MYSQL_STMT *stmt)
-{
- MYSQL_DATA *result;
-
+my_bool mthd_stmt_get_param_metadata(MYSQL_STMT *stmt) {
+ /* We use locally allocated memory pool that we directly supply to
+ to `mthd_my_read_metadata_ex` instead of the previous
+ approach in which we simply called `mthd_my_read_metadata`
+ and the shared structure `mysql->field_alloc` was used
+ for this purpose. Using `mysql->field_alloc` was proven
+ to have dangerous side effects, as reported in issue #XXXX.
+ Alternatives solutions to this one were also provided in
+ the same issue, including `libmysql` approach. The local
+ memory allocation approach was selected as it's the most
+ defensive alternative for this.
+ */
+ MA_MEM_ROOT mt_alloc;
+ ma_init_alloc_root(&mt_alloc, 8192, 0); /* Assume rowlength < 8192 */
- if (!(result= stmt->mysql->methods->db_read_rows(stmt->mysql, (MYSQL_FIELD *)0,
- 7 + ma_extended_type_info_rows(stmt->mysql))))
+ if (!(mthd_my_read_metadata(stmt->mysql, stmt->param_count,
+ if (!(mthd_my_read_metadata_ex(stmt->mysql, &mt_alloc, stmt->param_count,
+ 7 + ma_extended_type_info_rows(stmt->mysql))))
return(1);
- free_rows(result);
+ ma_free_root(&stmt->mysql->field_alloc, MYF(0));
+ ma_free_root(&mt_alloc, MYF(0));
+
return(0);
}
@ -782,7 +799,7 @@ index 8f5bddc..5bd0ece 100644
return(1);
return(0);
}
@@ -1869,6 +1899,12 @@ int stmt_read_execute_response(MYSQL_STMT *stmt)
@@ -1871,6 +1915,12 @@ int stmt_read_execute_response(MYSQL_STMT *stmt)
if (!stmt->mysql)
return(1);

@ -0,0 +1,373 @@
/**
* @file reg_test_3434-stmt_metadata-t.cpp
* @brief This test is a regression test for issue #3434.
* @details This test executes a combination of prepared statements and text protocol
* queries to the same ProxySQL backend connection, to create the required flow to
* trigger issue #3434. The test tries to particularly stress the code related
* to metadata handling.
* @date 2021-05-04
*/
#include <vector>
#include <string>
#include <stdio.h>
#include <cstring>
#include <unistd.h>
#include <time.h>
#include <iostream>
#include <thread>
#include <mysql.h>
#include "proxysql_utils.h"
#include "tap.h"
#include "command_line.h"
#include "utils.h"
using std::string;
const int STRING_SIZE=32;
int g_seed = 0;
inline int fastrand() {
g_seed = (214013*g_seed+2531011);
return (g_seed>>16)&0x7FFF;
}
inline unsigned long long monotonic_time() {
struct timespec ts;
clock_gettime(CLOCK_MONOTONIC, &ts);
return (((unsigned long long) ts.tv_sec) * 1000000) + (ts.tv_nsec / 1000);
}
void gen_random_str(char *s, const int len) {
g_seed = monotonic_time() ^ getpid() ^ pthread_self();
static const char alphanum[] =
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
"abcdefghijklmnopqrstuvwxyz";
for (int i = 0; i < len; ++i) {
s[i] = alphanum[fastrand() % (sizeof(alphanum) - 1)];
}
s[len] = 0;
}
int perform_text_select(
const CommandLine& cl,
const std::string& query
) {
MYSQL* proxysql_text = mysql_init(NULL);
if (!proxysql_text) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql_text));
return EXIT_FAILURE;
}
if (!mysql_real_connect(proxysql_text, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql_text));
return EXIT_FAILURE;
}
MYSQL_QUERY(proxysql_text, query.c_str());
MYSQL_RES* result = mysql_store_result(proxysql_text);
mysql_free_result(result);
mysql_close(proxysql_text);
return EXIT_SUCCESS;
}
int perform_stmt_select(
const CommandLine& cl,
const std::string& query,
uint32_t num_query_params
) {
int res = EXIT_SUCCESS;
MYSQL* proxysql_mysql = mysql_init(NULL);
if (!proxysql_mysql) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql_mysql));
return EXIT_FAILURE;
}
if (!mysql_real_connect(proxysql_mysql, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql_mysql));
return EXIT_FAILURE;
}
MYSQL_STMT *stmt = mysql_stmt_init(proxysql_mysql);
if (!stmt) {
diag("mysql_stmt_init(), out of memory");
res = EXIT_FAILURE;
goto exit;
}
if (mysql_stmt_prepare(stmt, query.c_str(), strlen(query.c_str()))) {
diag("mysql_stmt_prepare at line %d failed: %s", __LINE__ , mysql_error(proxysql_mysql));
mysql_close(proxysql_mysql);
mysql_library_end();
res = EXIT_FAILURE;
goto exit;
}
{
std::vector<MYSQL_BIND> bind_params(num_query_params);
std::vector<int64_t> data_param(num_query_params, 0);
for (uint32_t i = 0; i < data_param.size(); i++) {
data_param[i] = i;
}
for (int i = 0; i < num_query_params; i++) {
memset(&bind_params[i], 0, sizeof(MYSQL_BIND));
bind_params[i].buffer_type = MYSQL_TYPE_LONGLONG;
bind_params[i].buffer = (char *)&data_param[i];
bind_params[i].buffer_length = sizeof(int64_t);
}
if (mysql_stmt_bind_param(stmt, &bind_params[0])) {
diag(
"mysql_stmt_bind_result at line %d failed: %s", __LINE__ ,
mysql_stmt_error(stmt)
);
res = EXIT_FAILURE;
goto exit;
}
if (mysql_stmt_execute(stmt)) {
diag(
"mysql_stmt_execute at line %d failed: %s", __LINE__ ,
mysql_stmt_error(stmt)
);
res = EXIT_FAILURE;
goto exit;
}
MYSQL_BIND bind[3];
int data_id;
int64_t data_c1;
char data_c2[STRING_SIZE];
char is_null[3];
long unsigned int length[3];
char error[3];
memset(bind, 0, sizeof(bind));
bind[0].buffer_type = MYSQL_TYPE_LONG;
bind[0].buffer = (char *)&data_id;
bind[0].buffer_length = sizeof(int);
bind[0].is_null = &is_null[0];
bind[0].length = &length[0];
bind[1].buffer_type = MYSQL_TYPE_LONGLONG;
bind[1].buffer = (char *)&data_c1;
bind[1].buffer_length = sizeof(int64_t);
bind[1].is_null = &is_null[1];
bind[1].length = &length[1];
bind[2].buffer_type = MYSQL_TYPE_STRING;
bind[2].buffer = (char *)&data_c2;
bind[2].buffer_length = STRING_SIZE;
bind[2].is_null = &is_null[2];
bind[2].length = &length[2];
bind[2].error = &error[2];
if (mysql_stmt_bind_result(stmt, bind)) {
diag(
"mysql_stmt_bind_result at line %d failed: %s", __LINE__,
mysql_stmt_error(stmt)
);
res = EXIT_FAILURE;
goto exit;
}
if (mysql_stmt_fetch(stmt) == 1) {
diag(
"mysql_stmt_fetch at line %d failed: %s", __LINE__,
mysql_stmt_error(stmt)
);
res = EXIT_FAILURE;
goto exit;
}
bool data_match_expected =
(data_id == static_cast<int64_t>(1)) &&
(data_c1 == static_cast<int64_t>(100)) &&
(strcmp(data_c2, "abcde") == 0);
if (data_match_expected == false) {
diag(
"Prepared statement SELECT result didn't matched expected -"
" Exp=(id:1, c1:100, c2:'abcde'), Act=(id:%d, c1:%ld, c2:'%s')",
data_id,
data_c1,
data_c2
);
res = EXIT_FAILURE;
goto exit;
}
}
exit:
if (stmt) { mysql_stmt_close(stmt); }
mysql_close(proxysql_mysql);
return res;
}
std::string build_random_select_query(
const std::string& rnd_table_name,
const uint32_t hostgroup,
const uint32_t num_params
) {
// Force the 'hostgroup' for the 'SELECT' query
std::string t_query {
"SELECT /*+ ;hostgroup=%d,%s */ * FROM %s WHERE id IN ("
};
for (uint32_t i = 0; i < num_params; i++) {
t_query += "?";
if (i != num_params - 1) {
t_query += ",";
}
}
t_query += ")";
std::string query {};
std::string rnd_str(static_cast<std::size_t>(20), '\0');
gen_random_str(&rnd_str[0], 20);
string_format(
t_query, query, hostgroup, rnd_str.c_str(), rnd_table_name.c_str()
);
return query;
}
uint32_t SELECT_PARAM_NUM = 20000;
uint32_t ITERATIONS = 10;
uint32_t HOSTGROUP = 0;
int main(int argc, char** argv) {
CommandLine cl;
if (cl.getEnv()) {
diag("Failed to get the required environmental variables.");
return -1;
}
plan(1);
MYSQL* proxysql_mysql = mysql_init(NULL);
if (!proxysql_mysql) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql_mysql));
return exit_status();
}
if (!mysql_real_connect(proxysql_mysql, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql_mysql));
return exit_status();
}
// Configure ProxySQL
// *************************************************************************
// We configure ProxySQL allowing a maximum of `1` connections to the backend server
// we are targetting for this test. This way we ensure that all the operations that
// trigger this specific bug are performed against the same backend connection.
// We know this because we are going to later impose this queries to be specifically
// redirected to the hostgroup of this backend.
MYSQL* proxysql_admin = mysql_init(NULL);
if (!proxysql_admin) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxysql_admin));
return exit_status();
}
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_status();
}
std::string t_update_servers_query {
"UPDATE mysql_servers SET max_connections=1 WHERE hostgroup_id=%d"
};
std::string update_servers_query {};
string_format(t_update_servers_query, update_servers_query, HOSTGROUP);
MYSQL_QUERY(proxysql_admin, update_servers_query.c_str());
MYSQL_QUERY(proxysql_admin, "LOAD MYSQL SERVERS TO RUNTIME");
sleep(2);
// *************************************************************************
// Insert data in the table to be queried
// *************************************************************************
MYSQL_QUERY(proxysql_mysql, "CREATE DATABASE IF NOT EXISTS test");
MYSQL_QUERY(proxysql_mysql, "DROP TABLE IF EXISTS test.reg_test_3434");
MYSQL_QUERY(
proxysql_mysql,
"CREATE TABLE IF NOT EXISTS test.reg_test_3434"
" (id INT NOT NULL AUTO_INCREMENT PRIMARY KEY, `c1` BIGINT, `c2` varchar(32))"
);
MYSQL_QUERY(proxysql_mysql, "INSERT INTO test.reg_test_3434 (c1, c2) VALUES (100, 'abcde')");
mysql_close(proxysql_mysql);
// *************************************************************************
// This test support supplying to it the number of iterations that should be
// performed, for debugging purposes. `1` iteration should be enough to
// trigger the memory corruption, and experimental tests show that after
// just `1` ProxySQL always crash. For safety by default we are leaving 10
// iterations.
if (argc == 2) {
ITERATIONS = std::atoi(argv[1]);
std::cout << "Supplied iterations were: " << ITERATIONS << "\n";
}
int query_res = 0;
// Force the 'hostgroup' for the 'SELECT' queries, to ensure that
// the connections are going to the same server, and thus,
// targetting the same backend connection, since we have reduced
// the maximum number of backend connections for this server to `1`.
for (int i = 0; i < ITERATIONS; i++) {
std::string query_1 {
build_random_select_query("test.reg_test_3434", HOSTGROUP, SELECT_PARAM_NUM)
};
std::string text_query_1 {};
std::string::size_type pos = query_1.find("WHERE");
if (pos == std::string::npos) {
text_query_1 = "SELECT 1";
} else {
text_query_1 = query_1.substr(0, pos);
}
query_res = perform_stmt_select(cl, query_1, SELECT_PARAM_NUM);
if (query_res != EXIT_SUCCESS) { break; }
query_res = perform_text_select(cl, text_query_1);
if (query_res != EXIT_SUCCESS) { break; }
std::string query_2 {
build_random_select_query("test.reg_test_3434", HOSTGROUP, SELECT_PARAM_NUM)
};
std::string text_query_2 {};
pos = query_2.find("WHERE");
if (pos == std::string::npos) {
text_query_2 = "SELECT 1";
} else {
text_query_2 = query_1.substr(0, pos);
}
query_res = perform_stmt_select(cl, query_2, SELECT_PARAM_NUM);
if (query_res != EXIT_SUCCESS) { break; }
}
ok(query_res == EXIT_SUCCESS, "Check that none of the queries failed to be executed.");
mysql_close(proxysql_admin);
return exit_status();
}
Loading…
Cancel
Save