[dm-devel] [PATCH] libmultipath: fix a memory leak in set_ble_device
Benjamin Marzinski
bmarzins at redhat.com
Thu Jul 30 21:25:43 UTC 2020
On Thu, Jul 30, 2020 at 09:27:28PM +0800, lixiaokeng wrote:
> Hi.
> I'm very sorry for subject mistake in first mail.
>
> In set_ble_device func, if blist is NULL or ble is NULL,
> the vendor and product isn't free. We think it is not
> reasonable that strdup(XXX) is used as set_ble_device
> and store_ble functions' parameter.
>
> Here we call strdup() in store_ble and set_ble_device
> functions and the string will be free if functions fail.
> Because constant string like "sdb" will be their parameter,
> char * is changed to const char *. This is base on
> upstream-queue branch in openSUSE/multipath-tools.
>
> Here we have another question. We want to know which branch
> is mainline in https://github.com/openSUSE/multipath-tools.
> The master is not changed since two years ago. And we find
> the ups/submit-2007 branch in mwilck/multipath-tools have
> a great changes. Do you have a plan to merge it to openSUSE.
The branch mwilck/upstream-queue aims to have the current upstream head
plus all the outstanding patches that have been acked on dm-devel but
not yet added to the usptream repository,
If you're looking for something to base upstream patches on, that's
what you want.
> Signed-off-by: Lixiaokeng <lixiaokeng at huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26 at huawei.com>
Reviewed-by: Benjamin Marzinski <bmazins at redhat.com>
>
> ---
> libmultipath/blacklist.c | 74 +++++++++++++++++++++++-----------------
> libmultipath/blacklist.h | 4 +--
> tests/blacklist.c | 31 +++++++----------
> 3 files changed, 58 insertions(+), 51 deletions(-)
>
> diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
> index db58ccca..04aeb426 100644
> --- a/libmultipath/blacklist.c
> +++ b/libmultipath/blacklist.c
> @@ -29,14 +29,19 @@ char *check_invert(char *str, bool *invert)
> return str;
> }
>
> -int store_ble(vector blist, char * str, int origin)
> +int store_ble(vector blist, const char * str, int origin)
> {
> struct blentry * ble;
> char *regex_str;
> + char *strdup_str = NULL;
>
> if (!str)
> return 0;
>
> + strdup_str = strdup(str);
> + if (!strdup_str)
> + return 1;
> +
> if (!blist)
> goto out;
>
> @@ -45,21 +50,21 @@ int store_ble(vector blist, char * str, int origin)
> if (!ble)
> goto out;
>
> - regex_str = check_invert(str, &ble->invert);
> + regex_str = check_invert(strdup_str, &ble->invert);
> if (regcomp(&ble->regex, regex_str, REG_EXTENDED|REG_NOSUB))
> goto out1;
>
> if (!vector_alloc_slot(blist))
> goto out1;
>
> - ble->str = str;
> + ble->str = strdup_str;
> ble->origin = origin;
> vector_set_slot(blist, ble);
> return 0;
> out1:
> FREE(ble);
> out:
> - FREE(str);
> + FREE(strdup_str);
> return 1;
> }
>
> @@ -79,10 +84,12 @@ int alloc_ble_device(vector blist)
> return 0;
> }
>
> -int set_ble_device(vector blist, char * vendor, char * product, int origin)
> +int set_ble_device(vector blist, const char * vendor, const char * product, int origin)
> {
> struct blentry_device * ble;
> char *regex_str;
> + char *vendor_str = NULL;
> + char *product_str = NULL;
>
> if (!blist)
> return 1;
> @@ -93,31 +100,42 @@ int set_ble_device(vector blist, char * vendor, char * product, int origin)
> return 1;
>
> if (vendor) {
> - regex_str = check_invert(vendor, &ble->vendor_invert);
> + vendor_str = STRDUP(vendor);
> + if (!vendor_str)
> + goto out;
> +
> + regex_str = check_invert(vendor_str, &ble->vendor_invert);
> if (regcomp(&ble->vendor_reg, regex_str,
> REG_EXTENDED|REG_NOSUB)) {
> - FREE(vendor);
> - if (product)
> - FREE(product);
> - return 1;
> + goto out;
> }
> - ble->vendor = vendor;
> + ble->vendor = vendor_str;
> }
> if (product) {
> - regex_str = check_invert(product, &ble->product_invert);
> + product_str = STRDUP(product);
> + if (!product_str)
> + goto out1;
> +
> + regex_str = check_invert(product_str, &ble->product_invert);
> if (regcomp(&ble->product_reg, regex_str,
> REG_EXTENDED|REG_NOSUB)) {
> - FREE(product);
> - if (vendor) {
> - ble->vendor = NULL;
> - FREE(vendor);
> - }
> - return 1;
> + goto out1;
> }
> - ble->product = product;
> + ble->product = product_str;
> }
> ble->origin = origin;
> return 0;
> +out1:
> + if (vendor_str)
> + ble->vendor = NULL;
> +out:
> + if (vendor_str)
> + FREE(vendor_str);
> +
> + if (product_str)
> + FREE(product_str);
> +
> + return 1;
> }
>
> static int
> @@ -178,20 +196,16 @@ setup_default_blist (struct config * conf)
> {
> struct blentry * ble;
> struct hwentry *hwe;
> - char * str;
> int i;
>
> - str = STRDUP("!^(sd[a-z]|dasd[a-z]|nvme[0-9])");
> - if (!str)
> - return 1;
> - if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
> + if (store_ble(conf->blist_devnode, "!^(sd[a-z]|dasd[a-z]|nvme[0-9])"
> + , ORIGIN_DEFAULT)) {
> return 1;
> + }
>
> - str = STRDUP("(SCSI_IDENT_|ID_WWN)");
> - if (!str)
> - return 1;
> - if (store_ble(conf->elist_property, str, ORIGIN_DEFAULT))
> + if (store_ble(conf->elist_property, "(SCSI_IDENT_|ID_WWN)", ORIGIN_DEFAULT)) {
> return 1;
> + }
>
> vector_foreach_slot (conf->hwtable, hwe, i) {
> if (hwe->bl_product) {
> @@ -202,9 +216,7 @@ setup_default_blist (struct config * conf)
> return 1;
> ble = VECTOR_SLOT(conf->blist_device,
> VECTOR_SIZE(conf->blist_device) - 1);
> - if (set_ble_device(conf->blist_device,
> - STRDUP(hwe->vendor),
> - STRDUP(hwe->bl_product),
> + if (set_ble_device(conf->blist_device,hwe->vendor,hwe->bl_product,
> ORIGIN_DEFAULT)) {
> FREE(ble);
> vector_del_slot(conf->blist_device, VECTOR_SIZE(conf->blist_device) - 1);
> diff --git a/libmultipath/blacklist.h b/libmultipath/blacklist.h
> index 4305857d..b08e0978 100644
> --- a/libmultipath/blacklist.h
> +++ b/libmultipath/blacklist.h
> @@ -42,8 +42,8 @@ int filter_device (vector, vector, char *, char *, char *);
> int filter_path (struct config *, struct path *);
> int filter_property(struct config *, struct udev_device *, int, const char*);
> int filter_protocol(vector, vector, struct path *);
> -int store_ble (vector, char *, int);
> -int set_ble_device (vector, char *, char *, int);
> +int store_ble (vector, const char *, int);
> +int set_ble_device (vector, const char *, const char *, int);
> void free_blacklist (vector);
> void free_blacklist_device (vector);
> void merge_blacklist(vector);
> diff --git a/tests/blacklist.c b/tests/blacklist.c
> index d5c40898..84a3ba2f 100644
> --- a/tests/blacklist.c
> +++ b/tests/blacklist.c
> @@ -94,67 +94,62 @@ static int setup(void **state)
>
> blist_devnode_sdb = vector_alloc();
> if (!blist_devnode_sdb ||
> - store_ble(blist_devnode_sdb, strdup("sdb"), ORIGIN_CONFIG))
> + store_ble(blist_devnode_sdb, "sdb", ORIGIN_CONFIG))
> return -1;
> blist_devnode_sdb_inv = vector_alloc();
> if (!blist_devnode_sdb_inv ||
> - store_ble(blist_devnode_sdb_inv, strdup("!sdb"), ORIGIN_CONFIG))
> + store_ble(blist_devnode_sdb_inv, "!sdb", ORIGIN_CONFIG))
> return -1;
>
> blist_all = vector_alloc();
> - if (!blist_all || store_ble(blist_all, strdup(".*"), ORIGIN_CONFIG))
> + if (!blist_all || store_ble(blist_all, ".*", ORIGIN_CONFIG))
> return -1;
>
> blist_device_foo_bar = vector_alloc();
> if (!blist_device_foo_bar || alloc_ble_device(blist_device_foo_bar) ||
> - set_ble_device(blist_device_foo_bar, strdup("foo"), strdup("bar"),
> - ORIGIN_CONFIG))
> + set_ble_device(blist_device_foo_bar, "foo", "bar", ORIGIN_CONFIG))
> return -1;
> blist_device_foo_inv_bar = vector_alloc();
> if (!blist_device_foo_inv_bar ||
> alloc_ble_device(blist_device_foo_inv_bar) ||
> - set_ble_device(blist_device_foo_inv_bar, strdup("!foo"),
> - strdup("bar"), ORIGIN_CONFIG))
> + set_ble_device(blist_device_foo_inv_bar, "!foo", "bar", ORIGIN_CONFIG))
> return -1;
> blist_device_foo_bar_inv = vector_alloc();
> if (!blist_device_foo_bar_inv ||
> alloc_ble_device(blist_device_foo_bar_inv) ||
> - set_ble_device(blist_device_foo_bar_inv, strdup("foo"),
> - strdup("!bar"), ORIGIN_CONFIG))
> + set_ble_device(blist_device_foo_bar_inv, "foo", "!bar", ORIGIN_CONFIG))
> return -1;
>
> blist_device_all = vector_alloc();
> if (!blist_device_all || alloc_ble_device(blist_device_all) ||
> - set_ble_device(blist_device_all, strdup(".*"), strdup(".*"),
> - ORIGIN_CONFIG))
> + set_ble_device(blist_device_all, ".*", ".*", ORIGIN_CONFIG))
> return -1;
>
> blist_wwid_xyzzy = vector_alloc();
> if (!blist_wwid_xyzzy ||
> - store_ble(blist_wwid_xyzzy, strdup("xyzzy"), ORIGIN_CONFIG))
> + store_ble(blist_wwid_xyzzy, "xyzzy", ORIGIN_CONFIG))
> return -1;
> blist_wwid_xyzzy_inv = vector_alloc();
> if (!blist_wwid_xyzzy_inv ||
> - store_ble(blist_wwid_xyzzy_inv, strdup("!xyzzy"), ORIGIN_CONFIG))
> + store_ble(blist_wwid_xyzzy_inv, "!xyzzy", ORIGIN_CONFIG))
> return -1;
>
> blist_protocol_fcp = vector_alloc();
> if (!blist_protocol_fcp ||
> - store_ble(blist_protocol_fcp, strdup("scsi:fcp"), ORIGIN_CONFIG))
> + store_ble(blist_protocol_fcp, "scsi:fcp", ORIGIN_CONFIG))
> return -1;
> blist_protocol_fcp_inv = vector_alloc();
> if (!blist_protocol_fcp_inv ||
> - store_ble(blist_protocol_fcp_inv, strdup("!scsi:fcp"),
> - ORIGIN_CONFIG))
> + store_ble(blist_protocol_fcp_inv, "!scsi:fcp", ORIGIN_CONFIG))
> return -1;
>
> blist_property_wwn = vector_alloc();
> if (!blist_property_wwn ||
> - store_ble(blist_property_wwn, strdup("ID_WWN"), ORIGIN_CONFIG))
> + store_ble(blist_property_wwn, "ID_WWN", ORIGIN_CONFIG))
> return -1;
> blist_property_wwn_inv = vector_alloc();
> if (!blist_property_wwn_inv ||
> - store_ble(blist_property_wwn_inv, strdup("!ID_WWN"), ORIGIN_CONFIG))
> + store_ble(blist_property_wwn_inv, "!ID_WWN", ORIGIN_CONFIG))
> return -1;
>
> return 0;
> --
More information about the dm-devel
mailing list