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 <[email protected]> >> Signed-off-by: Zhiqiang Liu <[email protected]> >> --- >> 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 > [email protected] > https://www.redhat.com/mailman/listinfo/dm-devel > > > . >
-- dm-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/dm-devel
