maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Brandon!

On May 03, Brandon Nesterenko wrote:
> revision-id: 242cc3fc41f (mariadb-10.6.0-17-g242cc3fc41f)
> parent(s): fd8c68c7fe6
> author: Brandon Nesterenko <brandon.nesterenko@xxxxxxxxxxx>
> committer: Brandon Nesterenko <brandon.nesterenko@xxxxxxxxxxx>
> timestamp: 2021-04-30 23:17:37 +0000
> message:
> 
> MDEV-14974: --port ignored for --host=localhost
> 
> Problem:
> =======
> MariaDB's command line utilities (e.g., mysql, mysqldump, etc) silently ignore the --port option if no host is given or it is localhost.

please, try to avoid very long lines in commit comments, they're
difficult to read in git console tools.

> 
> Fix:
> ===
> During configuration processing, force protocol to TCP if port was specified via the command line. However, if protocol is additionally specified via the command line, it will be prioritized.
> 
> diff --git a/client/client_priv.h b/client/client_priv.h
> index 64818d2ab8d..6cb1f02ba33 100644
> --- a/client/client_priv.h
> +++ b/client/client_priv.h
> @@ -136,3 +136,85 @@ enum options_client
>    Name of the sys schema database.
>  */
>  #define SYS_SCHEMA_DB_NAME "sys"
> +
> +
> +
> +/**
> +  Changes flags to prepare for (and revert to the previous state) 
> +  from handle_options.
> +
> +  When creating the state, the original state is saved and can be later
> +  resumed by calling this function and setting do_revert_values to TRUE.
> +
> +  Flags:
> +   my_defaults_mark_files: propagates the source of a set option 
> +
> +  @param [in] do_revert_values behavior should revert when TRUE, prepare
> +                               when FALSE
> + */
> +static inline void set_flags_for_option_handling(my_bool do_revert_values)
> +{
> +  static my_bool prev_mark_files;
> +
> +  if (do_revert_values)
> +  {
> +    my_defaults_mark_files = prev_mark_files;
> +  }
> +  else 
> +  {
> +    prev_mark_files = my_defaults_mark_files;
> +    my_defaults_mark_files = TRUE;
> +  }
> +}

this isn't very helpful. my_defaults_mark_files is part of the my_getopt
API. It's supposed to be used directly by whatever tool uses my_getopt.

Here you've turned one line

   my_defaults_mark_files= TRUE;

into 27 lines, so when I (or somebody reading the code) will see

  set_flags_for_option_handling(...);

he'd have to go to the function definition, read the comment and the
function, all 27 lines, understand what they do, and then jump back.

I'd rather prefer to read one line with a very clear semantics and no
conditionals instead.

> +
> +/**
> + Helper function to prepare state for handle_options (current state is saved)
> + */
> +static inline void prepare_option_handling_flags()
> +{
> +  set_flags_for_option_handling(FALSE);
> +}
> +
> +/**
> + Helper function to revert state after handle_options to that from before
> + prepare_option_handling_flags was called
> + */
> +static inline void revert_option_handling_flags()
> +{
> +  set_flags_for_option_handling(TRUE);
> +}

and it keeps going on. if you remove the first function, you won't need
these two functions either.

Saving old value, restoring it. my_defaults_mark_files
affects only my_load_defaults(). It makes no sense to restore it.

> +/**
> +
> +  Utility function to force the connection protocol to TCP when
> +  just the port is specified via the command line. Additionally,
> +  warns the user that the protocol has been changed (MDEV-14974).
> +
> +  Notes:
> +    1) This only takes effect when connecting to localhost
> +    2) Windows uses TCP by default
> +  
> +  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] protocol_loc Location of the protocol option
> +                                  variable to update
> +*/
> +static inline void warn_tcp_protocol_override(FILE *warn_to,
> +    char *host,
> +    uint *protocol_loc)
> +{
> +#ifndef _WIN32
> +  if ((host == NULL
> +        || strncmp(host, LOCAL_HOST, sizeof(LOCAL_HOST)-1) == 0))
> +  {
> +    fprintf(warn_to, "WARNING: "
> +        "Forcing protocol to TCP due to port specification. "
> +        "Please explicitly state TCP protocol or remove "
> +        "port if unintended.\n");
> +    *protocol_loc = MYSQL_PROTOCOL_TCP;
> +  }
> +#endif
> +}
> diff --git a/client/mysql.cc b/client/mysql.cc
> index 433fbd281b9..980daa14365 100644
> --- a/client/mysql.cc
> +++ b/client/mysql.cc
> @@ -206,6 +206,10 @@ static uint opt_protocol=0;
>  static const char *opt_protocol_type= "";
>  static CHARSET_INFO *charset_info= &my_charset_latin1;
>  
> +#ifndef _WIN32
> +static my_bool port_forcing_tcp_proto = FALSE;
> +#endif
> +
>  #include "sslopt-vars.h"
>  
>  const char *default_dbug_option="d:t:o,/tmp/mariadb.trace";
> @@ -1162,6 +1166,8 @@ int main(int argc,char *argv[])
>        close(stdout_fileno_copy);             /* Clean up dup(). */
>    }
>  
> +  prepare_option_handling_flags();

Just do

     my_defaults_mark_files= TRUE;

here

>    load_defaults_or_exit("my", load_default_groups, &argc, &argv);
>    defaults_argv=argv;
>    if ((status.exit_status= get_options(argc, (char **) argv)))
> @@ -1171,6 +1177,14 @@ int main(int argc,char *argv[])
>      exit(status.exit_status);
>    }
>  
> +  revert_option_handling_flags();

and don't revert it.

> +  if (port_forcing_tcp_proto) 
> +  {
> +    warn_tcp_protocol_override(stderr, current_host, &opt_protocol);

I'm not sure it needs a warning. But I don't have strong arguments
against the warning either, so let's keep a warning, if you like it that
way.

Why everything is not on _WIN32 ?
It has named pipes and tcp. Wouldn't it have the same issue with --port?

> +  }
> +
> +
>    if (status.batch && !status.line_buff &&
>        !(status.line_buff= batch_readline_init(MAX_BATCH_BUFFER_SIZE, stdin)))
>    {
> @@ -1715,8 +1729,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_port_override = FALSE;
> +
>    switch(opt->id) {
>    case OPT_CHARSETS_DIR:
>      strmake_buf(mysql_charsets_dir, argument);
> @@ -1780,6 +1797,19 @@ get_one_option(const struct my_option *opt, const char *argument, const char *)
>      else if ((opt_protocol= find_type_with_warning(argument, &sql_protocol_typelib,
>                                                     opt->name)) <= 0)
>        exit(1);
> +#ifndef _WIN32
> +    /* MDEV-14974
> +     *
> +     * Where specifying port implicitly will set the protocol to use TCP, if the
> +     * protocol is explicitly set after the port, prioritize protocol.
> +     */

please, see how multi-comments are usually formatted, and follow the same
style. Generally there's no need to specify the MDEV, and there's no
vertical line of asterisks.

> +    if(filename[0] == '\0')
> +    {
> +      /* Protocol is specified via command line */
> +      ignore_port_override = TRUE;
> +      port_forcing_tcp_proto = FALSE;
> +    }
> +#endif
>  #endif
>      break;
>    case OPT_SERVER_ARG:
> @@ -1883,6 +1913,20 @@ get_one_option(const struct my_option *opt, const char *argument, const char *)
>      status.exit_status= 0;
>      mysql_end(-1);
>      break;
> +  case 'P':
> +#ifndef _WIN32
> +    /*  MDEV-14974:
> +     *
> +     * If port is set via CLI, change protocol to TCP unless protocol has already
> +     * been specified via CLI or TCP is already used
> +     */
> +    if (filename[0] == '\0' && !ignore_port_override 
> +        && opt_protocol != MYSQL_PROTOCOL_TCP)
> +    {
> +      port_forcing_tcp_proto = TRUE;
> +    }

let's also do the same treatment for --socket. If it's specified on the
command line - force the socket protocol.

What could be the least surprising behavior?
I'd think the most intuitive would be, like, WYSIWIG thing -
* if port is specified on the command line (but no socket or protocol) -
  it forces protocol=TCP
* if socket is specified on the command line (but no port of protocol) -
  it forces protocol=SOCKET
* if protocol is specified explicitly anywhere on the command line -
  port and socket lose their magic behavior
* if there's no protocol, but both socket and port are specified? -
  I don't know, perhaps, let's keep the old behavior, no magic?

hmm. How does --host affect that?

> +#endif
> +    break;
>    case 'I':
>    case '?':
>      usage(0);
> @@ -1916,8 +1960,10 @@ static int get_options(int argc, char **argv)
>    opt_max_allowed_packet= *mysql_params->p_max_allowed_packet;
>    opt_net_buffer_length= *mysql_params->p_net_buffer_length;
>  
> +  my_defaults_mark_files = TRUE;
>    if ((ho_error=handle_options(&argc, &argv, my_long_options, get_one_option)))
>      return(ho_error);
> +  my_defaults_mark_files = FALSE;

my_defaults_mark_files has no effect on handle_options()

>    *mysql_params->p_max_allowed_packet= opt_max_allowed_packet;
>    *mysql_params->p_net_buffer_length= opt_net_buffer_length;
>
> diff --git a/mysql-test/main/port_force_tcp.result b/mysql-test/main/port_force_tcp.result
> new file mode 100644
> index 00000000000..ae976bda6cc
> --- /dev/null
> +++ b/mysql-test/main/port_force_tcp.result
> @@ -0,0 +1,14 @@
> +#
> +# MDEV-14974: --port ignored for --host=localhost
> +#
> +Connection:		Localhost via UNIX socket
> +CURRENT_TEST: main.port_force_tcp
> +Connection:		localhost via TCP/IP
> +CURRENT_TEST: main.port_force_tcp
> +Connection:		Localhost via UNIX socket
> +CURRENT_TEST: main.port_force_tcp
> +Connection:		127.0.0.1 via TCP/IP
> +CURRENT_TEST: main.port_force_tcp
> +Connection:		localhost via TCP/IP
> +CURRENT_TEST: main.port_force_tcp
> +WARNING: Forcing protocol to TCP due to port specification. Please explicitly state TCP protocol or remove port if unintended.
> diff --git a/mysql-test/main/port_force_tcp.test b/mysql-test/main/port_force_tcp.test
> new file mode 100644
> index 00000000000..607670d16b0
> --- /dev/null
> +++ b/mysql-test/main/port_force_tcp.test
> @@ -0,0 +1,20 @@
> +--echo #
> +--echo # MDEV-14974: --port ignored for --host=localhost
> +--echo #
> +
> +--source include/not_embedded.inc
> +
> +--exec $MYSQL --host=localhost -e "status" | grep "Connection:"
> +--cat_file $MYSQLTEST_VARDIR/log/current_test
> +
> +--exec $MYSQL --port=$MASTER_MYPORT --protocol=tcp -e "status" | grep "Connection:"
> +--cat_file $MYSQLTEST_VARDIR/log/current_test
> +
> +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT --protocol=socket -e "status" | grep "Connection:"
> +--cat_file $MYSQLTEST_VARDIR/log/current_test
> +
> +--exec $MYSQL --host=127.0.0.1 --port=$MASTER_MYPORT -e "status" | grep "Connection:"
> +--cat_file $MYSQLTEST_VARDIR/log/current_test
> +
> +--exec $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:"
> +--cat_file $MYSQLTEST_VARDIR/log/current_test

this will likely fail on windows, you need to source not_windows.inc
too.

why do you cat current_test after every --exec?
better add --echo before every exec, like

--echo # --host=localhost --port=$MASTER_MYPORT
--exec $MYSQL --host=localhost --port=$MASTER_MYPORT -e "status" | grep "Connection:"

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx