From mboxrd@z Thu Jan 1 00:00:00 1970 From: lixiaokeng Subject: Re: [PATCH 08/14] libmultipath: donot free *dst if REALLOC fails in, merge_words Date: Mon, 7 Sep 2020 19:58:50 +0800 Message-ID: References: <37544d4c-950f-4281-3b66-e4d1884c5167@huawei.com> <20200904211109.GD11108@octiron.msp.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20200904211109.GD11108@octiron.msp.redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com Content-Language: en-GB To: Benjamin Marzinski Cc: linfeilong , dm-devel mailing list , Martin Wilck , "liuzhiqiang (I)" List-Id: dm-devel.ids 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 >> Signed-off-by: Lixiaokeng >> --- >> 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; >> -- > > > . >