[dm-devel] [PATCH] libmultipath: fix a memory leak in set_ble_device
lixiaokeng
lixiaokeng at huawei.com
Tue Aug 11 07:18:39 UTC 2020
Hello Martin:
Thanks for your reviews. I will modify this patch with your advice
and send it again.
On 2020/8/10 21:22, Martin Wilck wrote:
> Hello Lixiaokeng,
>
> On Thu, 2020-07-30 at 21:27 +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.
>>
>> Signed-off-by: Lixiaokeng <lixiaokeng at huawei.com>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26 at huawei.com>
>
> Thanks for the patch, it looks good. I have some minor formal issues
> below.
>
> Regards,
> Martin
>
>
>>
>> ---
>> 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)
>
> Please use kernel formatting conventions for added code, i.e.
> "const char *str".
>
>> {
>> 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);
>
> These if's are superfluous; free() ignores NULL pointers.
> Btw, while using FREE() here isn't wrong, I tend to not use it any
> more. IMO it's better to simply call free(), and set the pointer to
> NULL explicitly when necessary. multipath-tools builtin memory
> allocation debugging facility (dbg_free() etc) facility is obsolete, as
> valgrind etc. are far more powerful.
>
>> +
>> + 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