Possible memory leak in RDBSE_KEYDEF::setup

[hermanlee]

Issue by spetrunia
Monday Sep 14, 2015 at 10:17 GMT
Originally opened as MySQLOnRocksDB#110

---

The code there has this comment:

void RDBSE_KEYDEF::setup(TABLE *tbl)
{
  /*
    set max_length based on the table. If we're unlucky, setup() may be
    called concurrently from multiple threads but that is ok because result of
    compuation is assignment of maxlength to the same value.
    ^^ TODO: is this still true? concurrent setup() calls are not safe
    anymore...

The suspicion is true. RDBSE_KEYDEF::setup() is called from ha_rocksdb::open(). Calls to that function are not protected by any SQL-layer mutex.

One can apply this patch:

diff --git a/storage/rocksdb/rdb_datadic.cc b/storage/rocksdb/rdb_datadic.cc
index 8f3b58b..3e69e4b 100644
--- a/storage/rocksdb/rdb_datadic.cc
+++ b/storage/rocksdb/rdb_datadic.cc
@@ -28,6 +28,8 @@
 #include "ha_rocksdb_proto.h"
 #include "my_stacktrace.h"

+#include "debug_sync.h"
+
 void key_restore(uchar *to_record, uchar *from_key, KEY *key_info,
                  uint key_length);

@@ -198,6 +203,7 @@ void RDBSE_KEYDEF::setup(TABLE *tbl)

     size_t size= sizeof(Field_pack_info) * m_key_parts;
     pack_info= (Field_pack_info*)my_malloc(size, MYF(0));
+    DEBUG_SYNC(current_thd, "rdbse_keydef_setup");

     size_t max_len= INDEX_NUMBER_SIZE;
     int unpack_len= 0;

and then run this testcase

--source include/have_rocksdb.inc
--source include/have_debug_sync.inc

create table t1 (pk int primary key, a int) engine=rocksdb;

--enable_connect_log

connect (con1,localhost,root,,);

--connection con1
set DEBUG_SYNC='rdbse_keydef_setup WAIT_FOR test1';
send insert into t1 values (1,1);

--connection default
insert into t1 values (2,2);
set DEBUG_SYNC='now SIGNAL test1';
select 1;

--connection con1
reap;

drop table t1;

and it produces a memory leak. Under valgrind, the leak is visible like this:

72 bytes in 1 blocks are definitely lost in loss record 8 of 17
   at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0xD5614E: my_malloc (my_malloc.c:38)
   by 0xDA8BFC: RDBSE_KEYDEF::setup(TABLE*) (rdb_datadic.cc:205)
   by 0xD826C2: ha_rocksdb::open(char const*, int, unsigned int) (ha_rocksdb.cc:2736)
   by 0x8A0228: handler::ha_open(TABLE*, char const*, int, int) (handler.cc:2627)
   by 0xB21310: open_table_from_share(THD*, TABLE_SHARE*, char const*, unsigned int, unsigned int, unsigned int, TABLE*, bool) (table.cc:2636)
   by 0x9E567E: open_table(THD*, TABLE_LIST*, Open_table_context*) (sql_base.cc:3176)
   by 0x9E7CD7: open_and_process_table(THD*, LEX*, TABLE_LIST*, unsigned int*, unsigned int, Prelocking_strategy*, bool, Open_table_context*) (s
ql_base.cc:4674)
   by 0x9E89A9: open_tables(THD*, TABLE_LIST**, unsigned int*, unsigned int, Prelocking_strategy*) (sql_base.cc:5108)
   by 0x9E9B54: open_normal_and_derived_tables(THD*, TABLE_LIST*, unsigned int) (sql_base.cc:5821)
   by 0xA339D0: mysql_insert(THD*, TABLE_LIST*, List<Item>&, List<List<Item> >&, List<Item>&, List<Item>&, enum_duplicates, bool) (sql_insert.cc
:695)
   by 0xA57279: mysql_execute_command(THD*, unsigned long long*, unsigned long long*) (sql_parse.cc:3969)
   by 0xA5F836: mysql_parse(THD*, char*, unsigned int, Parser_state*, unsigned long long*, char*) (sql_parse.cc:7138)
   by 0xA51463: dispatch_command(enum_server_command, THD*, char*, unsigned int) (sql_parse.cc:1531)
   by 0xA5002A: do_command(THD*) (sql_parse.cc:1076)
   by 0xA156F3: do_handle_one_connection(THD*) (sql_connect.cc:1071)

[jkedgar]

https://reviews.facebook.net/D48801

Note: the example presented above will no longer run, as the wait (DEBUG_SYNC) is now called while holding a mutex and the code to release the mutex won't get hit until after a different connection gets and releases the mutex. If you try to run this, you will get a deadlock.