[dm-devel] [PATCH 3/4] libmultipath: add_feature: allow only 1 feature

[Benjamin Marzinski]

On Thu, Sep 07, 2017 at 04:42:09PM -0500, Benjamin Marzinski wrote:
> On Tue, Aug 29, 2017 at 12:05:35AM +0200, Martin Wilck wrote:
> > The existing test "if feature is already present" doesn't work for
> > multiple features, and we are only using add_feature() for single
> > feature additions anyway. Simplify the code by not allowing spaces
> > in the feature string to be added. This way we can drop the
> > complex "count new features" code. Moreover, print an error message if
> > the existing features string is malformed.
> > 
> > The old code calculated the width of the feature count twice, and the
> > second time messed up the buffer length field passed to snprintf(); fix
> > that, too.  Finally, replace several strcat() invocations by one
> > strncpy() call to make the code easier to review.
> > 
> > Signed-off-by: Martin Wilck <mwilck at suse.com>
> > ---
> >  libmultipath/structs.c | 64 +++++++++++++++++---------------------------------
> >  1 file changed, 21 insertions(+), 43 deletions(-)
> > 
> > diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> > index 11e33676..828e7907 100644
> > --- a/libmultipath/structs.c
> > +++ b/libmultipath/structs.c
> > @@ -516,8 +516,7 @@ void setup_feature(struct multipath *mpp, char *feature)
> >  int add_feature(char **f, const char *n)
> >  {
> >  	int c = 0, d, l;
> > -	char *e, *p, *t;
> > -	const char *q;
> > +	char *e, *t;
> >  
> >  	if (!f)
> >  		return 1;
> > @@ -526,6 +525,11 @@ int add_feature(char **f, const char *n)
> >  	if (!n || *n == '0')
> >  		return 0;
> >  
> > +	if (strchr(n, ' ') != NULL) {
> > +		condlog(0, "internal error: feature \"%s\" contains spaces", n);
> > +		return 1;
> > +	}
> > +
> >  	/* default feature is null */
> >  	if(!*f)
> >  	{
> > @@ -543,55 +547,29 @@ int add_feature(char **f, const char *n)
> >  
> >  	/* Get feature count */
> >  	c = strtoul(*f, &e, 10);
> > -	if (*f == e)
> > -		/* parse error */
> > +	if (*f == e || (*e != ' ' && *e != '\0')) {
> > +		condlog(0, "parse error in feature string \"%s\"", *f);
> >  		return 1;
> > -
> > -	/* Check if we need to increase feature count space */
> > -	l = strlen(*f) + strlen(n) + 1;
> > -
> > -	/* Count new features */
> > -	if ((c % 10) == 9)
> > -		l++;
> > -	c++;
> > -	q = n;
> > -	while (*q != '\0') {
> > -		if (*q == ' ' && q[1] != '\0' && q[1] != ' ') {
> > -			if ((c % 10) == 9)
> > -				l++;
> > -			c++;
> > -		}
> > -		q++;
> >  	}
> >  
> > +	/* Add 1 digit and 1 space */
> > +	l = strlen(e) + strlen(n) + 2;
> > +
> > +	c++;
> > +	/* Check if we need more digits for feature count */
> > +	for (d = c; d >= 10; d /= 10)
> > +		l++;
> > +
> >  	t = MALLOC(l + 1);
> >  	if (!t)
> >  		return 1;
> >  
> > -	memset(t, 0, l + 1);
> > +	/* e: old feature string with leading space, or "" */
> > +	if (*e == ' ')
> > +		while (*(e + 1) == ' ')
> > +			e++;
> >  
> > -	/* Update feature count */
> > -	d = c;
> > -	l = 1;
> > -	while (d > 9) {
> > -		d /= 10;
> > -		l++;
> > -	}
> > -	p = t;
> > -	snprintf(p, l + 2, "%0d ", c);
> > -
> > -	/* Copy the feature string */
> > -	p = strchr(*f, ' ');
> > -
> > -	if (p) {
> > -		while (*p == ' ')
> > -			p++;
> > -		strcat(t, p);
> > -		strcat(t, " ");
> > -	} else {
> > -		p = t + strlen(t);
> > -	}
> > -	strcat(t, n);
> > +	snprintf(t, l + 1, "%0d%s %s", c, e, n);
> 
> Just one nit, that I know I'm guilty of too (so Coverity says).  If
> you're sure that you have enough size for the string, you don't need
> snprintf. If you want to make sure that the string isn't too big, you
> should probably use "l" instead of "l + 1", so that you know that the
> string will get null termintated (from your earlier memset), and you
> won't read off the end of the array later. Otherwise, ACK. 

Nevermind, I was having a brain fart and thinking of strncpy.

-Ben

> 
> >  
> >  	FREE(*f);
> >  	*f = t;
> > -- 
> > 2.14.0
> 
> Reviewed-by: Benjamin Marzinski <bmarzins at redhat.com>
> 
> -Ben
> 
> --
> dm-devel mailing list
> dm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel