• Guilhem Bichot's avatar
    Fix for BUG#59894 · 1756d087
    Guilhem Bichot authored
    "set optimizer_switch to e or d causes invalid memory writes/valgrind warnings":
    due to prefix support, the argument "e" was overwritten with its full value
    "engine_condition_pushdown", which caused a buffer overrun.
    This was wrong usage of find_type(); other wrong usages are fixed here too.
    Please start reading with the comment of typelib.c.
    
    client/mysqldump.c:
      A bug: find_type() expects a bitmap as 3rd argument
      (each bit is a flag controlling a behaviour of the function);
      here it was instead passed the length of the string to search!
      That could give random behaviour of find_type()
      depending on the string.
      We rather need to pass a correct flag to find_type().
      The correct flag is FIND_TYPE_BASIC (0).
      Flag 8 is not needed as buff cannot have a comma (see how buff is filled).
      Flag 1 looks like a superfluous restriction.
      Flag 4 is not user-friendly (why use
      --compatible=2 rather than --compatible=mysql40 ?, and
      we probably not commit to "2" always meaning "mysql40"
      until the end of times).
    include/mysql.h.pp:
      This isn't a problematic API change as we go from char* to const char*:
      existing code will run unchanged.
    include/typelib.h:
      named constants. Not an enum to not significantly change
      the declaration of find_type() which would be an API change
      (typelib.h is included in mysql.h).
    mysql-test/r/mysqldump.result:
      correct result (see the two requested modes in SQL_MODE)
    mysql-test/suite/sys_vars/t/optimizer_switch_basic.test:
      test for BUG#59894. The second SET used to crash.
    mysql-test/t/mysqldump.test:
      we had no test for multiple modes in --compatible, which is
      supported according to --help
    mysys/typelib.c:
      Fix for BUG#59894. parse_name() is asked to match "e" with a row
      of the TYPELIB (the TYPELIB lists permitted flags of optimizer_switch;
      and comes from optimizer_switch_names[] of sys_vars.cc).
      find_type() is capable of supporting prefixes, but if it is not
      passed flag 2 in third argument, it will overwrite its first
      argument (the string to search for) with the complete name,
      here overwriting "e" with "engine_condition_pushdown". But
      as this "e" was a buffer allocated in an Item, it was not big
      enough to host the longer name, thus the crash.
      We don't need to know the complete flag's name; the output used
      from find_type() is just the flag's number (== function's return
      code). So we can pass flag 2 to find_type() in parse_name().
      After doing this fix and the other fixes in this patch, all usages
      of find_type() were using flag 2; in most usages the string to search for,
      is not guaranteed to be long enough to host the complete name
      (it is either directly from argv, or from alloc_root/my_malloc
      done in an earlier call).
      Thus, flag 2 is here made implicit: callers need not pass it anymore,
      it is always automatically turned on.
      This allows to eliminate an oddity: parse_name() took a const char**,
      and then removed "const" before calling find_type(), which could
      theoretically modify the pointed data, thus lying on constness.
      Last, constants for find_type() are now named.
    sql-common/client.c:
      Two bugs:
      1) The enum was not in sync with the array (due to a bad porting of WL 1054;
      the extra OPT_ values are about options present in 5.1 and deleted in 5.5);
      added a compile_time_assert() to make sure this doesn't happen again
      2) find_type() was writing past the end of opt_arg; as opt_arg was allocated
      with alloc_root() with no extra space, this was an overrun; it could be seen
      when
      ** building with -DWITH_VALGRIND -DHAVE_purify -DEXTRA_DEBUG
      ** making execution go through the faulty code; this faulty
      code is executed only if the client asks to read a configuration
      file like this:
        mysql_options(mysql, MYSQL_READ_DEFAULT_FILE, "/tmp/cnf.cnf");
      so by adding such line to the start of mysql_client_test.c::client_connect(),
      we could see the valgrind warning:
      ==30548== Invalid write of size 1
      ==30548==    at 0x4C2624C: strcpy (mc_replace_strmem.c:303)
      ==30548==    by 0x48DC29: find_type (typelib.c:120)
      ==30548==    by 0x465686: mysql_read_default_options (client.c:1344)
      ==30548==    by 0x46830F: mysql_real_connect (client.c:2971)
      ==30548==    by 0x409339: client_connect (mysql_client_test.c:331)
      ==30548==    by 0x463A7F: main (mysql_client_test.c:19902)
      ==30548==  Address 0x61875ad is 0 bytes after a block of size 29 alloc'd
      ==30548==    at 0x4C25153: malloc (vg_replace_malloc.c:195)
      ==30548==    by 0x49BFF1: my_malloc (my_malloc.c:38)
      ==30548==    by 0x49C65C: alloc_root (my_alloc.c:166)
      ==30548==    by 0x48EF97: handle_default_option (default.c:381)
      ==30548==    by 0x49068C: search_default_file_with_ext (default.c:992)
      ==30548==    by 0x48F929: search_default_file (default.c:670)
      ==30548==    by 0x48EDC4: my_search_option_files (default.c:312)
      ==30548==    by 0x48F4B1: my_load_defaults (default.c:576)
      ==30548==    by 0x46517A: mysql_read_default_options (client.c:1207)
      ==30548==    by 0x46830F: mysql_real_connect (client.c:2971)
      ==30548==    by 0x409339: client_connect (mysql_client_test.c:331)
      ==30548==    by 0x463A7F: main (mysql_client_test.c:19902)
      This is fixed by having find_type() not overwrite anymore.
    sql/sql_help.cc:
      cast not needed anymore.
    sql/table.cc:
      cast not needed anymore.
    1756d087
optimizer_switch_basic.result 4.37 KB