On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote:
> multipath always requires absolute pathnames, so make sure all file
> and
> directory names start with a slash. Also check that the directories
> exist. Finally, some strings, like the alias, will be used in paths.
> These must not contain the slash character '/', since it is a
> forbidden
> character in file/directory names. This patch adds seperate handlers
> for
> these three cases. If a config line is invalid, these handlers retain
> the existing config string, if any.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
Mostly OK, see remarks below. I'm a bit wary that when we start this,
we might need to do other checks as well. For example, as multipathd is
running as root, we may want to check that the paths to files it writes
to (bindings_file etc.) don't contain symlinks and have proper
permissions... But that'd be another patch.
Regards,
Martin
> ---
> libmultipath/dict.c | 88 +++++++++++++++++++++++++++++++++++++++----
> --
> 1 file changed, 78 insertions(+), 10 deletions(-)
>
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index 1758bd26..91333068 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -5,6 +5,8 @@
> * Copyright (c) 2005 Kiyoshi Ueda, NEC
> */
> #include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> #include <pwd.h>
> #include <string.h>
> #include "checkers.h"
> @@ -111,6 +113,72 @@ set_str(vector strvec, void *ptr, const char
> *file, int line_nr)
> return 0;
> }
>
> +static int
> +set_dir(vector strvec, void *ptr, const char *file, int line_nr)
> +{
> + char **str_ptr = (char **)ptr;
> + char *old_str = *str_ptr;
> + struct stat sb;
> +
> + *str_ptr = set_value(strvec);
> + if (!*str_ptr) {
> + free(old_str);
> + return 1;
> + }
> + if ((*str_ptr)[0] != '/'){
> + condlog(1, "%s line %d, %s is not an absolute
> directory path. Ignoring", file, line_nr, *str_ptr);
> + *str_ptr = old_str;
> + } else {
> + if (stat(*str_ptr, &sb) == 0 && S_ISDIR(sb.st_mode))
> + free(old_str);
> + else {
> + condlog(1, "%s line %d, %s is not an existing
> directory. Ignoring", file, line_nr, *str_ptr);
> + *str_ptr = old_str;
> + }
> + }
> + return 0;
> +}
> +
> +static int
> +set_path(vector strvec, void *ptr, const char *file, int line_nr)
> +{
> + char **str_ptr = (char **)ptr;
> + char *old_str = *str_ptr;
> +
> + *str_ptr = set_value(strvec);
> + if (!*str_ptr) {
> + free(old_str);
> + return 1;
> + }
> + if ((*str_ptr)[0] != '/'){
> + condlog(1, "%s line %d, %s is not an absolute path.
> Ignoring",
> + file, line_nr, *str_ptr);
> + *str_ptr = old_str;
> + } else
> + free(old_str);
> + return 0;
> +}
Once you go down this route, you might as well test that the dirname of
the path is an existing directory.
> +
> +static int
> +set_str_noslash(vector strvec, void *ptr, const char *file, int
> line_nr)
> +{
> + char **str_ptr = (char **)ptr;
> + char *old_str = *str_ptr;
> +
> + *str_ptr = set_value(strvec);
> + if (!*str_ptr) {
> + free(old_str);
> + return 1;
> + }
> + if (strchr(*str_ptr, '/')) {
> + condlog(1, "%s line %d, %s cannot contain a slash.
> Ignoring",
> + file, line_nr, *str_ptr);
> + *str_ptr = old_str;
> + } else
> + free(old_str);
> + return 0;
> +}
> +
> static int
> set_yes_no(vector strvec, void *ptr, const char *file, int line_nr)
> {
> @@ -353,13 +421,13 @@ declare_def_snprint(verbosity, print_int)
> declare_def_handler(reassign_maps, set_yes_no)
> declare_def_snprint(reassign_maps, print_yes_no)
>
> -declare_def_handler(multipath_dir, set_str)
> +declare_def_handler(multipath_dir, set_dir)
> declare_def_snprint(multipath_dir, print_str)
>
> static int def_partition_delim_handler(struct config *conf, vector
> strvec,
> const char *file, int line_nr)
> {
> - int rc = set_str(strvec, &conf->partition_delim, file,
> line_nr);
> + int rc = set_str_noslash(strvec, &conf->partition_delim,
> file, line_nr);
>
> if (rc != 0)
> return rc;
> @@ -490,11 +558,11 @@ declare_hw_snprint(prio_name, print_str)
> declare_mp_handler(prio_name, set_str)
> declare_mp_snprint(prio_name, print_str)
>
> -declare_def_handler(alias_prefix, set_str)
> +declare_def_handler(alias_prefix, set_str_noslash)
> declare_def_snprint_defstr(alias_prefix, print_str,
> DEFAULT_ALIAS_PREFIX)
> -declare_ovr_handler(alias_prefix, set_str)
> +declare_ovr_handler(alias_prefix, set_str_noslash)
> declare_ovr_snprint(alias_prefix, print_str)
> -declare_hw_handler(alias_prefix, set_str)
> +declare_hw_handler(alias_prefix, set_str_noslash)
> declare_hw_snprint(alias_prefix, print_str)
>
> declare_def_handler(prio_args, set_str)
> @@ -586,13 +654,13 @@ declare_hw_snprint(user_friendly_names,
> print_yes_no_undef)
> declare_mp_handler(user_friendly_names, set_yes_no_undef)
> declare_mp_snprint(user_friendly_names, print_yes_no_undef)
>
> -declare_def_handler(bindings_file, set_str)
> +declare_def_handler(bindings_file, set_path)
> declare_def_snprint(bindings_file, print_str)
>
> -declare_def_handler(wwids_file, set_str)
> +declare_def_handler(wwids_file, set_path)
> declare_def_snprint(wwids_file, print_str)
>
> -declare_def_handler(prkeys_file, set_str)
> +declare_def_handler(prkeys_file, set_path)
> declare_def_snprint(prkeys_file, print_str)
>
> declare_def_handler(retain_hwhandler, set_yes_no_undef)
> @@ -692,7 +760,7 @@ def_config_dir_handler(struct config *conf,
> vector strvec, const char *file,
> /* this is only valid in the main config file */
> if (conf->processed_main_config)
> return 0;
> - return set_str(strvec, &conf->config_dir, file, line_nr);
> + return set_path(strvec, &conf->config_dir, file, line_nr);
> }
Why not set_dir() here?
> declare_def_snprint(config_dir, print_str)
>
> @@ -1732,7 +1800,7 @@ multipath_handler(struct config *conf, vector
> strvec, const char *file,
> declare_mp_handler(wwid, set_str)
> declare_mp_snprint(wwid, print_str)
>
> -declare_mp_handler(alias, set_str)
> +declare_mp_handler(alias, set_str_noslash)
> declare_mp_snprint(alias, print_str)
>
> /*
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
