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

ASAN: main.metadata and user_variables.basic MTR failures after MDEV-26572

Details

    • Bug
    • Status: Closed (View Workflow)
    • Blocker
    • Resolution: Fixed
    • 10.7(EOL)
    • 10.7.1
    • OTHER
    • None

    Description

      After MDEV-26572 two tests fail with an ASAN build:

      READ of size 4 at 0x6290000e6f60 thread T5
          #0 0x558dcc06be73 in my_strcoll_ascii_4bytes_found /home/buildbot/buildbot/build/mariadb-10.7.0/strings/ctype-ascii.h:111
          #1 0x558dcc06d45a in my_strnncoll_utf8mb3_general_ci /home/buildbot/buildbot/build/mariadb-10.7.0/strings/strcoll.ic:226
          #2 0x558dcbf06683 in hashcmp /home/buildbot/buildbot/build/mariadb-10.7.0/mysys/hash.c:373
          #3 0x558dcbf05ef6 in my_hash_first_from_hash_value /home/buildbot/buildbot/build/mariadb-10.7.0/mysys/hash.c:288
          #4 0x558dcbf05d11 in my_hash_first /home/buildbot/buildbot/build/mariadb-10.7.0/mysys/hash.c:262
          #5 0x558dcbf05a04 in my_hash_search /home/buildbot/buildbot/build/mariadb-10.7.0/mysys/hash.c:235
          #6 0x558dcac84412 in get_variable(st_hash*, st_mysql_const_lex_string*, bool) /home/buildbot/buildbot/build/mariadb-10.7.0/sql/item_func.cc:4634
          #7 0x558dcac84924 in Item_func_set_user_var::set_entry(THD*, bool) /home/buildbot/buildbot/build/mariadb-10.7.0/sql/item_func.cc:4687
      

      This is a minimum reproducible script:

      SET NAMES utf8;
      set @arg00=1;
      SELECT @:=1e0;
      

      The problem was not really introduced by MDEV-26572, it only revealed the fact that hashcmp() in mysys/hash.c asubes strnncoll():

        NOTES:
          If length is 0, comparison is done using the length of the
          record being compared against.
      

      This quick patch fixes the problem:

      diff --git a/mysys/hash.c b/mysys/hash.c
      index abc11b42500..c0c60a8117d 100644
      --- a/mysys/hash.c
      +++ b/mysys/hash.c
      @@ -369,6 +369,22 @@ static int hashcmp(const HASH *hash, HASH_LINK *pos, const uchar *key,
       {
         size_t rec_keylength;
         uchar *rec_key= (uchar*) my_hash_key(hash, pos->data, &rec_keylength, 1);
      +  if (hash->charset != &my_charset_bin)
      +  {
      +    /*
      +      The "length == rec_keylength" is needed for backward compatibility,
      +      to avoid things like partition names and plugin names to be
      +      accent insensitive. We should eventually:
      +      - Add a new utf8 accent sensitive case insensitive collation
      +      - Change SQL object names (plugins, partitions, etc) to use
      +        this new collation.
      +      - Remove the length test from here
      +    */
      +    return length != rec_keylength ||
      +           my_strnncoll(hash->charset,
      +                        (uchar*) rec_key, rec_keylength,
      +                        (uchar*) key, length);
      +  }
         return ((length && length != rec_keylength) ||
                my_strnncoll(hash->charset, (uchar*) rec_key, rec_keylength,
                             (uchar*) key, rec_keylength));
      

      This patch assumes that the length "0" is passed only on binary data, while in case of text string data a correct length value is passed.

      All MTR tests passed with the patch. So this assumption seems to be true.

      But the patch slows down a very hot function by adding a new if testing hash->charset . It should not be pushed AS IS. It's only a demo showing where the real problem is.

      The real patch should fix all my_hash_search() calls not to pass 0 as length.

      Attachments

        Issue Links

          Activity

            It would be nice to figure out who need and why we allow user variables with an empty name (zero length name)

            sanja Oleksandr Byelkin added a comment - It would be nice to figure out who need and why we allow user variables with an empty name (zero length name)
            bar Alexander Barkov added a comment - - edited

            I think fixing the code not to pass 0 meaning "use length of the other side from hash" would be a more reliable solution. That would fix the source of the problem. It's absolutely for sure that hash abuses the function strnncoll(), which is not supposed to be called this way. strnncoll() requires precise length.

            On the contrary, disallowing user variables with empty names would fix only one of the visible consequence. We cannot know how many other invisible consequences we have.

            bar Alexander Barkov added a comment - - edited I think fixing the code not to pass 0 meaning "use length of the other side from hash" would be a more reliable solution. That would fix the source of the problem. It's absolutely for sure that hash abuses the function strnncoll(), which is not supposed to be called this way. strnncoll() requires precise length. On the contrary, disallowing user variables with empty names would fix only one of the visible consequence. We cannot know how many other invisible consequences we have.

            It is patches as result of the bug investigation. I think they should be moved to 10.2

            Only "hash" fix is obligatory fix, all other is fixing "strange things" found.

            commit f64531a17b5b0828d04e226fd47ed5f7f36840fc (HEAD -> bb-10.7-MDEV-26637, ori
            gin/bb-10.7-MDEV-26637)
            Author: Oleksandr Byelkin <sanja@mariadb.com>
            Date:   Thu Sep 30 12:45:19 2021 +0200
             
                MDEV-26637: (hash) ASAN: main.metadata and user_variables.basic MTR failures
             after MDEV-26572
                
                Explicitly devide two function of 0 length in the hash keys comparing.
             
            commit 4a3834e249769778ed81eab2bb97b7792fa4a039
            Author: Oleksandr Byelkin <sanja@mariadb.com>
            Date:   Thu Sep 30 11:45:51 2021 +0200
             
                MDEV-26637: (roles) ASAN: main.metadata and user_variables.basic MTR failures after MDEV-26572
                
                No empty name roles exists (no sens to look for).
             
            commit a3351a6248ff37e42c00f054c76f7349449cc58c
            Author: Oleksandr Byelkin <sanja@mariadb.com>
            Date:   Thu Sep 30 10:56:45 2021 +0200
             
                MDEV-26637: (variables) ASAN: main.metadata and user_variables.basic MTR failures after MDEV-26572
                
                Prohibit user variables without name
             
            commit 2544edd01b41857ff16120ecd5f1ce7cec476d7c
            Author: Oleksandr Byelkin <sanja@mariadb.com>
            Date:   Thu Sep 30 10:14:56 2021 +0200
             
                MDEV-26637: (plugin) ASAN: main.metadata and user_variables.basic MTR failures after MDEV-26572
                
                Get rid of locking "empty" plugin
            commit 39a90aad13e30e4c4a7d6046583a1b48a3605a2c
            Author: Oleksandr Byelkin <sanja@mariadb.com>
            Date:   Thu Sep 30 10:14:28 2021 +0200
             
                MDEV-26637: (explicit length) ASAN: main.metadata and user_variables.basic MTR failures after MDEV-26572
                
                Use explicit length for hash record length
            

            sanja Oleksandr Byelkin added a comment - It is patches as result of the bug investigation. I think they should be moved to 10.2 Only "hash" fix is obligatory fix, all other is fixing "strange things" found. commit f64531a17b5b0828d04e226fd47ed5f7f36840fc (HEAD -> bb-10.7-MDEV-26637, ori gin/bb-10.7-MDEV-26637) Author: Oleksandr Byelkin <sanja@mariadb.com> Date: Thu Sep 30 12:45:19 2021 +0200   MDEV-26637: (hash) ASAN: main.metadata and user_variables.basic MTR failures after MDEV-26572 Explicitly devide two function of 0 length in the hash keys comparing.   commit 4a3834e249769778ed81eab2bb97b7792fa4a039 Author: Oleksandr Byelkin <sanja@mariadb.com> Date: Thu Sep 30 11:45:51 2021 +0200   MDEV-26637: (roles) ASAN: main.metadata and user_variables.basic MTR failures after MDEV-26572 No empty name roles exists (no sens to look for).   commit a3351a6248ff37e42c00f054c76f7349449cc58c Author: Oleksandr Byelkin <sanja@mariadb.com> Date: Thu Sep 30 10:56:45 2021 +0200   MDEV-26637: (variables) ASAN: main.metadata and user_variables.basic MTR failures after MDEV-26572 Prohibit user variables without name   commit 2544edd01b41857ff16120ecd5f1ce7cec476d7c Author: Oleksandr Byelkin <sanja@mariadb.com> Date: Thu Sep 30 10:14:56 2021 +0200   MDEV-26637: (plugin) ASAN: main.metadata and user_variables.basic MTR failures after MDEV-26572 Get rid of locking "empty" plugin commit 39a90aad13e30e4c4a7d6046583a1b48a3605a2c Author: Oleksandr Byelkin <sanja@mariadb.com> Date: Thu Sep 30 10:14:28 2021 +0200   MDEV-26637: (explicit length) ASAN: main.metadata and user_variables.basic MTR failures after MDEV-26572 Use explicit length for hash record length

            Lots of tests fail with a similar stack trace on 10.7 valgrind or MSAN builds. I hope it will be fixed by the same patches.

            10.7 4930209b

            multi_source.info_logs                   [ fail ]  Found warnings/errors in server log file!
                    Test ended at 2021-10-03 14:00:27
            line
            ==4155805== Conditional jump or move depends on uninitialised value(s)
            ==4155805==    at 0x179586B: my_strcoll_ascii_4bytes_found (ctype-ascii.h:110)
            ==4155805==    by 0x1795F38: my_strnncoll_utf8mb3_general_ci (strcoll.ic:226)
            ==4155805==    by 0x16FBDE6: hashcmp (hash.c:373)
            ==4155805==    by 0x16FBBA3: my_hash_first_from_hash_value (hash.c:288)
            ==4155805==    by 0x16FBA29: my_hash_search_using_hash_value (hash.c:244)
            ==4155805==    by 0x16FBE8D: my_hash_insert (hash.c:401)
            ==4155805==    by 0xC4C5A0: Master_info_index::add_master_info(Master_info*, bool) (rpl_mi.cc:1430)
            ==4155805==    by 0x95C7FF: init_slave() (slave.cc:606)
            ==4155805==    by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790)
            ==4155805==    by 0x8E89FC: main (main.cc:34)
            ==4155805== Invalid read of size 4
            ==4155805==    at 0x1795862: my_strcoll_ascii_4bytes_found (ctype-ascii.h:111)
            ==4155805==    by 0x1795F66: my_strnncoll_utf8mb3_general_ci (strcoll.ic:231)
            ==4155805==    by 0x16FBDE6: hashcmp (hash.c:373)
            ==4155805==    by 0x16FBBA3: my_hash_first_from_hash_value (hash.c:288)
            ==4155805==    by 0x16FBA29: my_hash_search_using_hash_value (hash.c:244)
            ==4155805==    by 0x16FBE8D: my_hash_insert (hash.c:401)
            ==4155805==    by 0xC4C5A0: Master_info_index::add_master_info(Master_info*, bool) (rpl_mi.cc:1430)
            ==4155805==    by 0x95C7FF: init_slave() (slave.cc:606)
            ==4155805==    by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790)
            ==4155805==    by 0x8E89FC: main (main.cc:34)
            ==4155805==  Address 0xb6de14d is 29 bytes inside a block of size 32 alloc'd
            ==4155805==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
            ==4155805==    by 0x1728E69: my_malloc (my_malloc.c:90)
            ==4155805==    by 0xC48527: Master_info::Master_info(st_mysql_const_lex_string*, bool) (rpl_mi.cc:61)
            ==4155805==    by 0x95C75A: init_slave() (slave.cc:597)
            ==4155805==    by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790)
            ==4155805==    by 0x8E89FC: main (main.cc:34)
            ==4155805== Conditional jump or move depends on uninitialised value(s)
            ==4155805==    at 0x179586B: my_strcoll_ascii_4bytes_found (ctype-ascii.h:110)
            ==4155805==    by 0x1795F66: my_strnncoll_utf8mb3_general_ci (strcoll.ic:231)
            ==4155805==    by 0x16FBDE6: hashcmp (hash.c:373)
            ==4155805==    by 0x16FBBA3: my_hash_first_from_hash_value (hash.c:288)
            ==4155805==    by 0x16FBA29: my_hash_search_using_hash_value (hash.c:244)
            ==4155805==    by 0x16FBE8D: my_hash_insert (hash.c:401)
            ==4155805==    by 0xC4C5A0: Master_info_index::add_master_info(Master_info*, bool) (rpl_mi.cc:1430)
            ==4155805==    by 0x95C7FF: init_slave() (slave.cc:606)
            ==4155805==    by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790)
            ==4155805==    by 0x8E89FC: main (main.cc:34)
            ==4155805== Invalid read of size 1
            ==4155805==    at 0x1795A2C: my_strcoll_ascii_toupper_8bytes (ctype-ascii.h:159)
            ==4155805==    by 0x1795F7D: my_strnncoll_utf8mb3_general_ci (strcoll.ic:233)
            ==4155805==    by 0x16FBDE6: hashcmp (hash.c:373)
            ==4155805==    by 0x16FBBA3: my_hash_first_from_hash_value (hash.c:288)
            ==4155805==    by 0x16FBA29: my_hash_search_using_hash_value (hash.c:244)
            ==4155805==    by 0x16FBE8D: my_hash_insert (hash.c:401)
            ==4155805==    by 0xC4C5A0: Master_info_index::add_master_info(Master_info*, bool) (rpl_mi.cc:1430)
            ==4155805==    by 0x95C7FF: init_slave() (slave.cc:606)
            ==4155805==    by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790)
            ==4155805==    by 0x8E89FC: main (main.cc:34)
            ==4155805==  Address 0xb6de150 is 0 bytes after a block of size 32 alloc'd
            ==4155805==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
            ==4155805==    by 0x1728E69: my_malloc (my_malloc.c:90)
            ==4155805==    by 0xC48527: Master_info::Master_info(st_mysql_const_lex_string*, bool) (rpl_mi.cc:61)
            ==4155805==    by 0x95C75A: init_slave() (slave.cc:597)
            ==4155805==    by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790)
            ==4155805==    by 0x8E89FC: main (main.cc:34)
            ==4155805== Conditional jump or move depends on uninitialised value(s)
            ==4155805==    at 0x1795AE5: my_strcoll_ascii_toupper_8bytes (ctype-ascii.h:162)
            ==4155805==    by 0x1795F7D: my_strnncoll_utf8mb3_general_ci (strcoll.ic:233)
            ==4155805==    by 0x16FBDE6: hashcmp (hash.c:373)
            ==4155805==    by 0x16FBBA3: my_hash_first_from_hash_value (hash.c:288)
            ==4155805==    by 0x16FBA29: my_hash_search_using_hash_value (hash.c:244)
            ==4155805==    by 0x16FBE8D: my_hash_insert (hash.c:401)
            ==4155805==    by 0xC4C5A0: Master_info_index::add_master_info(Master_info*, bool) (rpl_mi.cc:1430)
            ==4155805==    by 0x95C7FF: init_slave() (slave.cc:606)
            ==4155805==    by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790)
            ==4155805==    by 0x8E89FC: main (main.cc:34)
            ==4155805== Conditional jump or move depends on uninitialised value(s)
            ==4155805==    at 0x1795AEF: my_strcoll_ascii_toupper_8bytes (ctype-ascii.h:162)
            ==4155805==    by 0x1795F7D: my_strnncoll_utf8mb3_general_ci (strcoll.ic:233)
            ==4155805==    by 0x16FBDE6: hashcmp (hash.c:373)
            ==4155805==    by 0x16FBBA3: my_hash_first_from_hash_value (hash.c:288)
            ==4155805==    by 0x16FBA29: my_hash_search_using_hash_value (hash.c:244)
            ==4155805==    by 0x16FBE8D: my_hash_insert (hash.c:401)
            ==4155805==    by 0xC4C5A0: Master_info_index::add_master_info(Master_info*, bool) (rpl_mi.cc:1430)
            ==4155805==    by 0x95C7FF: init_slave() (slave.cc:606)
            ==4155805==    by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790)
            ==4155805==    by 0x8E89FC: main (main.cc:34)
            ^ Found warnings in /mnt-hd8t/bld/10.7-valgrind-nightly/mysql-test/var/log/mysqld.3.err
            

            elenst Elena Stepanova added a comment - Lots of tests fail with a similar stack trace on 10.7 valgrind or MSAN builds. I hope it will be fixed by the same patches. 10.7 4930209b multi_source.info_logs [ fail ] Found warnings/errors in server log file! Test ended at 2021-10-03 14:00:27 line ==4155805== Conditional jump or move depends on uninitialised value(s) ==4155805== at 0x179586B: my_strcoll_ascii_4bytes_found (ctype-ascii.h:110) ==4155805== by 0x1795F38: my_strnncoll_utf8mb3_general_ci (strcoll.ic:226) ==4155805== by 0x16FBDE6: hashcmp (hash.c:373) ==4155805== by 0x16FBBA3: my_hash_first_from_hash_value (hash.c:288) ==4155805== by 0x16FBA29: my_hash_search_using_hash_value (hash.c:244) ==4155805== by 0x16FBE8D: my_hash_insert (hash.c:401) ==4155805== by 0xC4C5A0: Master_info_index::add_master_info(Master_info*, bool) (rpl_mi.cc:1430) ==4155805== by 0x95C7FF: init_slave() (slave.cc:606) ==4155805== by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790) ==4155805== by 0x8E89FC: main (main.cc:34) ==4155805== Invalid read of size 4 ==4155805== at 0x1795862: my_strcoll_ascii_4bytes_found (ctype-ascii.h:111) ==4155805== by 0x1795F66: my_strnncoll_utf8mb3_general_ci (strcoll.ic:231) ==4155805== by 0x16FBDE6: hashcmp (hash.c:373) ==4155805== by 0x16FBBA3: my_hash_first_from_hash_value (hash.c:288) ==4155805== by 0x16FBA29: my_hash_search_using_hash_value (hash.c:244) ==4155805== by 0x16FBE8D: my_hash_insert (hash.c:401) ==4155805== by 0xC4C5A0: Master_info_index::add_master_info(Master_info*, bool) (rpl_mi.cc:1430) ==4155805== by 0x95C7FF: init_slave() (slave.cc:606) ==4155805== by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790) ==4155805== by 0x8E89FC: main (main.cc:34) ==4155805== Address 0xb6de14d is 29 bytes inside a block of size 32 alloc'd ==4155805== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==4155805== by 0x1728E69: my_malloc (my_malloc.c:90) ==4155805== by 0xC48527: Master_info::Master_info(st_mysql_const_lex_string*, bool) (rpl_mi.cc:61) ==4155805== by 0x95C75A: init_slave() (slave.cc:597) ==4155805== by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790) ==4155805== by 0x8E89FC: main (main.cc:34) ==4155805== Conditional jump or move depends on uninitialised value(s) ==4155805== at 0x179586B: my_strcoll_ascii_4bytes_found (ctype-ascii.h:110) ==4155805== by 0x1795F66: my_strnncoll_utf8mb3_general_ci (strcoll.ic:231) ==4155805== by 0x16FBDE6: hashcmp (hash.c:373) ==4155805== by 0x16FBBA3: my_hash_first_from_hash_value (hash.c:288) ==4155805== by 0x16FBA29: my_hash_search_using_hash_value (hash.c:244) ==4155805== by 0x16FBE8D: my_hash_insert (hash.c:401) ==4155805== by 0xC4C5A0: Master_info_index::add_master_info(Master_info*, bool) (rpl_mi.cc:1430) ==4155805== by 0x95C7FF: init_slave() (slave.cc:606) ==4155805== by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790) ==4155805== by 0x8E89FC: main (main.cc:34) ==4155805== Invalid read of size 1 ==4155805== at 0x1795A2C: my_strcoll_ascii_toupper_8bytes (ctype-ascii.h:159) ==4155805== by 0x1795F7D: my_strnncoll_utf8mb3_general_ci (strcoll.ic:233) ==4155805== by 0x16FBDE6: hashcmp (hash.c:373) ==4155805== by 0x16FBBA3: my_hash_first_from_hash_value (hash.c:288) ==4155805== by 0x16FBA29: my_hash_search_using_hash_value (hash.c:244) ==4155805== by 0x16FBE8D: my_hash_insert (hash.c:401) ==4155805== by 0xC4C5A0: Master_info_index::add_master_info(Master_info*, bool) (rpl_mi.cc:1430) ==4155805== by 0x95C7FF: init_slave() (slave.cc:606) ==4155805== by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790) ==4155805== by 0x8E89FC: main (main.cc:34) ==4155805== Address 0xb6de150 is 0 bytes after a block of size 32 alloc'd ==4155805== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==4155805== by 0x1728E69: my_malloc (my_malloc.c:90) ==4155805== by 0xC48527: Master_info::Master_info(st_mysql_const_lex_string*, bool) (rpl_mi.cc:61) ==4155805== by 0x95C75A: init_slave() (slave.cc:597) ==4155805== by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790) ==4155805== by 0x8E89FC: main (main.cc:34) ==4155805== Conditional jump or move depends on uninitialised value(s) ==4155805== at 0x1795AE5: my_strcoll_ascii_toupper_8bytes (ctype-ascii.h:162) ==4155805== by 0x1795F7D: my_strnncoll_utf8mb3_general_ci (strcoll.ic:233) ==4155805== by 0x16FBDE6: hashcmp (hash.c:373) ==4155805== by 0x16FBBA3: my_hash_first_from_hash_value (hash.c:288) ==4155805== by 0x16FBA29: my_hash_search_using_hash_value (hash.c:244) ==4155805== by 0x16FBE8D: my_hash_insert (hash.c:401) ==4155805== by 0xC4C5A0: Master_info_index::add_master_info(Master_info*, bool) (rpl_mi.cc:1430) ==4155805== by 0x95C7FF: init_slave() (slave.cc:606) ==4155805== by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790) ==4155805== by 0x8E89FC: main (main.cc:34) ==4155805== Conditional jump or move depends on uninitialised value(s) ==4155805== at 0x1795AEF: my_strcoll_ascii_toupper_8bytes (ctype-ascii.h:162) ==4155805== by 0x1795F7D: my_strnncoll_utf8mb3_general_ci (strcoll.ic:233) ==4155805== by 0x16FBDE6: hashcmp (hash.c:373) ==4155805== by 0x16FBBA3: my_hash_first_from_hash_value (hash.c:288) ==4155805== by 0x16FBA29: my_hash_search_using_hash_value (hash.c:244) ==4155805== by 0x16FBE8D: my_hash_insert (hash.c:401) ==4155805== by 0xC4C5A0: Master_info_index::add_master_info(Master_info*, bool) (rpl_mi.cc:1430) ==4155805== by 0x95C7FF: init_slave() (slave.cc:606) ==4155805== by 0x8F4768: mysqld_main(int, char**) (mysqld.cc:5790) ==4155805== by 0x8E89FC: main (main.cc:34) ^ Found warnings in /mnt-hd8t/bld/10.7-valgrind-nightly/mysql-test/var/log/mysqld.3.err

            Hi!

            Here follows the review of each commit:

            39a90aad13e30e4c4a7d6046583a1b48a3605a2c
            MDEV-26637: (explicit length)

            ok

            2544edd01b41857ff16120ecd5f1ce7cec476d7c
            Get rid of locking "empty" plugin

            As there was no test case, under what conditions could this happen?
            Please add this to the commit message.
            Is it easy to do a test case? If yes, please create one.

            a3351a6248ff37e42c00f054c76f7349449cc58c
            Prohibit user variables without name

            ok

            4a3834e249769778ed81eab2bb97b7792fa4a039
            No empty name roles exists (no sens to look for).

            Would be even better if role would be changed to a LEX_CSTRING, but that
            can be done later.

            f64531a17b5b0828d04e226fd47ed5f7f36840fc
            Explicitly devide two function of 0 length in the hash keys comparing.

            This one I don't like.

            Instead of fixing hashcmp, which can be called many times in a loop,
            consider doing the following instead:
            (This uses the fact that for not fixed key lengths, hash->key_length will be 0)

            diff --git a/mysys/hash.c b/mysys/hash.c
            index abc11b42500..3b10a906704 100644
            — a/mysys/hash.c
            +++ b/mysys/hash.c
            @@ -282,6 +282,8 @@ uchar* my_hash_first_from_hash_value(const HASH *hash,
            uint flag= 1;
            uint idx= my_hash_mask(hash_value,
            hash->blength, hash->records);
            + if (!length) /* Fixed key length */
            + length= hash->key_length;
            do
            {
            pos= dynamic_element(&hash->array,idx,HASH_LINK*);
            @@ -316,6 +318,8 @@ uchar* my_hash_next(const HASH *hash, const uchar *key, size_t length,
            if (*current_record != NO_RECORD)
            {
            HASH_LINK data=dynamic_element(&hash->array,0,HASH_LINK);
            + if (!length) /* Fixed key length */
            + length= hash->key_length;
            for (idx=data[*current_record].next; idx != NO_RECORD ; idx=pos->next)
            {
            pos=data+idx;
            @@ -355,10 +359,6 @@ static void movelink(HASH_LINK *array,uint find,uint next_link,uint newlink)
            key key for comparison
            length length of key

            • NOTES:
            • If length is 0, comparison is done using the length of the
            • record being compared against.
              -
              RETURN
              = 0 key of record == key
              != 0 key of record != key
              @@ -369,7 +369,7 @@ static int hashcmp(const HASH *hash, HASH_LINK *pos, const uchar *key, { size_t rec_keylength; uchar *rec_key= (uchar*) my_hash_key(hash, pos->data, &rec_keylength, 1); - return ((length && length != rec_keylength) || + return (length != rec_keylength || my_strnncoll(hash->charset, (uchar*) rec_key, rec_keylength, (uchar*) key, rec_keylength)); }
            monty Michael Widenius added a comment - Hi! Here follows the review of each commit: 39a90aad13e30e4c4a7d6046583a1b48a3605a2c MDEV-26637 : (explicit length) ok 2544edd01b41857ff16120ecd5f1ce7cec476d7c Get rid of locking "empty" plugin As there was no test case, under what conditions could this happen? Please add this to the commit message. Is it easy to do a test case? If yes, please create one. a3351a6248ff37e42c00f054c76f7349449cc58c Prohibit user variables without name ok 4a3834e249769778ed81eab2bb97b7792fa4a039 No empty name roles exists (no sens to look for). Would be even better if role would be changed to a LEX_CSTRING, but that can be done later. f64531a17b5b0828d04e226fd47ed5f7f36840fc Explicitly devide two function of 0 length in the hash keys comparing. This one I don't like. Instead of fixing hashcmp, which can be called many times in a loop, consider doing the following instead: (This uses the fact that for not fixed key lengths, hash->key_length will be 0) diff --git a/mysys/hash.c b/mysys/hash.c index abc11b42500..3b10a906704 100644 — a/mysys/hash.c +++ b/mysys/hash.c @@ -282,6 +282,8 @@ uchar* my_hash_first_from_hash_value(const HASH *hash, uint flag= 1; uint idx= my_hash_mask(hash_value, hash->blength, hash->records); + if (!length) /* Fixed key length */ + length= hash->key_length; do { pos= dynamic_element(&hash->array,idx,HASH_LINK*); @@ -316,6 +318,8 @@ uchar* my_hash_next(const HASH *hash, const uchar *key, size_t length, if (*current_record != NO_RECORD) { HASH_LINK data=dynamic_element(&hash->array,0,HASH_LINK ); + if (!length) /* Fixed key length */ + length= hash->key_length; for (idx=data [*current_record] .next; idx != NO_RECORD ; idx=pos->next) { pos=data+idx; @@ -355,10 +359,6 @@ static void movelink(HASH_LINK *array,uint find,uint next_link,uint newlink) key key for comparison length length of key NOTES: If length is 0, comparison is done using the length of the record being compared against. - RETURN = 0 key of record == key != 0 key of record != key @@ -369,7 +369,7 @@ static int hashcmp(const HASH *hash, HASH_LINK *pos, const uchar *key, { size_t rec_keylength; uchar *rec_key= (uchar*) my_hash_key(hash, pos->data, &rec_keylength, 1); - return ((length && length != rec_keylength) || + return (length != rec_keylength || my_strnncoll(hash->charset, (uchar*) rec_key, rec_keylength, (uchar*) key, rec_keylength)); }

            Review done. Can be found in comment section

            monty Michael Widenius added a comment - Review done. Can be found in comment section

            SET @dict_sql=0;
            SET @=0;
            

            Leads to:

            10.7.1 8dd4794c4e11b8790fadf0c203bcd118e7b755e8 (Optimized)

            ==2384454==ERROR: AddressSanitizer: use-after-poison on address 0x6290000872b1 at pc 0x5626a6ad56ad bp 0x146472aef240 sp 0x146472aef230
            READ of size 1 at 0x6290000872b1 thread T15
                #0 0x5626a6ad56ac in my_strcoll_ascii_4bytes_found /test/10.7_opt_san/strings/ctype-ascii.h:111
                #1 0x5626a6ad56ac in my_strnncoll_utf8mb3_general_ci /test/10.7_opt_san/strings/strcoll.ic:226
                #2 0x5626a683f1cb in hashcmp /test/10.7_opt_san/mysys/hash.c:373
                #3 0x5626a683f1cb in my_hash_first_from_hash_value /test/10.7_opt_san/mysys/hash.c:288
                #4 0x5626a683f9c4 in my_hash_search /test/10.7_opt_san/mysys/hash.c:235
                #5 0x5626a418a974 in get_variable(st_hash*, st_mysql_const_lex_string*, bool) /test/10.7_opt_san/sql/item_func.cc:4634
                #6 0x5626a41900ed in Item_func_set_user_var::set_entry(THD*, bool) /test/10.7_opt_san/sql/item_func.cc:4687
                #7 0x5626a41b2014 in Item_func_set_user_var::fix_fields(THD*, Item**) /test/10.7_opt_san/sql/item_func.cc:4713
                #8 0x5626a21ed851 in set_var_user::check(THD*) /test/10.7_opt_san/sql/set_var.cc:899
                #9 0x5626a21fecb4 in sql_set_variables(THD*, List<set_var_base>*, bool) /test/10.7_opt_san/sql/set_var.cc:738
                #10 0x5626a2852306 in mysql_execute_command(THD*, bool) /test/10.7_opt_san/sql/sql_parse.cc:5034
                #11 0x5626a27dafe8 in mysql_parse(THD*, char*, unsigned int, Parser_state*) /test/10.7_opt_san/sql/sql_parse.cc:8028
                #12 0x5626a2830655 in dispatch_command(enum_server_command, THD*, char*, unsigned int, bool) /test/10.7_opt_san/sql/sql_parse.cc:1894
                #13 0x5626a283be52 in do_command(THD*, bool) /test/10.7_opt_san/sql/sql_parse.cc:1402
                #14 0x5626a30e77bd in do_handle_one_connection(CONNECT*, bool) /test/10.7_opt_san/sql/sql_connect.cc:1418
                #15 0x5626a30ea2b4 in handle_one_connection /test/10.7_opt_san/sql/sql_connect.cc:1312
                #16 0x5626a50b2ce1 in pfs_spawn_thread /test/10.7_opt_san/storage/perfschema/pfs.cc:2201
                #17 0x146495bd2608 in start_thread /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477
                #18 0x146494e48292 in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x122292)
             
            0x6290000872b1 is located 177 bytes inside of 16400-byte region [0x629000087200,0x62900008b210)
            allocated by thread T15 here:
                #0 0x5626a1fccab8 in __interceptor_malloc (/test/UBASAN_MD300921-mariadb-10.7.1-linux-x86_64-opt/bin/mariadbd+0x7b60ab8)
                #1 0x5626a68b14cb in my_malloc /test/10.7_opt_san/mysys/my_malloc.c:90
                #2 0x5626a688cf30 in root_alloc /test/10.7_opt_san/mysys/my_alloc.c:66
                #3 0x5626a688cf30 in reset_root_defaults /test/10.7_opt_san/mysys/my_alloc.c:243
                #4 0x5626a24d7e23 in THD::init_for_queries() /test/10.7_opt_san/sql/sql_class.cc:1405
                #5 0x5626a30e1695 in prepare_new_connection_state(THD*) /test/10.7_opt_san/sql/sql_connect.cc:1240
                #6 0x5626a30e2fd7 in thd_prepare_connection(THD*) /test/10.7_opt_san/sql/sql_connect.cc:1333
                #7 0x5626a30e2fd7 in thd_prepare_connection(THD*) /test/10.7_opt_san/sql/sql_connect.cc:1322
                #8 0x5626a30e6674 in do_handle_one_connection(CONNECT*, bool) /test/10.7_opt_san/sql/sql_connect.cc:1408
                #9 0x5626a30ea2b4 in handle_one_connection /test/10.7_opt_san/sql/sql_connect.cc:1312
                #10 0x5626a50b2ce1 in pfs_spawn_thread /test/10.7_opt_san/storage/perfschema/pfs.cc:2201
                #11 0x146495bd2608 in start_thread /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477
             
            Thread T15 created by T0 here:
                #0 0x5626a1ef9725 in __interceptor_pthread_create (/test/UBASAN_MD300921-mariadb-10.7.1-linux-x86_64-opt/bin/mariadbd+0x7a8d725)
                #1 0x5626a50cb0cf in my_thread_create /test/10.7_opt_san/storage/perfschema/my_thread.h:48
                #2 0x5626a50cb0cf in pfs_spawn_thread_v1 /test/10.7_opt_san/storage/perfschema/pfs.cc:2252
                #3 0x5626a2020805 in inline_mysql_thread_create /test/10.7_opt_san/include/mysql/psi/mysql_thread.h:1139
                #4 0x5626a2020805 in create_thread_to_handle_connection(CONNECT*) /test/10.7_opt_san/sql/mysqld.cc:5952
                #5 0x5626a2034510 in handle_accepted_socket(st_mysql_socket, st_mysql_socket) /test/10.7_opt_san/sql/mysqld.cc:6073
                #6 0x5626a203578b in handle_connections_sockets() /test/10.7_opt_san/sql/mysqld.cc:6197
                #7 0x5626a2039409 in mysqld_main(int, char**) /test/10.7_opt_san/sql/mysqld.cc:5847
                #8 0x146494d4d0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
             
            SUMMARY: AddressSanitizer: use-after-poison /test/10.7_opt_san/strings/ctype-ascii.h:111 in my_strcoll_ascii_4bytes_found
            Shadow bytes around the buggy address:
              0x0c5280008e00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
              0x0c5280008e10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
              0x0c5280008e20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
              0x0c5280008e30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
              0x0c5280008e40: 00 00 00 00 00 00 f7 00 00 00 00 00 00 00 00 00
            =>0x0c5280008e50: 00 00 06 f7 00 f7[01]f7 00 00 f7 02 f7 00 00 00
              0x0c5280008e60: 00 00 00 00 00 00 00 00 00 00 00 00 f7 00 00 00
              0x0c5280008e70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
              0x0c5280008e80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
              0x0c5280008e90: 00 00 00 00 00 f7 00 00 f7 00 00 f7 f7 f7 f7 f7
              0x0c5280008ea0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
            Shadow byte legend (one shadow byte represents 8 application bytes):
              Addressable:           00
              Partially addressable: 01 02 03 04 05 06 07
              Heap left redzone:       fa
              Freed heap region:       fd
              Stack left redzone:      f1
              Stack mid redzone:       f2
              Stack right redzone:     f3
              Stack after return:      f5
              Stack use after scope:   f8
              Global redzone:          f9
              Global init order:       f6
              Poisoned by user:        f7
              Container overflow:      fc
              Array cookie:            ac
              Intra object redzone:    bb
              ASan internal:           fe
              Left alloca redzone:     ca
              Right alloca redzone:    cb
              Shadow gap:              cc
            ==2384454==ABORTING
            

            Setup:

            Compiled with GCC >=7.5.0 (I use GCC 9.3.0) and:
                -DWITH_ASAN=ON -DWITH_ASAN_SCOPE=ON -DWITH_UBSAN=ON -DWITH_RAPID=OFF -DWSREP_LIB_WITH_ASAN=ON
            Set before execution:
                export ASAN_OPTIONS=quarantine_size_mb=512:atexit=1:detect_invalid_pointer_pairs=3:dump_instruction_bytes=1:abort_on_error=1
            

            Bug confirmed present in:
            MariaDB: 10.7.1 (dbg), 10.7.1 (opt)

            Bug (or feature/syntax) confirmed not present in:
            MariaDB: 10.2.41 (dbg), 10.2.41 (opt), 10.3.32 (dbg), 10.3.32 (opt), 10.4.22 (dbg), 10.4.22 (opt), 10.5.13 (dbg), 10.5.13 (opt), 10.6.5 (dbg), 10.6.5 (opt)

            Roel Roel Van de Paar added a comment - SET @dict_sql=0; SET @=0; Leads to: 10.7.1 8dd4794c4e11b8790fadf0c203bcd118e7b755e8 (Optimized) ==2384454==ERROR: AddressSanitizer: use-after-poison on address 0x6290000872b1 at pc 0x5626a6ad56ad bp 0x146472aef240 sp 0x146472aef230 READ of size 1 at 0x6290000872b1 thread T15 #0 0x5626a6ad56ac in my_strcoll_ascii_4bytes_found /test/10.7_opt_san/strings/ctype-ascii.h:111 #1 0x5626a6ad56ac in my_strnncoll_utf8mb3_general_ci /test/10.7_opt_san/strings/strcoll.ic:226 #2 0x5626a683f1cb in hashcmp /test/10.7_opt_san/mysys/hash.c:373 #3 0x5626a683f1cb in my_hash_first_from_hash_value /test/10.7_opt_san/mysys/hash.c:288 #4 0x5626a683f9c4 in my_hash_search /test/10.7_opt_san/mysys/hash.c:235 #5 0x5626a418a974 in get_variable(st_hash*, st_mysql_const_lex_string*, bool) /test/10.7_opt_san/sql/item_func.cc:4634 #6 0x5626a41900ed in Item_func_set_user_var::set_entry(THD*, bool) /test/10.7_opt_san/sql/item_func.cc:4687 #7 0x5626a41b2014 in Item_func_set_user_var::fix_fields(THD*, Item**) /test/10.7_opt_san/sql/item_func.cc:4713 #8 0x5626a21ed851 in set_var_user::check(THD*) /test/10.7_opt_san/sql/set_var.cc:899 #9 0x5626a21fecb4 in sql_set_variables(THD*, List<set_var_base>*, bool) /test/10.7_opt_san/sql/set_var.cc:738 #10 0x5626a2852306 in mysql_execute_command(THD*, bool) /test/10.7_opt_san/sql/sql_parse.cc:5034 #11 0x5626a27dafe8 in mysql_parse(THD*, char*, unsigned int, Parser_state*) /test/10.7_opt_san/sql/sql_parse.cc:8028 #12 0x5626a2830655 in dispatch_command(enum_server_command, THD*, char*, unsigned int, bool) /test/10.7_opt_san/sql/sql_parse.cc:1894 #13 0x5626a283be52 in do_command(THD*, bool) /test/10.7_opt_san/sql/sql_parse.cc:1402 #14 0x5626a30e77bd in do_handle_one_connection(CONNECT*, bool) /test/10.7_opt_san/sql/sql_connect.cc:1418 #15 0x5626a30ea2b4 in handle_one_connection /test/10.7_opt_san/sql/sql_connect.cc:1312 #16 0x5626a50b2ce1 in pfs_spawn_thread /test/10.7_opt_san/storage/perfschema/pfs.cc:2201 #17 0x146495bd2608 in start_thread /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477 #18 0x146494e48292 in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x122292)   0x6290000872b1 is located 177 bytes inside of 16400-byte region [0x629000087200,0x62900008b210) allocated by thread T15 here: #0 0x5626a1fccab8 in __interceptor_malloc (/test/UBASAN_MD300921-mariadb-10.7.1-linux-x86_64-opt/bin/mariadbd+0x7b60ab8) #1 0x5626a68b14cb in my_malloc /test/10.7_opt_san/mysys/my_malloc.c:90 #2 0x5626a688cf30 in root_alloc /test/10.7_opt_san/mysys/my_alloc.c:66 #3 0x5626a688cf30 in reset_root_defaults /test/10.7_opt_san/mysys/my_alloc.c:243 #4 0x5626a24d7e23 in THD::init_for_queries() /test/10.7_opt_san/sql/sql_class.cc:1405 #5 0x5626a30e1695 in prepare_new_connection_state(THD*) /test/10.7_opt_san/sql/sql_connect.cc:1240 #6 0x5626a30e2fd7 in thd_prepare_connection(THD*) /test/10.7_opt_san/sql/sql_connect.cc:1333 #7 0x5626a30e2fd7 in thd_prepare_connection(THD*) /test/10.7_opt_san/sql/sql_connect.cc:1322 #8 0x5626a30e6674 in do_handle_one_connection(CONNECT*, bool) /test/10.7_opt_san/sql/sql_connect.cc:1408 #9 0x5626a30ea2b4 in handle_one_connection /test/10.7_opt_san/sql/sql_connect.cc:1312 #10 0x5626a50b2ce1 in pfs_spawn_thread /test/10.7_opt_san/storage/perfschema/pfs.cc:2201 #11 0x146495bd2608 in start_thread /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477   Thread T15 created by T0 here: #0 0x5626a1ef9725 in __interceptor_pthread_create (/test/UBASAN_MD300921-mariadb-10.7.1-linux-x86_64-opt/bin/mariadbd+0x7a8d725) #1 0x5626a50cb0cf in my_thread_create /test/10.7_opt_san/storage/perfschema/my_thread.h:48 #2 0x5626a50cb0cf in pfs_spawn_thread_v1 /test/10.7_opt_san/storage/perfschema/pfs.cc:2252 #3 0x5626a2020805 in inline_mysql_thread_create /test/10.7_opt_san/include/mysql/psi/mysql_thread.h:1139 #4 0x5626a2020805 in create_thread_to_handle_connection(CONNECT*) /test/10.7_opt_san/sql/mysqld.cc:5952 #5 0x5626a2034510 in handle_accepted_socket(st_mysql_socket, st_mysql_socket) /test/10.7_opt_san/sql/mysqld.cc:6073 #6 0x5626a203578b in handle_connections_sockets() /test/10.7_opt_san/sql/mysqld.cc:6197 #7 0x5626a2039409 in mysqld_main(int, char**) /test/10.7_opt_san/sql/mysqld.cc:5847 #8 0x146494d4d0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)   SUMMARY: AddressSanitizer: use-after-poison /test/10.7_opt_san/strings/ctype-ascii.h:111 in my_strcoll_ascii_4bytes_found Shadow bytes around the buggy address: 0x0c5280008e00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c5280008e10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c5280008e20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c5280008e30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c5280008e40: 00 00 00 00 00 00 f7 00 00 00 00 00 00 00 00 00 =>0x0c5280008e50: 00 00 06 f7 00 f7[01]f7 00 00 f7 02 f7 00 00 00 0x0c5280008e60: 00 00 00 00 00 00 00 00 00 00 00 00 f7 00 00 00 0x0c5280008e70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c5280008e80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c5280008e90: 00 00 00 00 00 f7 00 00 f7 00 00 f7 f7 f7 f7 f7 0x0c5280008ea0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==2384454==ABORTING Setup: Compiled with GCC >=7.5.0 (I use GCC 9.3.0) and: -DWITH_ASAN=ON -DWITH_ASAN_SCOPE=ON -DWITH_UBSAN=ON -DWITH_RAPID=OFF -DWSREP_LIB_WITH_ASAN=ON Set before execution: export ASAN_OPTIONS=quarantine_size_mb=512:atexit=1:detect_invalid_pointer_pairs=3:dump_instruction_bytes=1:abort_on_error=1 Bug confirmed present in: MariaDB: 10.7.1 (dbg), 10.7.1 (opt) Bug (or feature/syntax) confirmed not present in: MariaDB: 10.2.41 (dbg), 10.2.41 (opt), 10.3.32 (dbg), 10.3.32 (opt), 10.4.22 (dbg), 10.4.22 (opt), 10.5.13 (dbg), 10.5.13 (opt), 10.6.5 (dbg), 10.6.5 (opt)

            People

              sanja Oleksandr Byelkin
              bar Alexander Barkov
              Votes:
              0 Vote for this issue
              Watchers:
              7 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.