Bug #99112 When using mysql_real_connect_nonblocking a memory leak can occur
Submitted: 30 Mar 2020 23:27 Modified: 11 May 2020 17:53
Reporter: Jay Edgar Email Updates:
Status: Closed Impact on me:
None 
Category:MySQL Server: C API (client library) Severity:S3 (Non-critical)
Version:8.0.19 OS:Any
Assigned to: CPU Architecture:Any

[30 Mar 2020 23:27] Jay Edgar
Description:
The mysql_real_connect_nonblocking allocates a buffer in mysql_async_context->connect_context.  Normally when the connect call finishes this gets freed.  However, with the nonblocking call the caller is not guaranteed to keep calling until the connection completes.  If the connection takes too long the caller may give up.

The caller then should call mysql_close() to free all memory on the MYSQL*.  However this particular buffer is not freed.

How to repeat:
Apply the fixes for #98177, #98574, and #98980 and then add the following test:

diff --git a/testclients/mysql_client_test.cc b/testclients/mysql_client_test.cc
index 9e558664abf..c36b79b03db 100644
--- a/testclients/mysql_client_test.cc
+++ b/testclients/mysql_client_test.cc
@@ -20165,6 +20165,26 @@ static void test_bug27443252() {

 void perform_arithmatic() { fprintf(stdout, "\n Do some other stuff."); }

+static void test_crash() {
+  MYSQL *mysql_local;
+  net_async_status status;
+
+  /*make new non blocking connection to do asynchronous operations */
+  if (!(mysql_local = mysql_client_init(NULL))) {
+    myerror("mysql_client_init() failed");
+    exit(1);
+  }
+
+  status = mysql_real_connect_nonblocking(mysql_local, "0100::", opt_user, opt_password, current_db, opt_port, opt_unix_socket, CLIENT_MULTI_STATEMENTS);
+  if (status != NET_ASYNC_NOT_READY) {
+    myerror("Unexpected result from mysql_real_connect_nonblocking()");
+    exit(1);
+  }
+
+  // See if we can close without crashing
+  mysql_close(mysql_local);
+}
+
 static void test_wl11381() {
   MYSQL *mysql_local;
   MYSQL_RES *result;
@@ -20874,6 +20894,7 @@ static struct my_tests_st my_tests[] = {
     {"test_wl11772", test_wl11772},
     {"test_wl12475", test_wl12475},
     {"test_bug30032302", test_bug30032302},
+    {"test_crash", test_crash},
     {0, 0}};

 static struct my_tests_st *get_my_tests() { return my_tests; }

In my tests with ASAN (address sanitizer) it told me that a leak of 704 bytes occurred.  I tracked it down the above mentioned buffer.  There may be other buffers allocated at later stages in the connection process that also need to be checked.  This one occurs when the host/port we are attempting to connect to does not respond.

Suggested fix:
diff --git a/sql-common/client.cc b/sql-common/client.cc
index d2ac9e7f68e..5153dfdd837 100644
--- a/sql-common/client.cc
+++ b/sql-common/client.cc
@@ -3350,7 +3350,11 @@ MYSQL_EXTENSION *mysql_extension_init(MYSQL *mysql MY_ATTRIBUTE((unused))) {
 void mysql_extension_free(MYSQL_EXTENSION *ext) {
   if (!ext) return;
   if (ext->trace_data) my_free(ext->trace_data);
-  if (ext->mysql_async_context) my_free(ext->mysql_async_context);
+  if (ext->mysql_async_context) {
+    if (ext->mysql_async_context->connect_context)
+      my_free(ext->mysql_async_context->connect_context);
+    my_free(ext->mysql_async_context);
+  }
   // free state change related resources.
   free_state_change_info(ext);
[31 Mar 2020 0:58] Jay Edgar
It turns out I submitted this too soon.  There is another change needed in order to avoid a double-free.  The `my_free(ctx);` at the end of mysql_real_connect_nonblocking() in the error case needs to be removed because the mysql_close_free() call a few lines earlier now frees the same buffer.

diff --git a/sql-common/client.cc b/sql-common/client.cc
index 5153dfdd837..2c052675c18 100644
--- a/sql-common/client.cc
+++ b/sql-common/client.cc
@@ -5907,7 +5907,6 @@ net_async_status STDCALL mysql_real_connect_nonblocking(
       my_free(ctx->scramble_buffer);
       ctx->scramble_buffer = nullptr;
     }
-    my_free(ctx);
     DBUG_RETURN(NET_ASYNC_ERROR);
   }
 }
[31 Mar 2020 14:20] MySQL Verification Team
Hi Mr. Edgar,

Thank you very much for your bug report.

I have analysed your patches and I agree with you fully.

Verified as reported.
[2 Apr 2020 20:43] Jay Edgar
Further testing with Valgrind shows that we need to move the mysql_close_free() call since the memory is still being accessed.

diff --git a/sql-common/client.cc b/sql-common/client.cc
index 2c052675c18..b8b2a48afc4 100644
--- a/sql-common/client.cc
+++ b/sql-common/client.cc
@@ -5900,13 +5900,13 @@ net_async_status STDCALL mysql_real_connect_nonblocking(
                          mysql->net.sqlstate, mysql->net.last_error));
     /* Free alloced memory */
     end_server(mysql);
-    mysql_close_free(mysql);
     if (!(ctx->client_flag & CLIENT_REMEMBER_OPTIONS))
       mysql_close_free_options(mysql);
     if (ctx->scramble_buffer_allocated) {
       my_free(ctx->scramble_buffer);
       ctx->scramble_buffer = nullptr;
     }
+    mysql_close_free(mysql);
     DBUG_RETURN(NET_ASYNC_ERROR);
   }
 }
[3 Apr 2020 12:14] MySQL Verification Team
Thank you, Mr. Edgar.
[11 May 2020 17:53] Paul DuBois
Posted by developer:
 
Fixed in 8.0.21.

Calling mysql_real_connect_nonblocking() with an invalid host could
cause the client to exit upon calling mysql_close().
[12 May 2020 12:15] MySQL Verification Team
Thank you, Paul.