[dm-devel] [PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map

[lixiaokeng]

On 2020/9/9 0:35, Martin Wilck wrote:
> On Tue, 2020-09-08 at 10:45 -0500, Benjamin Marzinski wrote:
>> On Mon, Sep 07, 2020 at 08:21:28PM +0800, lixiaokeng wrote:
>>>>> @@ -86,9 +86,12 @@ assemble_map (struct multipath * mp, char *
>>>>> params, int len)
>>>>>  	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
>>>>>  		add_feature(&mp->features, retain_hwhandler);
>>>>>
>>>>> -	f = STRDUP(mp->features);
>>>>
>>>> clearly strdup()ing without checking if mp->features NULL is
>>>> incorrect.
>>>> However, I'm not sure that we need to fail if mp->features is
>>>> NULL.
>>>> instead, int the APPEND call, we could use the gcc ternary
>>>> operator
>>>> extension
>>>>
>>>> (mp->features)?: "0"
>>>>
>>>> to use "0" if mp->features is NULL.
>>>>
>>>> Also, have you seen this actually occur?  Or is this just a
>>>> theoretical
>>>> issue that you've found from reading the code.  It seems like
>>>> setup_map() will always call select_features() before calling
>>>> assemble_map(), which should mean that mp->features will always
>>>> be set
>>>> in this case. Perhaps I'm missing something here.
>>>>
>>>> -Ben
>>>>
>>> Hi Ben,
>>>   This just a theoretical issue and I did not see it. But it's not
>>> necessary
>>> to call strdup. In your opinion, need multipath be checked?  I will
>>> make new
>>> patch with your suggestion.
>>
>> Since we don't believe it's possible for mp->features (or mp-
>>> hwhandler)
>> to be set to NULL here, it makes sense to print an error if it is
>> NULL.
>> So, I guess my suggestion would be to print an error message if
>> mp->features or mp->hwhandler are NULL, but to assemble the map
>> anyway,
>> using the default value of "0" if they are NULL. That's how
>> assemble_map() currently handles failures in add_feature().
>> add_feature() will print an error, but assemble_map() will go ahead
>> with
>> assembling the map.
>>
>> I'm willing to be convinced that there is a better solution, however.
> 
> What about this:
> 
> assemble_map() is only called from setup_map(), which sets mp->features 
> in select_features(). So what we should do is check for NULL after
> select_features(), or have that function return an error code if strdup
> fails, and bail out early in setup_map() in that case.
> 
> Then we simply need to add a comment in assemble_map() saying that 
> mp->features must be non-null when this function is called.
> 
> As I said earlier, I'm of course not against checking function
> parameters, but here we should fail to setup a "struct multipath" in
> the first place in setup map(), rather than returning an incompletely
> initialized one. If we handle it this way, we don't need to check the
> fields of struct multipath over and over again. Similar arguments hold
> for other structs.
> 
> Of course this kind of assumption needs to be better documented in the
> code.
> 
> Regards
> Martin
> 
> 
Hi Martin,
   If I don't misunderstand, we check feature after select_features and
return 1 if feature is NULL in setup_map. We delete strdup and add
message "mp->features must be non-null" in assemble_map. Like this:
---
select_features(conf, mpp);
if (!mpp->featrues)
	return 1;

---
/* mp->feature must not be NULL */
APPEND(p, end, "%s %s %i %i", mp->features, mp->hwhandler
---

Regards
Lixiaokeng