Re: [Maria-developers] 2a5663ae524: MDEV-14974: --port ignored for --host=localhost

Hi, Brandon! On May 04, Brandon Nesterenko wrote:

revision-id: 2a5663ae524 (mariadb-10.6.0-18-g2a5663ae524) parent(s): 9fe681c9e4d author: Brandon Nesterenko committer: Brandon Nesterenko timestamp: 2021-05-03 19:47:07 +0000 message:

MDEV-14974: --port ignored for --host=localhost

Problem: ======= MariaDB's command line utilities (e.g., mysql, mysqldump, etc) silently ignore connection property options (e.g., --port and --socket) when protocol is not explicitly set via the command-line for localhost connections.

Fix: === If connection properties are specified without a protocol, override the protocol to be consistent. For example, if --port is specified, automatically set protocol=tcp.

Caveats: ======= * When multiple connection properties are specified, nothing is overridden * If protocol is is set via the command-line, its value is used

diff --git a/client/client_priv.h b/client/client_priv.h index 64818d2ab8d..94e1fc8bb08 100644 --- a/client/client_priv.h +++ b/client/client_priv.h @@ -136,3 +136,57 @@ enum options_client Name of the sys schema database. */ #define SYS_SCHEMA_DB_NAME "sys" + + +/** + Utility function to implicitly change the connection protocol to a + consistent value given the command line arguments. Additionally, + warns the user that the protocol has been changed. + + Arguments: + @param [in] warn_to The file to write the warning to + @param [in] host Name of the host to connect to + @param [in, out] opt_protocol Location of the protocol option + variable to update + @param [in] new_protocol New protocol to force +*/ +static inline void warn_protocol_override(FILE *warn_to,

it's always stderr, isn't it? Why did you want it as an argument?

+ char *host, + uint *opt_protocol, + uint new_protocol) +{ + if ((host == NULL + || strncmp(host, LOCAL_HOST, sizeof(LOCAL_HOST)-1) == 0)) + { + const char *TCP_NAME = "TCP"; + const char *SOCKET_NAME = "SOCKET"; + char *protocol_name; + + if(new_protocol == MYSQL_PROTOCOL_TCP) + protocol_name = (char *) TCP_NAME;

better not to hard-code protocol names here, you can do protocol_name= sql_protocol_typelib.type_names[new_protocol-1];

+ else if(new_protocol == MYSQL_PROTOCOL_SOCKET) + protocol_name = (char *) SOCKET_NAME; + else + { + /* + This should never be entered, but just in case we are called incorrectly + */ + + fprintf(warn_to, "%s %d %s %d\n", + "WARNING: Protocol ID ", + new_protocol, + " cannot override connection type. " + "Using the configuration value of ", + *opt_protocol); + return;

for things that cannot happen code-wise (some code guarantees that it can never happen), better add an assert (DBUG_ASSERT, in fact) and there's no need to do an warning or anything. An assert is like a self-maintaining documentation. Its main purpose is to document that something cannot happen. But unlike your comment it also checks that it doesn't become outdated. so, this could become DBUG_ASSERT(new_protocol == MYSQL_PROTOCOL_SOCKET || new_protocol == MYSQL_PROTOCOL_TCP); protocol_name= sql_protocol_typelib.type_names[new_protocol-1];

+ } + + fprintf(warn_to, "%s %s %s\n", + "WARNING: Forcing protocol to ", + protocol_name, + " due to option specification. " + "Please explicitly state intended protocol."); + + *opt_protocol = new_protocol; + } +} diff --git a/client/mysql.cc b/client/mysql.cc index 433fbd281b9..5ca9b4393ec 100644 --- a/client/mysql.cc +++ b/client/mysql.cc @@ -206,6 +206,8 @@ static uint opt_protocol=0; static const char *opt_protocol_type= ""; static CHARSET_INFO *charset_info= &my_charset_latin1;

+static uint protocol_to_force= MYSQL_PROTOCOL_DEFAULT; + #include "sslopt-vars.h"

const char *default_dbug_option="d:t:o,/tmp/mariadb.trace"; @@ -1162,6 +1164,9 @@ int main(int argc,char *argv[]) close(stdout_fileno_copy); /* Clean up dup(). */ }

+ /* We need to know if protocol-related options originate from CLI args */ + my_defaults_mark_files = TRUE; + load_defaults_or_exit("my", load_default_groups, &argc, &argv); defaults_argv=argv; if ((status.exit_status= get_options(argc, (char **) argv))) @@ -1171,6 +1176,14 @@ int main(int argc,char *argv[]) exit(status.exit_status); }

+ /* Command line options override configured protocol */ + if (protocol_to_force > MYSQL_PROTOCOL_DEFAULT + && protocol_to_force != opt_protocol) + { + warn_protocol_override(stderr, current_host, &opt_protocol, protocol_to_force); + } + + if (status.batch && !status.line_buff && !(status.line_buff= batch_readline_init(MAX_BATCH_BUFFER_SIZE, stdin))) { @@ -1715,8 +1728,11 @@ static void usage(int version)

my_bool -get_one_option(const struct my_option *opt, const char *argument, const char *) +get_one_option(const struct my_option *opt, const char *argument, const char *filename) { + /* Track when protocol is set via CLI to not force port TCP protocol override */ + static my_bool ignore_protocol_override = FALSE; + switch(opt->id) { case OPT_CHARSETS_DIR: strmake_buf(mysql_charsets_dir, argument); @@ -1781,6 +1797,14 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) opt->name)) <= 0) exit(1); #endif + + /* Specification of protocol via CLI trumps implicit overrides */ + if (filename[0] == '\0') + { + ignore_protocol_override = TRUE; + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; + } + break; case OPT_SERVER_ARG: #ifdef EMBEDDED_LIBRARY @@ -1872,6 +1896,13 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) #ifdef __WIN__ opt_protocol = MYSQL_PROTOCOL_PIPE; opt_protocol_type= "pipe"; + + /* Prioritize pipe if explicit via command line */ + if (filename[0] == '\0') + { + ignore_protocol_override = TRUE; + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; + } #endif break; #include @@ -1883,6 +1914,40 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) status.exit_status= 0; mysql_end(-1); break; + case 'P': + /* If port and socket are set, fall back to default behavior */ + if (protocol_to_force == MYSQL_PROTOCOL_SOCKET) + { + ignore_protocol_override = TRUE; + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; + } + + /* If port is set via CLI, try to force protocol to TCP */ + if (filename[0] == '\0' && + !ignore_protocol_override && + MYSQL_PROTOCOL_TCP > protocol_to_force)

that's a strange condition. I believe it's practically identical to protocol_to_force == MYSQL_PROTOCOL_DEFAULT but a lot more confusing. Why not to write it as protocol_to_force == MYSQL_PROTOCOL_DEFAULT ?

+ { + protocol_to_force = MYSQL_PROTOCOL_TCP; + } + break; + case 'S': +#ifndef __WIN__ + /* If port and socket are set, fall back to default behavior */ + if (protocol_to_force == MYSQL_PROTOCOL_TCP) + { + ignore_protocol_override = TRUE; + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; + } + + /* Prioritize socket if set via command line */ + if (filename[0] == '\0' && + !ignore_protocol_override && + MYSQL_PROTOCOL_SOCKET > protocol_to_force)

same here

+ { + protocol_to_force = MYSQL_PROTOCOL_SOCKET; + } +#endif + break; case 'I': case '?': usage(0); diff --git a/client/mysqladmin.cc b/client/mysqladmin.cc index e40e82f8038..08ec14815bd 100644 --- a/client/mysqladmin.cc +++ b/client/mysqladmin.cc @@ -54,6 +54,8 @@ static bool sql_log_bin_off= false; static uint opt_protocol=0; static myf error_flags; /* flags to pass to my_printf_error, like ME_BELL */

+static uint protocol_to_force= MYSQL_PROTOCOL_DEFAULT; + /* When using extended-status relatively, ex_val_max_len is the estimated maximum length for any relative value printed by extended-status. The @@ -241,8 +243,12 @@ static const char *load_default_groups[]= 0 };

my_bool -get_one_option(const struct my_option *opt, const char *argument, const char *) +get_one_option(const struct my_option *opt, const char *argument, const char *filename) { + + /* Track when protocol is set via CLI to not force overrides */ + static my_bool ignore_protocol_override = FALSE; + switch(opt->id) { case 'c': opt_count_iterations= 1; @@ -274,6 +280,13 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) case 'W': #ifdef __WIN__ opt_protocol = MYSQL_PROTOCOL_PIPE; + + /* Prioritize pipe if explicit via command line */ + if (filename[0] == '\0') + { + ignore_protocol_override = TRUE; + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; + } #endif break; case '#': @@ -309,6 +322,48 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) sf_leaking_memory= 1; /* no memory leak reports here */ exit(1); } + + /* Specification of protocol via CLI trumps implicit overrides */ + if (filename[0] == '\0') + { + ignore_protocol_override = TRUE; + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; + } + + break; + case 'P': + /* If port and socket are set, fall back to default behavior */ + if (protocol_to_force == MYSQL_PROTOCOL_SOCKET) + { + ignore_protocol_override = TRUE; + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; + } + + /* If port is set via CLI, try to force protocol to TCP */ + if (filename[0] == '\0' && + !ignore_protocol_override && + MYSQL_PROTOCOL_TCP > protocol_to_force) + { + protocol_to_force = MYSQL_PROTOCOL_TCP; + } + break; + case 'S': +#ifndef __WIN__ + /* If port and socket are set, fall back to default behavior */ + if (protocol_to_force == MYSQL_PROTOCOL_TCP) + { + ignore_protocol_override = TRUE; + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; + } + + /* Prioritize socket if set via command line */ + if (filename[0] == '\0' && + !ignore_protocol_override && + MYSQL_PROTOCOL_SOCKET > protocol_to_force) + { + protocol_to_force = MYSQL_PROTOCOL_SOCKET; + } +#endif break; } return 0; @@ -323,6 +378,10 @@ int main(int argc,char *argv[])

MY_INIT(argv[0]); sf_leaking_memory=1; /* don't report memory leaks on early exits */ + + /* We need to know if protocol-related options originate from CLI args */ + my_defaults_mark_files = TRUE; + load_defaults_or_exit("my", load_default_groups, &argc, &argv); save_argv = argv; /* Save for free_defaults */

@@ -331,6 +390,13 @@ int main(int argc,char *argv[]) temp_argv= mask_password(argc, &argv); temp_argc= argc;

+ /* Command line options override configured protocol */ + if (protocol_to_force > MYSQL_PROTOCOL_DEFAULT + && protocol_to_force != opt_protocol) + { + warn_protocol_override(stderr, host, &opt_protocol, protocol_to_force); + } + if (debug_info_flag) my_end_arg= MY_CHECK_ERROR | MY_GIVE_INFO; if (debug_check_flag) diff --git a/client/mysqlbinlog.cc b/client/mysqlbinlog.cc index fd31ab6694e..1ff6048e5ad 100644 --- a/client/mysqlbinlog.cc +++ b/client/mysqlbinlog.cc @@ -98,6 +98,8 @@ static const char *output_prefix= ""; static char **defaults_argv= 0; static MEM_ROOT glob_root;

+static uint protocol_to_force= MYSQL_PROTOCOL_DEFAULT; + #ifndef DBUG_OFF static const char *default_dbug_option = "d:t:o,/tmp/mariadb-binlog.trace"; const char *current_dbug_option= default_dbug_option; @@ -1959,9 +1961,13 @@ static my_time_t convert_str_to_timestamp(const char* str)

extern "C" my_bool -get_one_option(const struct my_option *opt, const char *argument, const char *) +get_one_option(const struct my_option *opt, const char *argument, const char *filename) { bool tty_password=0; + + /* Track when protocol is set via CLI to not force overrides */ + static my_bool ignore_protocol_override = FALSE; + switch (opt->id) { #ifndef DBUG_OFF case '#': @@ -2011,6 +2017,14 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) sf_leaking_memory= 1; /* no memory leak reports here */ die(); } + + /* Specification of protocol via CLI trumps implicit overrides */ + if (filename[0] == '\0') + { + ignore_protocol_override = TRUE; + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; + } + break; #ifdef WHEN_FLASHBACK_REVIEW_READY case opt_flashback_review: @@ -2092,6 +2106,40 @@ get_one_option(const struct my_option *opt, const char *argument, const char *) case OPT_PRINT_ROW_EVENT_POSITIONS: print_row_event_positions_used= 1; break; + case 'P': + /* If port and socket are set, fall back to default behavior */ + if (protocol_to_force == MYSQL_PROTOCOL_SOCKET) + { + ignore_protocol_override = TRUE; + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; + } + + /* If port is set via CLI, try to force protocol to TCP */ + if (filename[0] == '\0' && + !ignore_protocol_override && + MYSQL_PROTOCOL_TCP > protocol_to_force) + { + protocol_to_force = MYSQL_PROTOCOL_TCP; + } + break; + case 'S': +#ifndef __WIN__ + /* If port and socket are set, fall back to default behavior */ + if (protocol_to_force == MYSQL_PROTOCOL_TCP) + { + ignore_protocol_override = TRUE; + protocol_to_force = MYSQL_PROTOCOL_DEFAULT; + } + + /* Prioritize socket if set via command line */ + if (filename[0] == '\0' && + !ignore_protocol_override && + MYSQL_PROTOCOL_SOCKET > protocol_to_force) + { + protocol_to_force = MYSQL_PROTOCOL_SOCKET; + } +#endif + break; case 'v': if (argument == disabled_my_option) verbose= 0; @@ -3049,6 +3097,9 @@ int main(int argc, char** argv) my_init_time(); // for time functions tzset(); // set tzname

+ /* We need to know if protocol-related options originate from CLI args */ + my_defaults_mark_files = TRUE; + load_defaults_or_exit("my", load_groups, &argc, &argv); defaults_argv= argv;

@@ -3062,6 +3113,13 @@ int main(int argc, char** argv)

parse_args(&argc, (char***)&argv);

+ /* Command line options override configured protocol */ + if (protocol_to_force > MYSQL_PROTOCOL_DEFAULT + && protocol_to_force != opt_protocol) + { + warn_protocol_override(stderr, host, &opt_protocol, protocol_to_force); + } + if (!argc || opt_version) { if (!opt_version) diff --git a/client/mysqlcheck.c b/client/mysqlcheck.c index fb3103a318d..54f8d2eb4f3 100644 --- a/client/mysqlcheck.c +++ b/client/mysqlcheck.c @@ -56,6 +56,8 @@ static char *opt_skip_database; DYNAMIC_ARRAY tables4repair, tables4rebuild, alter_table_cmds; DYNAMIC_ARRAY views4repair; static uint opt_protocol=0; + +static uint protocol_to_force= MYSQL_PROTOCOL_DEFAULT;

enum operations { DO_CHECK=1, DO_REPAIR, DO_ANALYZE, DO_OPTIMIZE, DO_FIX_NAMES }; const char *operation_name[]= @@ -289,6 +291,10 @@ get_one_option(const struct my_option *opt, const char *filename __attribute__((unused)))

not __attribute__((unused)) anymore here and in other files too, I won't repeat this comment for every file

{ int orig_what_to_do= what_to_do; + + /* Track when protocol is set via CLI to not force overrides */ + static my_bool ignore_protocol_override = FALSE; + DBUG_ENTER("get_one_option");

switch(opt->id) { diff --git a/mysql-test/main/cli_options_force_protocol.result b/mysql-test/main/cli_options_force_protocol.result --- /dev/null +++ b/mysql-test/main/cli_options_force_protocol.result @@ -0,0 +1,27 @@ +# +# MDEV-14974: --port ignored for --host=localhost +# +# +# The following tests until the first cat_file should produce no warnings +# +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost -e "status" | grep "Connection:"

oh-oh. You cannot hard-code your local path into the result file. Not the port number either. That's why in my previous review I wrote --echo # --host=localhost --port=MASTER_MYPORT and not --echo # $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" (actually I had --port=$MASTER_MYPORT in my email, but that was wrong, should be no $-sign in --echo :)

+Connection: Localhost via UNIX socket +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --port=16000 --protocol=tcp -e "status" | grep "Connection:" +Connection: localhost via TCP/IP +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost --port=16000 --protocol=socket -e "status" | grep "Connection:" +Connection: Localhost via UNIX socket +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=127.0.0.1 --port=16000 -e "status" | grep "Connection:" +Connection: 127.0.0.1 via TCP/IP +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost --socket=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/tmp/mysqld.1.sock --port=16000 -e "status" | grep "Connection:" +Connection: Localhost via UNIX socket +CURRENT_TEST: main.cli_options_force_protocol +# +# The remaining tests should produce warnings +# +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost --port=16000 -e "status" | grep "Connection:" +Connection: localhost via TCP/IP +# /home/buildbot/workspace/server/build-mariadb-server-debug/client/mysql --defaults-file=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/my.cnf --host=localhost --socket=/home/buildbot/workspace/server/build-mariadb-server-debug/mysql-test/var/tmp/mysqld.1.sock -e "status" | grep "Connection:" +Connection: Localhost via UNIX socket +CURRENT_TEST: main.cli_options_force_protocol +WARNING: Forcing protocol to TCP due to option specification. Please explicitly state intended protocol. +WARNING: Forcing protocol to SOCKET due to option specification. Please explicitly state intended protocol. diff --git a/mysql-test/main/cli_options_force_protocol.test b/mysql-test/main/cli_options_force_protocol.test new file mode 100644 index 00000000000..f91d7833a0d --- /dev/null +++ b/mysql-test/main/cli_options_force_protocol.test @@ -0,0 +1,41 @@ +--echo # +--echo # MDEV-14974: --port ignored for --host=localhost +--echo # + +--source include/not_embedded.inc +--source include/not_windows.inc + +--echo # +--echo # The following tests until the first cat_file should produce no warnings +--echo # + +--echo # $MYSQL --host=localhost -e "status" | grep "Connection:" +--exec $MYSQL --host=localhost -e "status" | grep "Connection:" + +--echo # $MYSQL --port=$MASTER_MYPORT --protocol=tcp -e "status" | grep "Connection:" +--exec $MYSQL --port=$MASTER_MYPORT --protocol=tcp -e "status" | grep "Connection:" + +--echo # $MYSQL --host=localhost --port=$MASTER_MYPORT --protocol=socket -e "status" | grep "Connection:" +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT --protocol=socket -e "status" | grep "Connection:" + +--echo # $MYSQL --host=127.0.0.1 --port=$MASTER_MYPORT -e "status" | grep "Connection:" +--exec $MYSQL --host=127.0.0.1 --port=$MASTER_MYPORT -e "status" | grep "Connection:" + +--echo # $MYSQL --host=localhost --socket=$MASTER_MYSOCK --port=$MASTER_MYPORT -e "status" | grep "Connection:" +--exec $MYSQL --host=localhost --socket=$MASTER_MYSOCK --port=$MASTER_MYPORT -e "status" | grep "Connection:" + +--cat_file $MYSQLTEST_VARDIR/log/current_test + + +--echo # +--echo # The remaining tests should produce warnings +--echo # + +--echo # $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:" + +--echo # $MYSQL --host=localhost --socket=$MASTER_MYSOCK -e "status" | grep "Connection:" +--exec $MYSQL --host=localhost --socket=$MASTER_MYSOCK -e "status" | grep "Connection:" + +--cat_file $MYSQLTEST_VARDIR/log/current_test

I still don't understand why you include current test name into the result file. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org