commit eca38e7e289310103059b19aa396325154261a72 Author: davidtyang Date: Thu Jul 18 10:47:04 2024 +0800 Bug#115662 Unexpected error and exit on unrecognized flags in flagset sysvar Problem: In a setup with multiple installations of MySQL on the same machine, either name or value of an option might not be recognized by all versions. The --loose prefix can be used to ignore unrecognized option names, while numeric option values can be adjusted. Both issue warnings instead of exit on error. Flagset options are common practice to deliver new sub-features among minor versions. However, an unrecognized flagset value could not be adjusted, which would generate an error and abort bootstrap: mysqld --optimizer_switch='mrr=on,foo=off' [ERROR] [MY-000077] [Server] mysqld: Error while setting value 'mrr=on,foo=off' to 'optimizer_switch'. Solution: Unrecognized flags resemble out-of-bound values, and could be adjusted in similar way as truncating numeric values. So, add extra params to find_set_from_flags() to collect known and unrecognized flags separately. Generate a warning instead of an error in both bootstrap and non-strict modes: mysqld --optimizer_switch='mrr=on,foo=off' [Warning] [MY-000094] [Server] option 'optimizer_switch': value 'mrr=on,foo=off' adjusted to 'mrr=on', discarding 'foo=off'. mysql> set optimizer_switch='mrr=on,foo=off'; -- non-strict mode Warning 6424 Adjusted incorrect optimizer_switch value: 'mrr=on,foo=off', discarding 'foo=off'. -- strict mode, same error ERROR 1231 (42000): Variable 'optimizer_switch' can't be set to the value of 'foo=off' diff --git a/include/mysys_err.h b/include/mysys_err.h index 53609d146b2..ae7dcbce499 100644 --- a/include/mysys_err.h +++ b/include/mysys_err.h @@ -136,7 +136,8 @@ extern const char *globerrs[]; /* my_error_messages is here */ #define EE_FAILED_TO_RESET_BEFORE_SECONDARY_IGNORABLE_CHAR 91 #define EE_FAILED_PROCESSING_DIRECTIVE 92 #define EE_PTHREAD_KILL_FAILED 93 -#define EE_ERROR_LAST 93 /* Copy last error nr */ +#define EE_ADJUSTED_FLAGSET_VALUE_FOR_OPTION 94 +#define EE_ERROR_LAST 94 /* Copy last error nr */ /* Add error numbers before EE_ERROR_LAST and change it accordingly. */ /* Exit codes for option processing. When exiting from server use the diff --git a/include/typelib.h b/include/typelib.h index d43f28c4c2c..de8c2971e2b 100644 --- a/include/typelib.h +++ b/include/typelib.h @@ -62,6 +62,8 @@ extern TYPELIB sql_protocol_typelib; uint64_t find_set_from_flags(const TYPELIB *lib, int default_name, uint64_t cur_set, uint64_t default_set, const char *str, unsigned int length, - const char **err_pos, unsigned int *err_len); + const char **err_pos, unsigned int *err_len, + char *abuf, uint64_t asize, char *ebuf, + uint64_t esize); #endif /* _typelib_h */ diff --git a/mysql-test/r/flagset_option_with_unrecognized_flag.result b/mysql-test/r/flagset_option_with_unrecognized_flag.result new file mode 100644 index 00000000000..6ceacdb93ac --- /dev/null +++ b/mysql-test/r/flagset_option_with_unrecognized_flag.result @@ -0,0 +1,25 @@ +CALL mtr.add_suppression(".*\\[Warning\\] \\[MY-000094\\] \\[Server\\] option 'optimizer_switch': value '.*' adjusted to '.*', discarding '.*'"); +# restart:--optimizer_switch=mrr=on,foo=off +LOG_TIME 0 [Warning] [MY-000094] [Server] option 'optimizer_switch': value 'mrr=on,foo=off' adjusted to 'mrr=on', discarding 'foo=off'. +SET @saved_sql_mode = @@sql_mode; +SET sql_mode = STRICT_ALL_TABLES; +Warnings: +Warning 3135 'NO_ZERO_DATE', 'NO_ZERO_IN_DATE' and 'ERROR_FOR_DIVISION_BY_ZERO' sql modes should be used with strict mode. They will be merged with strict mode in a future release. +SET optimizer_switch='mrr=on,foo=off'; +ERROR 42000: Variable 'optimizer_switch' can't be set to the value of 'foo=off' +SET sql_mode = @saved_sql_mode; +SET optimizer_switch='mrr=on,foo=off'; +Warnings: +Warning 6424 Adjusted incorrect optimizer_switch value: 'mrr=on,foo=off', discarding 'foo=off'. +SET optimizer_switch='foo=on,mrr=off'; +Warnings: +Warning 6424 Adjusted incorrect optimizer_switch value: 'foo=on,mrr=off', discarding 'foo=on'. +SET optimizer_switch='mrr=on,ab=c=-d=123-,derived_merge=off'; +Warnings: +Warning 6424 Adjusted incorrect optimizer_switch value: 'mrr=on,ab=c=-d=123-,derived_merge=off', discarding 'ab=c=-d=123-'. +SET optimizer_switch='ab=c=-d=123-,'; +Warnings: +Warning 6424 Adjusted incorrect optimizer_switch value: 'ab=c=-d=123-,', discarding 'ab=c=-d=123-'. +SET optimizer_switch='mrr=on'; +SET optimizer_switch=''; +# restart: diff --git a/mysql-test/t/flagset_option_with_unrecognized_flag.test b/mysql-test/t/flagset_option_with_unrecognized_flag.test new file mode 100644 index 00000000000..786f05ca07a --- /dev/null +++ b/mysql-test/t/flagset_option_with_unrecognized_flag.test @@ -0,0 +1,29 @@ +CALL mtr.add_suppression(".*\\[Warning\\] \\[MY-000094\\] \\[Server\\] option 'optimizer_switch': value '.*' adjusted to '.*', discarding '.*'"); + +# Always adjust to bootstrap. Note that sql_mode is not an early option. +# Only verify handling one unrecognized case here, leaving full tests to non-strict mode, +# because they share same base code. +let $restart_parameters="restart:--optimizer_switch='mrr=on,foo=off'"; +source include/restart_mysqld.inc; +let $expected_warning="\[Warning\] \[MY-000094\] \[Server\] option 'optimizer_switch': value 'mrr=on,foo=off' adjusted to 'mrr=on', discarding 'foo=off'."; +--replace_regex /[0-9]*-[0-9]*-[0-9]*T[0-9]*:[0-9]*:[0-9]*.[0-9]*Z/LOG_TIME/ +exec grep $expected_warning $MYSQLTEST_VARDIR/log/mysqld.1.err; + +# Verify in strict mode +SET @saved_sql_mode = @@sql_mode; +SET sql_mode = STRICT_ALL_TABLES; +--error ER_WRONG_VALUE_FOR_VAR +SET optimizer_switch='mrr=on,foo=off'; + +# Verify in non-strict mode +# Verified positions of unrecognized flags: tail, head, in the middle, all unrecognized, all recognized, none +SET sql_mode = @saved_sql_mode; +SET optimizer_switch='mrr=on,foo=off'; +SET optimizer_switch='foo=on,mrr=off'; +SET optimizer_switch='mrr=on,ab=c=-d=123-,derived_merge=off'; +SET optimizer_switch='ab=c=-d=123-,'; +SET optimizer_switch='mrr=on'; +SET optimizer_switch=''; + +let $restart_parameters = "restart:"; +--source include/restart_mysqld.inc diff --git a/mysys/errors.cc b/mysys/errors.cc index ef80e3a2776..37752cf9305 100644 --- a/mysys/errors.cc +++ b/mysys/errors.cc @@ -141,7 +141,8 @@ const char *globerrs[GLOBERRS] = { "Unknown LDML tag: '%.*s'.", "Failed to reset before a secondary ignorable character %s.", "Stopped processing the '%s' directive in file %s at line %d.", - "pthread_kill(thread_id:%lu, signal:%s) returned '%s'."}; + "pthread_kill(thread_id:%lu, signal:%s) returned '%s'.", + "option '%s': value '%s' adjusted to '%s', discarding '%s'."}; /* We cannot call my_error/my_printf_error here in this function. diff --git a/mysys/my_getopt.cc b/mysys/my_getopt.cc index dca6c04a217..b5bbb9831fb 100644 --- a/mysys/my_getopt.cc +++ b/mysys/my_getopt.cc @@ -947,11 +947,22 @@ static int setval(const struct my_option *opts, void *value, case GET_FLAGSET: { const char *flag_error; uint error_len; + char abuf[MYSYS_ERRMSG_SIZE]; + char ebuf[MYSYS_ERRMSG_SIZE]; *(static_cast(value)) = find_set_from_flags( opts->typelib, opts->typelib->count, *static_cast(value), opts->def_value, argument, - strlen(argument), &flag_error, &error_len); + strlen(argument), &flag_error, &error_len, abuf, sizeof(abuf), + ebuf, sizeof(ebuf)); + + if (ebuf[0] != '\0') { + my_getopt_error_reporter( + WARNING_LEVEL, EE_ADJUSTED_FLAGSET_VALUE_FOR_OPTION, opts->name, + argument, abuf, ebuf); + assert(!flag_error); + } + if (flag_error) { res = EXIT_ARGUMENT_INVALID; goto ret; diff --git a/mysys/typelib.cc b/mysys/typelib.cc index 36fed135e74..981d33e308e 100644 --- a/mysys/typelib.cc +++ b/mysys/typelib.cc @@ -261,6 +261,26 @@ static int parse_name(const TYPELIB *lib, const char **strpos, return find; } +// pp remembers valid positions in the buffer. +inline void append_flag(char *buf, uint size, char **pp, const char *start, + const char *end) { + assert(size > 0); + assert(*pp >= buf && *pp < buf + size); + assert(start <= end); + char *p = *pp; + char *reserved = buf + size - 1; + + ptrdiff_t copy_len = end - start; + if (p + copy_len >= reserved) copy_len = reserved - p; + + if (copy_len > 0) { + memcpy(p, start, copy_len); + p += copy_len; + *p = '\0'; + *pp = p; + } +} + /** Parse and apply a set of flag assingments @@ -274,6 +294,10 @@ static int parse_name(const TYPELIB *lib, const char **strpos, @param err_pos OUT If error, set to point to start of wrong set string NULL on success @param err_len OUT If error, set to the length of wrong set string + @param abuf OUT String buffer to collect recognized flags + @param asize Size of abuf + @param ebuf OUT String buffer to collect unrecognized flags + @param esize Size of ebuf @details Parse a set of flag assignments, that is, parse a string in form: @@ -299,11 +323,18 @@ static int parse_name(const TYPELIB *lib, const char **strpos, uint64_t find_set_from_flags(const TYPELIB *lib, int default_name, uint64_t cur_set, uint64_t default_set, const char *str, uint length, const char **err_pos, - uint *err_len) { + uint *err_len, char *abuf, uint64_t asize, + char *ebuf, uint64_t esize) { const char *end = str + length; uint64_t flags_to_set = 0, flags_to_clear = 0, res; bool set_defaults = false; + char *aptr = abuf; + char *eptr = ebuf; + assert(asize > 0 && esize > 0); + *aptr = '\0'; + *eptr = '\0'; + *err_pos = nullptr; /* No error yet */ if (str != end) { const char *start = str; @@ -312,7 +343,24 @@ uint64_t find_set_from_flags(const TYPELIB *lib, int default_name, uint value; const int flag_no = parse_name(lib, &pos, end); - if (flag_no <= 0) goto err; + + if (flag_no < 0) + goto err; + + if (flag_no == 0) { + /* Consume '=xxx' */ + for (; pos != end && *pos != ','; pos++) + ; + + append_flag(ebuf, esize, &eptr, start, pos + (*pos == ',')); + + if (pos >= end) break; + + pos++; + + start = pos; + continue; + } if (flag_no == default_name) { /* Using 'default' twice isn't allowed. */ @@ -338,6 +386,9 @@ uint64_t find_set_from_flags(const TYPELIB *lib, int default_name, flags_to_clear |= bit; } } + + append_flag(abuf, asize, &aptr, start, pos + (*pos == ',')); + if (pos >= end) break; if (*pos++ != ',') goto err; @@ -353,5 +404,11 @@ uint64_t find_set_from_flags(const TYPELIB *lib, int default_name, res = set_defaults ? default_set : cur_set; res |= flags_to_set; res &= ~flags_to_clear; + + if (aptr > abuf && *--aptr == ',') + *aptr = '\0'; + if (eptr > ebuf && *--eptr == ',') + *eptr = '\0'; + return res; } diff --git a/share/messages_to_clients.txt b/share/messages_to_clients.txt index a10bb7ce31b..1cb6dada701 100644 --- a/share/messages_to_clients.txt +++ b/share/messages_to_clients.txt @@ -10521,6 +10521,9 @@ ER_LH_CP_MISSING_FILES_MODE ER_LH_CP_COMMIT_FAILED eng "Lakehouse Incremental Load commit operation failed." +ER_ADJUSTED_WRONG_VALUE + eng "Adjusted incorrect %-.64s value: '%-.128s', discarding '%0.128s'." + # # End of 9.x error messages (server-to-client). # diff --git a/sql/set_var.cc b/sql/set_var.cc index 328c496201c..fbb32228673 100644 --- a/sql/set_var.cc +++ b/sql/set_var.cc @@ -590,6 +590,20 @@ bool throw_bounds_warning(THD *thd, const char *name, bool fixed, double v) { return false; } +bool throw_bounds_warning(THD *thd, const char *name, bool fixed, + const char *v, const char *err, const char *eb) { + if (fixed) { + if (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES) { + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name, err); + return true; + } + push_warning_printf(thd, Sql_condition::SL_WARNING, + ER_ADJUSTED_WRONG_VALUE, + ER_THD(thd, ER_ADJUSTED_WRONG_VALUE), name, v, eb); + } + return false; +} + const CHARSET_INFO *sys_var::charset(THD *thd) { return is_os_charset ? thd->variables.character_set_filesystem : system_charset_info; diff --git a/sql/sys_vars.h b/sql/sys_vars.h index 75b8f1b4139..01d9b3f881b 100644 --- a/sql/sys_vars.h +++ b/sql/sys_vars.h @@ -1561,14 +1561,22 @@ class Sys_var_flagset : public Sys_var_typelib { const char *error; uint error_len; + char abuf[MYSQL_ERRMSG_SIZE]; + char ebuf[MYSQL_ERRMSG_SIZE]; var->save_result.ulonglong_value = find_set_from_flags( &typelib, typelib.count, current_value, default_value, res->ptr(), - static_cast(res->length()), &error, &error_len); - if (error) { - const ErrConvString err(error, error_len, res->charset()); - my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name.str, err.ptr()); - return true; + static_cast(res->length()), &error, &error_len, + abuf, sizeof(abuf), ebuf, sizeof(ebuf)); + if (!error) { + error = ebuf; + error_len = strlen(ebuf); } + const ErrConvString err(error, error_len, res->charset()); + const ErrConvString eb(ebuf, strlen(ebuf), res->charset()); + + bool fixed = ebuf[0] != '\0'; + return throw_bounds_warning(current_thd, name.str, fixed, res->ptr(), + err.ptr(), eb.ptr()); } } else { const longlong tmp = var->value->val_int(); diff --git a/sql/sys_vars_shared.h b/sql/sys_vars_shared.h index 622c221ebad..56f91fcfd5f 100644 --- a/sql/sys_vars_shared.h +++ b/sql/sys_vars_shared.h @@ -44,6 +44,9 @@ extern bool throw_bounds_warning(THD *thd, const char *name, bool fixed, bool is_unsigned, longlong v); extern bool throw_bounds_warning(THD *thd, const char *name, bool fixed, double v); +extern bool throw_bounds_warning(THD *thd, const char *name, bool fixed, + const char *v, const char *err, + const char *eb); extern sys_var *find_static_system_variable(const std::string &name); extern sys_var *find_dynamic_system_variable(const std::string &name); extern sys_var *intern_find_sys_var(const char *str, size_t length);