From mboxrd@z Thu Jan 1 00:00:00 1970 From: lixiaokeng Subject: Re: [PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map Date: Wed, 9 Sep 2020 11:18:24 +0800 Message-ID: <3154dc82-e5cf-5d34-6955-04a0ad6e01f0@huawei.com> References: <37544d4c-950f-4281-3b66-e4d1884c5167@huawei.com> <40cf77df-eaa3-fb72-69fd-9f865d12dc34@huawei.com> <20200904213048.GE11108@octiron.msp.redhat.com> <8dd72a70-991a-cb7d-3279-3d5036df1b9d@huawei.com> <20200908154540.GK11108@octiron.msp.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: 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: Martin Wilck , Benjamin Marzinski Cc: linfeilong , dm-devel mailing list , "liuzhiqiang (I)" List-Id: dm-devel.ids 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