libmultipath: fix memory leak with "uid_attrs" config option
authorMartin Wilck <mwilck@suse.com>
Mon, 24 Jun 2019 09:27:53 +0000 (11:27 +0200)
committerChristophe Varoqui <christophe.varoqui@opensvc.com>
Wed, 3 Jul 2019 06:32:11 +0000 (08:32 +0200)
pp->uid_attribute is normally just a pointer to memory that belongs
to the configuration. But if "uid_attrs" is used, it's a pointer
to strdup()d memory returned by parse_uid_attribute_by_attrs().
Consequently, this strdup()'d memory is never freed.

Fix this by splitting the uid_attrs string when the configuration is
read, and using just a refererence to this memory in pp->uid_attribute.
A side effect is that this makes the code more efficient in both
memory and CPU terms.

This requires a change for the uevents test, as uid_attrs must now be
set up differently.

Cc: Tang Junhui <tang.junhui@zte.com.cn>
Signed-off-by: Martin Wilck <mwilck@suse.com>
libmultipath/config.c
libmultipath/config.h
libmultipath/dict.c
libmultipath/propsel.c
libmultipath/uevent.c
libmultipath/util.c
libmultipath/util.h
tests/globals.c
tests/uevent.c

index 141f092..20e3b8b 100644 (file)
@@ -585,8 +585,7 @@ free_config (struct config * conf)
        if (conf->uid_attribute)
                FREE(conf->uid_attribute);
 
-       if (conf->uid_attrs)
-               FREE(conf->uid_attrs);
+       vector_reset(&conf->uid_attrs);
 
        if (conf->getuid)
                FREE(conf->getuid);
@@ -718,7 +717,6 @@ load_config (char * file)
        conf->remove_retries = 0;
        conf->ghost_delay = DEFAULT_GHOST_DELAY;
        conf->all_tg_pt = DEFAULT_ALL_TG_PT;
-
        /*
         * preload default hwtable
         */
@@ -853,3 +851,50 @@ out:
        free_config(conf);
        return NULL;
 }
+
+char *get_uid_attribute_by_attrs(struct config *conf,
+                                const char *path_dev)
+{
+       vector uid_attrs = &conf->uid_attrs;
+       int j;
+       char *att, *col;
+
+       vector_foreach_slot(uid_attrs, att, j) {
+               col = strrchr(att, ':');
+               if (!col)
+                       continue;
+               if (!strncmp(path_dev, att, col - att))
+                       return col + 1;
+       }
+       return NULL;
+}
+
+int parse_uid_attrs(char *uid_attrs, struct config *conf)
+{
+       vector attrs  = &conf->uid_attrs;
+       char *uid_attr_record, *tmp;
+       int  ret = 0, count;
+
+       if (!uid_attrs)
+               return 1;
+
+       count = get_word(uid_attrs, &uid_attr_record);
+       while (uid_attr_record) {
+               tmp = strchr(uid_attr_record, ':');
+               if (!tmp) {
+                       condlog(2, "invalid record in uid_attrs: %s",
+                               uid_attr_record);
+                       free(uid_attr_record);
+                       ret = 1;
+               } else if (!vector_alloc_slot(attrs)) {
+                       free(uid_attr_record);
+                       ret = 1;
+               } else
+                       vector_set_slot(attrs, uid_attr_record);
+               if (!count)
+                       break;
+               uid_attrs += count;
+               count = get_word(uid_attrs, &uid_attr_record);
+       }
+       return ret;
+}
index f5bf5b1..ff2b4e8 100644 (file)
@@ -190,7 +190,7 @@ struct config {
 
        char * multipath_dir;
        char * selector;
-       char * uid_attrs;
+       struct _vector uid_attrs;
        char * uid_attribute;
        char * getuid;
        char * features;
@@ -250,4 +250,8 @@ void free_config (struct config * conf);
 extern struct config *get_multipath_config(void);
 extern void put_multipath_config(void *);
 
+int parse_uid_attrs(char *uid_attrs, struct config *conf);
+char *get_uid_attribute_by_attrs(struct config *conf,
+                                const char *path_dev);
+
 #endif
index 96815f8..c6eba0f 100644 (file)
@@ -374,8 +374,38 @@ declare_ovr_snprint(selector, print_str)
 declare_mp_handler(selector, set_str)
 declare_mp_snprint(selector, print_str)
 
-declare_def_handler(uid_attrs, set_str)
-declare_def_snprint(uid_attrs, print_str)
+static int snprint_uid_attrs(struct config *conf, char *buff, int len,
+                            const void *dummy)
+{
+       char *p = buff;
+       int n, j;
+       const char *att;
+
+       vector_foreach_slot(&conf->uid_attrs, att, j) {
+               n = snprintf(p, len, "%s%s", j == 0 ? "" : " ", att);
+               if (n >= len)
+                       return (p - buff) + n;
+               p += n;
+               len -= n;
+       }
+       return p - buff;
+}
+
+static int uid_attrs_handler(struct config *conf, vector strvec)
+{
+       char *val;
+
+       vector_reset(&conf->uid_attrs);
+       val = set_value(strvec);
+       if (!val)
+               return 1;
+       if (parse_uid_attrs(val, conf))
+               condlog(1, "error parsing uid_attrs: \"%s\"", val);
+       condlog(3, "parsed %d uid_attrs", VECTOR_SIZE(&conf->uid_attrs));
+       FREE(val);
+       return 0;
+}
+
 declare_def_handler(uid_attribute, set_str)
 declare_def_snprint_defstr(uid_attribute, print_str, DEFAULT_UID_ATTRIBUTE)
 declare_ovr_handler(uid_attribute, set_str)
@@ -1618,7 +1648,7 @@ init_keywords(vector keywords)
        install_keyword("multipath_dir", &def_multipath_dir_handler, &snprint_def_multipath_dir);
        install_keyword("path_selector", &def_selector_handler, &snprint_def_selector);
        install_keyword("path_grouping_policy", &def_pgpolicy_handler, &snprint_def_pgpolicy);
-       install_keyword("uid_attrs", &def_uid_attrs_handler, &snprint_def_uid_attrs);
+       install_keyword("uid_attrs", &uid_attrs_handler, &snprint_uid_attrs);
        install_keyword("uid_attribute", &def_uid_attribute_handler, &snprint_def_uid_attribute);
        install_keyword("getuid_callout", &def_getuid_handler, &snprint_def_getuid);
        install_keyword("prio", &def_prio_name_handler, &snprint_def_prio_name);
index e6263e9..6af2513 100644 (file)
@@ -536,9 +536,9 @@ int select_getuid(struct config *conf, struct path *pp)
 {
        const char *origin;
 
-       pp->uid_attribute = parse_uid_attribute_by_attrs(conf->uid_attrs, pp->dev);
+       pp->uid_attribute = get_uid_attribute_by_attrs(conf, pp->dev);
        if (pp->uid_attribute) {
-               origin = "(setting: multipath.conf defaults section)";
+               origin = "(setting: multipath.conf defaults section / uid_attrs)";
                goto out;
        }
 
index f73de8c..8f7b2ef 100644 (file)
@@ -163,13 +163,12 @@ uevent_get_wwid(struct uevent *uev)
 
        conf = get_multipath_config();
        pthread_cleanup_push(put_multipath_config, conf);
-       uid_attribute = parse_uid_attribute_by_attrs(conf->uid_attrs, uev->kernel);
+       uid_attribute = get_uid_attribute_by_attrs(conf, uev->kernel);
        pthread_cleanup_pop(1);
 
        val = uevent_get_env_var(uev, uid_attribute);
        if (val)
                uev->wwid = val;
-       FREE(uid_attribute);
 }
 
 bool
@@ -179,7 +178,7 @@ uevent_need_merge(void)
        bool need_merge = false;
 
        conf = get_multipath_config();
-       if (conf->uid_attrs)
+       if (VECTOR_SIZE(&conf->uid_attrs) > 0)
                need_merge = true;
        put_multipath_config(conf);
 
index 8a4be78..28cbf4b 100644 (file)
@@ -273,48 +273,6 @@ dev_t parse_devt(const char *dev_t)
        return makedev(maj, min);
 }
 
-char *parse_uid_attribute_by_attrs(char *uid_attrs, char *path_dev)
-{
-       char *uid_attribute;
-       char *uid_attr_record;
-       char *dev;
-       char *attr;
-       char *tmp;
-       int  count;
-
-       if(!uid_attrs || !path_dev)
-               return NULL;
-
-       count = get_word(uid_attrs, &uid_attr_record);
-       while (uid_attr_record) {
-               tmp = strrchr(uid_attr_record, ':');
-               if (!tmp) {
-                       free(uid_attr_record);
-                       if (!count)
-                               break;
-                       uid_attrs += count;
-                       count = get_word(uid_attrs, &uid_attr_record);
-                       continue;
-               }
-               dev = uid_attr_record;
-               attr = tmp + 1;
-               *tmp = '\0';
-
-               if(!strncmp(path_dev, dev, strlen(dev))) {
-                       uid_attribute = STRDUP(attr);
-                       free(uid_attr_record);
-                       return uid_attribute;
-               }
-
-               free(uid_attr_record);
-               if (!count)
-                       break;
-               uid_attrs += count;
-               count = get_word(uid_attrs, &uid_attr_record);
-       }
-       return NULL;
-}
-
 void
 setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached)
 {
index 1e0d832..693991c 100644 (file)
@@ -15,7 +15,6 @@ size_t strlcat(char *dst, const char *src, size_t size);
 int devt2devname (char *, int, char *);
 dev_t parse_devt(const char *dev_t);
 char *convert_dev(char *dev, int is_path_device);
-char *parse_uid_attribute_by_attrs(char *uid_attrs, char *path_dev);
 void setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached);
 int systemd_service_enabled(const char *dev);
 int get_linux_version_code(void);
index aeb7723..8add5eb 100644 (file)
@@ -5,7 +5,6 @@
 struct udev *udev;
 int logsink = -1;
 struct config conf = {
-       .uid_attrs = "sd:ID_BOGUS",
        .verbosity = 4,
 };
 
index b0d0bfd..215d97a 100644 (file)
@@ -43,7 +43,11 @@ void uevent_get_wwid(struct uevent *uev);
 
 static int setup_uev(void **state)
 {
+       static char test_uid_attrs[] =
+               "dasd:ID_SPAM   sd:ID_BOGUS nvme:ID_EGGS    ";
+
        struct uevent *uev = alloc_uevent();
+       struct config *conf;
 
        if (uev == NULL)
                return -1;
@@ -51,11 +55,16 @@ static int setup_uev(void **state)
        *state = uev;
        uev->kernel = "sdo";
        uev->envp[0] = "MAJOR=" str(MAJOR);
+       uev->envp[1] = "ID_SPAM=nonsense";
        uev->envp[1] = "ID_BOGUS=" WWID;
        uev->envp[2] = "MINOR=" str(MINOR);
        uev->envp[3] = "DM_NAME=" DM_NAME;
        uev->envp[4] = "DISK_RO=" str(DISK_RO);
        uev->envp[5] = NULL;
+
+       conf = get_multipath_config();
+       parse_uid_attrs(test_uid_attrs, conf);
+       put_multipath_config(conf);
        return 0;
 }