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@xxxxxxxxxx>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/dm-devel
>
>
> .
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
