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

[Martin Wilck]

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