* [PATCH V2] libmultipath: fix a memory leak in set_ble_device
@ 2020-08-11 7:23 lixiaokeng
2020-08-11 9:32 ` Martin Wilck
0 siblings, 1 reply; 4+ messages in thread
From: lixiaokeng @ 2020-08-11 7:23 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski, dm-devel
Cc: linfeilong, liuzhiqiang26, lutianxiong
In set_ble_device func, if blist is NULL or ble is NULL,
the vendor and product isn't freed. 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.
Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
libmultipath/blacklist.c | 81 ++++++++++++++++++++++------------------
libmultipath/blacklist.h | 4 +-
tests/blacklist.c | 31 +++++++--------
3 files changed, 59 insertions(+), 57 deletions(-)
diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
index db58ccca..a9ad9de4 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,40 @@ int set_ble_device(vector blist, char * vendor, char * product, int origin)
return 1;
if (vendor) {
- regex_str = check_invert(vendor, &ble->vendor_invert);
- if (regcomp(&ble->vendor_reg, regex_str,
- REG_EXTENDED|REG_NOSUB)) {
- FREE(vendor);
- if (product)
- FREE(product);
- return 1;
- }
- ble->vendor = vendor;
+ 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))
+ goto out;
+
+ ble->vendor = vendor_str;
}
if (product) {
- regex_str = check_invert(product, &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;
- }
- ble->product = product;
+ 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))
+ goto out1;
+
+ ble->product = product_str;
}
ble->origin = origin;
return 0;
+out1:
+ if (vendor_str)
+ ble->vendor = NULL;
+out:
+ free(vendor_str);
+ vendor_str = NULL;
+
+ free(product_str);
+ product_str = NULL;
+
+ return 1;
}
static int
@@ -178,19 +194,12 @@ 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) {
@@ -202,9 +211,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;
--
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH V2] libmultipath: fix a memory leak in set_ble_device
2020-08-11 7:23 [PATCH V2] libmultipath: fix a memory leak in set_ble_device lixiaokeng
@ 2020-08-11 9:32 ` Martin Wilck
2020-08-13 6:17 ` lixiaokeng
2020-08-13 8:00 ` lixiaokeng
0 siblings, 2 replies; 4+ messages in thread
From: Martin Wilck @ 2020-08-11 9:32 UTC (permalink / raw)
To: lixiaokeng, Christophe Varoqui, Benjamin Marzinski, dm-devel
Cc: linfeilong, liuzhiqiang26, lutianxiong
Hi Liaxiaokeng,
thanks again. I still have minor issues, see below.
On Tue, 2020-08-11 at 15:23 +0800, lixiaokeng wrote:
> In set_ble_device func, if blist is NULL or ble is NULL,
> the vendor and product isn't freed. 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.
>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
> libmultipath/blacklist.c | 81 ++++++++++++++++++++++--------------
> ----
> libmultipath/blacklist.h | 4 +-
> tests/blacklist.c | 31 +++++++--------
> 3 files changed, 59 insertions(+), 57 deletions(-)
>
> ...
>
> @@ -93,31 +100,40 @@ int set_ble_device(vector blist, char * vendor,
> char * product, int origin)
> return 1;
>
> if (vendor) {
> - regex_str = check_invert(vendor, &ble->vendor_invert);
> - if (regcomp(&ble->vendor_reg, regex_str,
> - REG_EXTENDED|REG_NOSUB)) {
> - FREE(vendor);
> - if (product)
> - FREE(product);
> - return 1;
> - }
> - ble->vendor = vendor;
> + 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))
> + goto out;
> +
> + ble->vendor = vendor_str;
> }
> if (product) {
> - regex_str = check_invert(product, &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;
> - }
> - ble->product = product;
> + 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))
> + goto out1;
> +
> + ble->product = product_str;
> }
> ble->origin = origin;
> return 0;
> +out1:
> + if (vendor_str)
> + ble->vendor = NULL;
> +out:
> + free(vendor_str);
> + vendor_str = NULL;
> +
> + free(product_str);
> + product_str = NULL;
> +
> + return 1;
> }
Thinking about it again, I believe the error handling code should look
like this:
out1:
if (vendor) {
regfree(&ble->vendor_reg);
ble->vendor_reg = NULL;
ble->vendor = NULL;
}
out:
free(vendor_str);
free(product_str);
return 1;
Rationale: vendor_str and product_str are local variables, there's no
point in setting them to NULL. But the ble fields need careful
treatment, as vendor and product can either be set in a single call of
this function, or in two separate calls. You should test "vendor"
rather than "vendor_str" in the "out1" clause to make this logic
obvious, even though you never pass "out1" if allocating vendor_str
failed.
Note the regfree() I added. It's missing in the current code as well.
Regards,
Martin
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V2] libmultipath: fix a memory leak in set_ble_device
2020-08-11 9:32 ` Martin Wilck
@ 2020-08-13 6:17 ` lixiaokeng
2020-08-13 8:00 ` lixiaokeng
1 sibling, 0 replies; 4+ messages in thread
From: lixiaokeng @ 2020-08-13 6:17 UTC (permalink / raw)
To: Martin Wilck, Christophe Varoqui, Benjamin Marzinski, dm-devel
Cc: linfeilong, liuzhiqiang26, lutianxiong
Hi,
Thanks for your advices. I will modify it.
On 2020/8/11 17:32, Martin Wilck wrote:
> Hi Liaxiaokeng,
>
> thanks again. I still have minor issues, see below.
>
> On Tue, 2020-08-11 at 15:23 +0800, lixiaokeng wrote:
>
>> In set_ble_device func, if blist is NULL or ble is NULL,
>> the vendor and product isn't freed. 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.
>>
>> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> ---
>> libmultipath/blacklist.c | 81 ++++++++++++++++++++++--------------
>> ----
>> libmultipath/blacklist.h | 4 +-
>> tests/blacklist.c | 31 +++++++--------
>> 3 files changed, 59 insertions(+), 57 deletions(-)
>>
>> ...
>>
>> @@ -93,31 +100,40 @@ int set_ble_device(vector blist, char * vendor,
>> char * product, int origin)
>> return 1;
>>
>> if (vendor) {
>> - regex_str = check_invert(vendor, &ble->vendor_invert);
>> - if (regcomp(&ble->vendor_reg, regex_str,
>> - REG_EXTENDED|REG_NOSUB)) {
>> - FREE(vendor);
>> - if (product)
>> - FREE(product);
>> - return 1;
>> - }
>> - ble->vendor = vendor;
>> + 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))
>> + goto out;
>> +
>> + ble->vendor = vendor_str;
>> }
>> if (product) {
>> - regex_str = check_invert(product, &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;
>> - }
>> - ble->product = product;
>> + 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))
>> + goto out1;
>> +
>> + ble->product = product_str;
>> }
>> ble->origin = origin;
>> return 0;
>> +out1:
>> + if (vendor_str)
>> + ble->vendor = NULL;
>> +out:
>> + free(vendor_str);
>> + vendor_str = NULL;
>> +
>> + free(product_str);
>> + product_str = NULL;
>> +
>> + return 1;
>> }
>
> Thinking about it again, I believe the error handling code should look
> like this:
>
> out1:
> if (vendor) {
> regfree(&ble->vendor_reg);
> ble->vendor_reg = NULL;
> ble->vendor = NULL;
> }
> out:
> free(vendor_str);
> free(product_str);
> return 1;
>
> Rationale: vendor_str and product_str are local variables, there's no
> point in setting them to NULL. But the ble fields need careful
> treatment, as vendor and product can either be set in a single call of
> this function, or in two separate calls. You should test "vendor"
> rather than "vendor_str" in the "out1" clause to make this logic
> obvious, even though you never pass "out1" if allocating vendor_str
> failed.
>
> Note the regfree() I added. It's missing in the current code as well.
>
> Regards,
> Martin
>
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>
>
> .
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V2] libmultipath: fix a memory leak in set_ble_device
2020-08-11 9:32 ` Martin Wilck
2020-08-13 6:17 ` lixiaokeng
@ 2020-08-13 8:00 ` lixiaokeng
1 sibling, 0 replies; 4+ messages in thread
From: lixiaokeng @ 2020-08-13 8:00 UTC (permalink / raw)
To: Martin Wilck, Christophe Varoqui, Benjamin Marzinski, dm-devel
Cc: linfeilong, liuzhiqiang26, lutianxiong
Hi Martin
I'm sorry for send "[PATCH V3] libmultipath: fix a memory leak in set_ble_device"
to you before finishing compile. Here is some question in ble->vendor_reg = NULL.
The type of ble->vendor_reg is regex_t but not void *, so it can not be set to
NULL. Do you have any good idea about how to set ble->vendor_reg. You can ignore
the email [PATCH V3] libmultipath: fix a memory leak in set_ble_device.
Lixiaokeng
> Thinking about it again, I believe the error handling code should look
> like this:
>
> out1:
> if (vendor) {
> regfree(&ble->vendor_reg);
> ble->vendor_reg = NULL;
> ble->vendor = NULL;
> }
> out:
> free(vendor_str);
> free(product_str);
> return 1;
>
> Rationale: vendor_str and product_str are local variables, there's no
> point in setting them to NULL. But the ble fields need careful
> treatment, as vendor and product can either be set in a single call of
> this function, or in two separate calls. You should test "vendor"
> rather than "vendor_str" in the "out1" clause to make this logic
> obvious, even though you never pass "out1" if allocating vendor_str
> failed.
>
> Note the regfree() I added. It's missing in the current code as well.
>
> Regards,
> Martin
>
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>
>
> .
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-13 8:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-11 7:23 [PATCH V2] libmultipath: fix a memory leak in set_ble_device lixiaokeng
2020-08-11 9:32 ` Martin Wilck
2020-08-13 6:17 ` lixiaokeng
2020-08-13 8:00 ` lixiaokeng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.