[dm-devel] [PATCH 6/8] libmultipath: improve checks for set_str
Benjamin Marzinski
bmarzins at redhat.com
Fri Nov 5 17:45:09 UTC 2021
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 at redhat.com>
>
> 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)
> > >
> > > /*
> >
More information about the dm-devel
mailing list