Re: [PATCH 6/8] libmultipath: improve checks for set_str

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Nov 05, 2021 at 06:59:11AM +0000, Martin Wilck wrote:
> 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.

That seems reasonable. I do want to ask around a little bit to see if I
can find anyone who is actually setting the directories. The only one I
really worry about is "config_dir". I worry that people might do
something like stick that on shared storage, to make it possible to
change the multipath config on a bunch of machines in one place.

-Ben

> 
> 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





[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux