From 1037977895aa4a145de16719df0a2375c71bbf26 Mon Sep 17 00:00:00 2001 From: Nisha Gopalakrishnan Date: Mon, 21 Jul 2014 21:21:15 +0530 Subject: [PATCH] BUG#17512527: LIST HANDLING INCORRECT IN MYSQL_PRUNE_STMT_LIST() Analysis: --------- Invalid memory access maybe observed when using prepared statements if: a) The mysql client connection is lost after statement preparation is complete and b) There is at least one statement which is in initialized state but not prepared yet. When the client detects a closed connection, it calls end_server() to shutdown the connection. As part of the clean up, the mysql_prune_stmt_list() removes the statements which has transitioned beyond the initialized state and retains only the statements which are in a initialized state. During this processing, the initialized statements are moved from 'mysql->stmts' to a temporary 'pruned_list'. When moving the first 'INIT_DONE' element to the pruned_list, 'element->next' is set to NULL. Hence the rest of the list is never traversed and the statements which have transitioned beyond the initialized state are never invalidated. When the mysql_stmt_close() is called for the statement which is not invalidated; the statements list is updated in order to remove the statement. This would end up accessing freed memory(freed by the mysql_stmt_close() for a previous statement in the list). Fix: --- mysql_prune_stmt_list() called list_add() incorrectly to create a temporary list. The use case of list_add() is to add a single element to the front of the doubly linked list. mysql_prune_stmt_list() called list_add() by passing an entire list as the 'element'. mysql_prune_stmt_list() now uses list_delete() to remove the statement which has transitioned beyond the initialized phase. Thus the statement list would contain only elements where the the state of the statement is initialized. Note: Run the test with valgrind-mysqltest and leak-check=full option to see the invalid memory access. Back-ported to MySQL 5.5 branch by Balint Reczey Conflicts: sql-common/client.c tests/mysql_client_test.c --- sql-common/client.c | 11 +++++++---- tests/mysql_client_test.c | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/sql-common/client.c b/sql-common/client.c index cd9b6a7..be60cc1 100644 --- a/sql-common/client.c +++ b/sql-common/client.c @@ -3790,12 +3790,15 @@ static void mysql_close_free(MYSQL *mysql) */ static void mysql_prune_stmt_list(MYSQL *mysql) { - LIST *element= mysql->stmts; - LIST *pruned_list= 0; + LIST *pruned_list= NULL; - for (; element; element= element->next) + while(mysql->stmts) { - MYSQL_STMT *stmt= (MYSQL_STMT *) element->data; + LIST *element= mysql->stmts; + MYSQL_STMT *stmt; + + mysql->stmts= list_delete(element, element); + stmt= (MYSQL_STMT *) element->data; if (stmt->state != MYSQL_STMT_INIT_DONE) { stmt->mysql= 0; diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c index e600d82..d3f3899 100644 --- a/tests/mysql_client_test.c +++ b/tests/mysql_client_test.c @@ -18648,6 +18648,46 @@ static void test_bug13001491() myquery(rc); } +static void test_bug17512527() +{ + MYSQL *conn1, *conn2; + MYSQL_STMT *stmt1, *stmt2; + const char *stmt1_txt= "SELECT NOW();"; + const char *stmt2_txt= "SELECT 1;"; + unsigned long thread_id; + char query[MAX_TEST_QUERY_LENGTH]; + int rc; + + conn1= client_connect(0, MYSQL_PROTOCOL_DEFAULT, 1); + conn2= client_connect(0, MYSQL_PROTOCOL_DEFAULT, 0); + + stmt1 = mysql_stmt_init(conn1); + check_stmt(stmt1); + rc= mysql_stmt_prepare(stmt1, stmt1_txt, strlen(stmt1_txt)); + check_execute(stmt1, rc); + + thread_id= mysql_thread_id(conn1); + sprintf(query, "KILL %lu", thread_id); + if (thread_query(query)) + exit(1); + + /* + After the connection is killed, the connection is + re-established due to the reconnect flag. + */ + stmt2 = mysql_stmt_init(conn1); + check_stmt(stmt2); + + rc= mysql_stmt_prepare(stmt2, stmt2_txt, strlen(stmt2_txt)); + check_execute(stmt1, rc); + + mysql_stmt_close(stmt2); + mysql_stmt_close(stmt1); + + mysql_close(conn1); + mysql_close(conn2); +} + static struct my_tests_st my_tests[]= { { "disable_query_logs", disable_query_logs }, @@ -18911,6 +18951,7 @@ static struct my_tests_st my_tests[]= { { "test_bug12337762", test_bug12337762 }, { "test_bug11754979", test_bug11754979 }, { "test_bug13001491", test_bug13001491 }, + { "test_bug17512527", test_bug17512527}, { 0, 0 } }; -- 2.1.4