Hi Ben,
On Thu, 2021-11-04 at 21:34 +0100, Martin Wilck wrote:
> 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>
I've changed my mind on this one. The options for directories and paths
should be turned into buildtime options instead. If we do that, we
don't need this sort of checks any more, except the "noslash" part.
Regards,
Martin
>
> 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
