Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-9997

CONNECT: memory leak after connect.alter connect.ini_grant

Details

    Description

      This is failing since quite a while in buildbot: http://buildbot.askmonty.org/buildbot/builders/work-amd64-valgrind/builds/8698/steps/test/logs/stdio (look for PROFILE_Open).

      This memory leak is easily reproducible:

      BUILD/compile-amd64-valgrind-max &&
      cd mysql-test &&
      ./mtr --no-reorder --valgrind connect.alter connect.ini_grant
      

      Even simpler test basing on previous two:

      CREATE TABLE t1 (c INT NOT NULL, d CHAR(10) NOT NULL) ENGINE=CONNECT TABLE_TYPE=fix FILE_NAME='tf1.txt' ENDING=1;
      ALTER TABLE t1 ENGINE=ARIA;
      DROP TABLE t1;
       
      CREATE TABLE t1 (sec CHAR(10) NOT NULL FLAG=1, val CHAR(10) NOT NULL) ENGINE=CONNECT TABLE_TYPE=INI FILE_NAME='t1.EXT';
      INSERT INTO t1 VALUES ('sec1','val1');
      DROP TABLE t1;
      

      Valgrind warnings:

      ==28595== 320 bytes in 10 blocks are still reachable in loss record 1 of 3
      ==28595==    at 0x4C2BC9F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
      ==28595==    by 0xC755E6: PROFILE_Open (inihandl.c:455)
      ==28595==    by 0xC76C95: PROFILE_GetPrivateProfileString (inihandl.c:1137)
      ==28595==    by 0xC76D74: GetPrivateProfileString (inihandl.c:1165)
      ==28595==    by 0xC43728: TDBINI::GetSeclist(_global*) (tabsys.cpp:189)
      ==28595==    by 0xC438F9: TDBINI::OpenDB(_global*) (tabsys.cpp:260)
      ==28595==    by 0xBF95DD: CntOpenTable(_global*, TDB*, MODE, char*, char*, bool, ha_connect*) (connect.cc:360)
      ==28595==    by 0xBE94A0: ha_connect::OpenTable(_global*, bool) (ha_connect.cc:1823)
      ==28595==    by 0xBEE54F: ha_connect::write_row(unsigned char*) (ha_connect.cc:3251)
      ==28595==    by 0x877313: handler::ha_write_row(unsigned char*) (handler.cc:6005)
      ==28595==    by 0x636E54: write_record(THD*, TABLE*, st_copy_info*) (sql_insert.cc:1847)
      ==28595==    by 0x634960: mysql_insert(THD*, TABLE_LIST*, List<Item>&, List<List<Item> >&, List<Item>&, List<Item>&, enum_duplicates, bool) (sql_insert.cc:964)
      ==28595==    by 0x656638: mysql_execute_command(THD*) (sql_parse.cc:3447)
      ==28595==    by 0x65F289: mysql_parse(THD*, char*, unsigned int, Parser_state*) (sql_parse.cc:6554)
      ==28595==    by 0x651280: dispatch_command(enum_server_command, THD*, char*, unsigned int) (sql_parse.cc:1308)
      ==28595==    by 0x65040A: do_command(THD*) (sql_parse.cc:998)
      

      This happens likely due to fact that MRUProfile is never freed:

      commit db77e64351310ce76c51f80c00a061cdc777232a
      Author: Olivier Bertrand <bertrandop@gmail.com>
      Date:   Wed Mar 19 02:25:28 2014 +0100
       
          - Suppress call to PROFILE_End in connect_done_func that causes Signal 11 on Linux
          modified:
            storage/connect/ha_connect.cc
       
      diff --git a/storage/connect/ha_connect.cc b/storage/connect/ha_connect.cc
      index 09917c8..ec8ed16 100644
      --- a/storage/connect/ha_connect.cc
      +++ b/storage/connect/ha_connect.cc
      @@ -378,7 +378,7 @@ static int connect_done_func(void *p)
       #endif   // LIBXML2_SUPPORT
       
       #if !defined(WIN32)
      -  PROFILE_End();
      +//PROFILE_End();                Causes signal 11
       #endif   // !WIN32
       
         for (pc= user_connect::to_users; pc; pc= pn) {
      

      But memory leak is in no way good workaround for SIGSEGV.

      Attachments

        Issue Links

          Activity

            Do you mean that restoring the call to PROFILE_END() solve the problem?

            bertrandop Olivier Bertrand added a comment - Do you mean that restoring the call to PROFILE_END() solve the problem?

            I didn't try. But FWICS PROFILE_End() is the only function that frees MRUProfile. I can try if you like.

            svoj Sergey Vojtovich added a comment - I didn't try. But FWICS PROFILE_End() is the only function that frees MRUProfile. I can try if you like.

            Yes please. As a matter of fact, I don't know what MRUProfile is and how it is connected to these CONNECT tests. Besides, what make you think that it is the item that is not freed?

            Note that because the connect_done_func is only called when the system is shutting down, this memory leak may not be really important. This once more demonstrates the stupidity of these signal raised by some ASSERT MariaDB is full of. ASSERTs, according to all programming experts, is a bad habit that in fact hides the errors, lazily avoid to seach for the cause of them, and prevents to fix them or at least to produce an explicite error message.

            bertrandop Olivier Bertrand added a comment - Yes please. As a matter of fact, I don't know what MRUProfile is and how it is connected to these CONNECT tests. Besides, what make you think that it is the item that is not freed? Note that because the connect_done_func is only called when the system is shutting down, this memory leak may not be really important. This once more demonstrates the stupidity of these signal raised by some ASSERT MariaDB is full of. ASSERTs, according to all programming experts, is a bad habit that in fact hides the errors, lazily avoid to seach for the cause of them, and prevents to fix them or at least to produce an explicite error message.

            I confirm that uncommenting PROFILE_End() in connect_done_func() solves the problem. However I had to also comment out "CurProfile = MRUProfile[i];" and "PROFILE_ReleaseFile();" in PROFILE_End() to make it work properly.

            Besides, what make you think that it is the item that is not freed?
            "Valgrind warnings" above point to "PROFILE_Open (inihandl.c:455)", which is "MRUProfile[i] = malloc(sizeof(PROFILE));"

            Note that because the connect_done_func is only called when the system is shutting down, this memory leak may not be really important.
            Strictly speaking connect_done_func() doesn't mean system shutdown, it doesn't even mean MariaDB server shutdown. It means CONNECT engine shutdown. Technically it might be not that important. But it costs us reputation: even if we suppress this leak for valgrind, sooner or later users will find this leak with different memory analysis tool.

            This once more demonstrates the stupidity of these signal raised by some ASSERT MariaDB is full of.
            How do asserts relate to this bug?

            ASSERTs, according to all programming experts, is a bad habit that in fact hides the errors
            How is it possible to "hide an error" with assert?

            The point of asserts in MariaDB is not "hidding errors" or "avoiding to search for them". Asserts issue additional checks in debug builds, which for performance reason are not issued in release builds.

            svoj Sergey Vojtovich added a comment - I confirm that uncommenting PROFILE_End() in connect_done_func() solves the problem. However I had to also comment out "CurProfile = MRUProfile[i];" and "PROFILE_ReleaseFile();" in PROFILE_End() to make it work properly. Besides, what make you think that it is the item that is not freed ? "Valgrind warnings" above point to "PROFILE_Open (inihandl.c:455)", which is "MRUProfile[i] = malloc(sizeof(PROFILE));" Note that because the connect_done_func is only called when the system is shutting down, this memory leak may not be really important. Strictly speaking connect_done_func() doesn't mean system shutdown, it doesn't even mean MariaDB server shutdown. It means CONNECT engine shutdown. Technically it might be not that important. But it costs us reputation: even if we suppress this leak for valgrind, sooner or later users will find this leak with different memory analysis tool. This once more demonstrates the stupidity of these signal raised by some ASSERT MariaDB is full of. How do asserts relate to this bug? ASSERTs, according to all programming experts, is a bad habit that in fact hides the errors How is it possible to "hide an error" with assert? The point of asserts in MariaDB is not "hidding errors" or "avoiding to search for them". Asserts issue additional checks in debug builds, which for performance reason are not issued in release builds.
            bertrandop Olivier Bertrand added a comment - - edited

            Good and thank you. I shall do this and hopefully fix this bug.

            inihandl.c: As a matter of fact, this code is meant to emulate Windows functions not existing on Unix/Linux. It was imported ten years ago in a previous program of mine and I don't know who wrote it. I have never tried to understand in detail what it does, working almost only on Windows.

            Signal was the reason why I commented out this line but apparently this did not happen when you restored the previous version so the reason why it raised a signal has probably been fixed (perhaps by your commenting out these two lines) Perhaps this stuff could be just removed on Linux distributions.

            About assertion see for example https://www.softwariness.com/articles/assertions-in-cpp/

            bertrandop Olivier Bertrand added a comment - - edited Good and thank you. I shall do this and hopefully fix this bug. inihandl.c: As a matter of fact, this code is meant to emulate Windows functions not existing on Unix/Linux. It was imported ten years ago in a previous program of mine and I don't know who wrote it. I have never tried to understand in detail what it does, working almost only on Windows. Signal was the reason why I commented out this line but apparently this did not happen when you restored the previous version so the reason why it raised a signal has probably been fixed (perhaps by your commenting out these two lines) Perhaps this stuff could be just removed on Linux distributions. About assertion see for example https://www.softwariness.com/articles/assertions-in-cpp/

            Thanks for accepting it. Please also note that MRUProfile[0] can be NULL and other array elements can be uninitialized in PROFILE_End(). The change should look something like this:

            diff --git a/storage/connect/inihandl.c b/storage/connect/inihandl.c
            index 542b807..9ec566c 100644
            --- a/storage/connect/inihandl.c
            +++ b/storage/connect/inihandl.c
            @@ -622,13 +622,14 @@ void PROFILE_End(void)
               if (trace)
                 htrc("PROFILE_End: CurProfile=%p N=%d\n", CurProfile, N_CACHED_PROFILES);
             
            +  if (!CurProfile)
            +    return;
            +
               /* Close all opened files and free the cache structure */
               for (i = 0; i < N_CACHED_PROFILES; i++) {
                 if (trace)
                   htrc("MRU=%s i=%d\n", SVP(MRUProfile[i]->filename), i);
             
            -    CurProfile = MRUProfile[i];
            -    PROFILE_ReleaseFile();
                 free(MRUProfile[i]);
                 } // endfor i
            
            

            svoj Sergey Vojtovich added a comment - Thanks for accepting it. Please also note that MRUProfile [0] can be NULL and other array elements can be uninitialized in PROFILE_End(). The change should look something like this: diff --git a/storage/connect/inihandl.c b/storage/connect/inihandl.c index 542b807..9ec566c 100644 --- a/storage/connect/inihandl.c +++ b/storage/connect/inihandl.c @@ -622,13 +622,14 @@ void PROFILE_End(void) if (trace) htrc("PROFILE_End: CurProfile=%p N=%d\n", CurProfile, N_CACHED_PROFILES);   + if (!CurProfile) + return; + /* Close all opened files and free the cache structure */ for (i = 0; i < N_CACHED_PROFILES; i++) { if (trace) htrc("MRU=%s i=%d\n", SVP(MRUProfile[i]->filename), i);   - CurProfile = MRUProfile[i]; - PROFILE_ReleaseFile(); free(MRUProfile[i]); } // endfor i

            Thanks to Sergey Vojtovich

            bertrandop Olivier Bertrand added a comment - Thanks to Sergey Vojtovich

            People

              bertrandop Olivier Bertrand
              svoj Sergey Vojtovich
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.