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