[dm-devel] [PATCH 08/14] libmultipath: donot free *dst if REALLOC fails in, merge_words

[lixiaokeng]

Hi Ben:
  I think you are right. We should not keep them with partial information.
Thanks for your review.

-Lixiaokeng

On 2020/9/5 5:11, Benjamin Marzinski wrote:
> On Wed, Sep 02, 2020 at 03:20:29PM +0800, lixiaokeng wrote:
>> In merge_words func, if REALLOC() fails, the input *dst will
>> be freed. If so, mpp->hwhandler| mpp->features|mpp->selector
>> may be set to NULL after calling merge_words func in
>> disassemble_map func. This may cause accessing freed memory
>> problem.
>>
> 
> I'm not sure that this is the right way to fix the issue you're seeing.
> If merge_words() frees mpp->hwhandler| mpp->features|mpp->selector, it
> also sets them to NULL.  I don't see any place in disassemble_map()
> where these would be accessed if merge_words() freed them.  Even with
> this fix, there are still cases where disassemble_map() will return 1,
> with these members set to NULL. If there is something that is
> dereferencing them without checking if they're NULL, we should fix that.
> But simply making them include partial information doesn't seem like it
> fixes anything.
> 
> Am I missing something here?
> 
> -Ben
> 
>> Here, we donot free *dst if REALLOC() fails in merge_words func.
>>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26 at huawei.com>
>> Signed-off-by: Lixiaokeng <lixiaokeng at huawei.com>
>> ---
>>  libmultipath/dmparser.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
>> index c1031616..482e9d0e 100644
>> --- a/libmultipath/dmparser.c
>> +++ b/libmultipath/dmparser.c
>> @@ -26,13 +26,12 @@ merge_words(char **dst, const char *word)
>>
>>  	dstlen = strlen(*dst);
>>  	len = dstlen + strlen(word) + 2;
>> -	*dst = REALLOC(*dst, len);
>> +	p = REALLOC(*dst, len);
>>
>> -	if (!*dst) {
>> -		free(p);
>> +	if (!p)
>>  		return 1;
>> -	}
>>
>> +	*dst = p;
>>  	p = *dst + dstlen;
>>  	*p = ' ';
>>  	++p;
>> -- 
> 
> 
> .
>