[PATCH 0/4] Coverity fixes

[PATCH 0/4] Coverity fixes

[Benjamin Marzinski]

These are fixes to some issues that coverity pointed out.

Benjamin Marzinski (4):
  libmultipath: handle clock_gettime failures in tur checker
  kpartx: fail if dup() of dasd file descriptor fails
  multipathd: fix REALLOC_REPLY with max length reply
  multipathd: handle NULL return from genhelp_handler

 kpartx/dasd.c               |  2 ++
 libmultipath/checkers/tur.c | 19 +++++++++++++++----
 multipathd/cli.c            |  6 +++++-
 multipathd/cli.h            | 17 +++++++++--------
 4 files changed, 31 insertions(+), 13 deletions(-)

-- 
2.17.2

[PATCH 1/4] libmultipath: handle clock_gettime failures in tur checker

[Benjamin Marzinski]

If clock_gettime() fails, and multipathd can't figure out when it should
time out, it should just default to assuming that it has already timed
out. Found by coverity.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 6b08dbbb..717353ef 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -290,7 +290,12 @@ static void *tur_thread(void *ctx)
 
 static void tur_timeout(struct timespec *tsp)
 {
-	clock_gettime(CLOCK_MONOTONIC, tsp);
+	if (clock_gettime(CLOCK_MONOTONIC, tsp) != 0) {
+		/* can't get time. clear tsp to not wait */
+		tsp->tv_sec = 0;
+		tsp->tv_nsec = 0;
+		return;
+	}
 	tsp->tv_nsec += 1000 * 1000; /* 1 millisecond */
 	normalize_timespec(tsp);
 }
@@ -300,8 +305,12 @@ static void tur_set_async_timeout(struct checker *c)
 	struct tur_checker_context *ct = c->context;
 	struct timespec now;
 
-	clock_gettime(CLOCK_MONOTONIC, &now);
-	ct->time = now.tv_sec + c->timeout;
+	if (clock_gettime(CLOCK_MONOTONIC, &now) != 0)
+		/* can't get time. clear time to always timeout on
+		 * next path check */
+		ct->time = 0;
+	else
+		ct->time = now.tv_sec + c->timeout;
 }
 
 static int tur_check_async_timeout(struct checker *c)
@@ -309,7 +318,9 @@ static int tur_check_async_timeout(struct checker *c)
 	struct tur_checker_context *ct = c->context;
 	struct timespec now;
 
-	clock_gettime(CLOCK_MONOTONIC, &now);
+	if (clock_gettime(CLOCK_MONOTONIC, &now) != 0)
+		/* can't get time. assume we've timed out */
+		return 1;
 	return (now.tv_sec > ct->time);
 }
 
-- 
2.17.2

[PATCH 2/4] kpartx: fail if dup() of dasd file descriptor fails

[Benjamin Marzinski]

If kpartx fails to create a copy of the dasd file descriptor, it should
fail, instead of treating the error value as a valid fd. Found by
coverity.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 kpartx/dasd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kpartx/dasd.c b/kpartx/dasd.c
index 61b609a5..d95d8ca0 100644
--- a/kpartx/dasd.c
+++ b/kpartx/dasd.c
@@ -138,6 +138,8 @@ read_dasd_pt(int fd, struct slice all, struct slice *sp, int ns)
 			return -1;
 	} else {
 		fd_dasd = dup(fd);
+		if (fd_dasd < 0)
+			return -1;
 	}
 
 	if (ioctl(fd_dasd, BIODASDINFO, (unsigned long)&info) != 0) {
-- 
2.17.2

[PATCH 3/4] multipathd: fix REALLOC_REPLY with max length reply

[Benjamin Marzinski]

Commit cd5a9797e added code to REALLOC_REPLY() that intended to stop
growing the reply buffer after it reached a maximum size. However this
code didn't stop the realloc() from happening. Worse, if the realloc()
failed, multipathd would double free the reply buffer. Found by
Coverity.

Fixes: cd5a9797e "libmpathcmd(coverity): limit reply length"
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/cli.h | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/multipathd/cli.h b/multipathd/cli.h
index f3fa077a..32dcffac 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -100,15 +100,16 @@ enum {
 			if (m >= MAX_REPLY_LEN) {		\
 				condlog(1, "Warning: max reply length exceeded"); \
 				free(tmp);			\
-				r = NULL;			\
+				(r) = NULL;			\
+			} else {				\
+				(r) = REALLOC((r), (m) * 2);	\
+				if ((r)) {			\
+					memset((r) + (m), 0, (m)); \
+					(m) *= 2;		\
+				}				\
+				else				\
+					free(tmp);		\
 			}					\
-			(r) = REALLOC((r), (m) * 2);		\
-			if ((r)) {				\
-				memset((r) + (m), 0, (m));	\
-				(m) *= 2;			\
-			}					\
-			else					\
-				free(tmp);			\
 		}						\
 	} while (0)
 
-- 
2.17.2

[PATCH 4/4] multipathd: handle NULL return from genhelp_handler

[Benjamin Marzinski]

parse_cmd() wasn't checking if genhelp_handler() returned NULL. It was simply
assuming that it got a string. On NULL, it now returns an error. Found by
Coverity.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/cli.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/multipathd/cli.c b/multipathd/cli.c
index ca176a99..17795b61 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -467,6 +467,8 @@ parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
 
 	if (r) {
 		*reply = genhelp_handler(cmd, r);
+		if (*reply == NULL)
+			return EINVAL;
 		*len = strlen(*reply) + 1;
 		return 0;
 	}
@@ -474,9 +476,11 @@ parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
 	h = find_handler(fingerprint(cmdvec));
 
 	if (!h || !h->fn) {
+		free_keys(cmdvec);
 		*reply = genhelp_handler(cmd, EINVAL);
+		if (*reply == NULL)
+			return EINVAL;
 		*len = strlen(*reply) + 1;
-		free_keys(cmdvec);
 		return 0;
 	}
 
-- 
2.17.2

Re: [PATCH 1/4] libmultipath: handle clock_gettime failures in tur checker

[Martin Wilck]

On Fri, 2019-05-17 at 11:14 -0500, Benjamin Marzinski wrote:
> If clock_gettime() fails, and multipathd can't figure out when it
> should
> time out, it should just default to assuming that it has already
> timed
> out. Found by coverity.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/checkers/tur.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)

I know coverity picks on this, but I don't like the result. It's
superfluous IMO, and uglifies the code. 

Other than passing an invalid address (unlikely because basically every
caller uses memory from the stack), the only possible reason for
failure in clock_gettime is "EINVAL - The clk_id specified is not
supported on this system".

We have this code in pthread_cond_init_mono():

	res = pthread_condattr_setclock(&attr, CLOCK_MONOTONIC);
	assert(res == 0);

this is called when initializing config_cond. So multipathd at least
won't even start if CLOCK_MONOTONIC is unsupported.

If that's not enough, I don't mind putting such a check in
mpath_lib_init() and refuse to start on systems without
CLOCK_MONOTONIC, then stop bothering with the return value of
clock_gettime() in the rest of the code.

Regards,
Martin


> diff --git a/libmultipath/checkers/tur.c
> b/libmultipath/checkers/tur.c
> index 6b08dbbb..717353ef 100644
> --- a/libmultipath/checkers/tur.c
> +++ b/libmultipath/checkers/tur.c
> @@ -290,7 +290,12 @@ static void *tur_thread(void *ctx)
>  
>  static void tur_timeout(struct timespec *tsp)
>  {
> -	clock_gettime(CLOCK_MONOTONIC, tsp);
> +	if (clock_gettime(CLOCK_MONOTONIC, tsp) != 0) {
> +		/* can't get time. clear tsp to not wait */
> +		tsp->tv_sec = 0;
> +		tsp->tv_nsec = 0;
> +		return;
> +	}
>  	tsp->tv_nsec += 1000 * 1000; /* 1 millisecond */
>  	normalize_timespec(tsp);
>  }
> @@ -300,8 +305,12 @@ static void tur_set_async_timeout(struct checker
> *c)
>  	struct tur_checker_context *ct = c->context;
>  	struct timespec now;
>  
> -	clock_gettime(CLOCK_MONOTONIC, &now);
> -	ct->time = now.tv_sec + c->timeout;
> +	if (clock_gettime(CLOCK_MONOTONIC, &now) != 0)
> +		/* can't get time. clear time to always timeout on
> +		 * next path check */
> +		ct->time = 0;
> +	else
> +		ct->time = now.tv_sec + c->timeout;
>  }
>  
>  static int tur_check_async_timeout(struct checker *c)
> @@ -309,7 +318,9 @@ static int tur_check_async_timeout(struct checker
> *c)
>  	struct tur_checker_context *ct = c->context;
>  	struct timespec now;
>  
> -	clock_gettime(CLOCK_MONOTONIC, &now);
> +	if (clock_gettime(CLOCK_MONOTONIC, &now) != 0)
> +		/* can't get time. assume we've timed out */
> +		return 1;
>  	return (now.tv_sec > ct->time);
>  }
>  

Re: [PATCH 0/4] Coverity fixes

[Martin Wilck]

On Fri, 2019-05-17 at 11:14 -0500, Benjamin Marzinski wrote:
> These are fixes to some issues that coverity pointed out.
> 
> Benjamin Marzinski (4):
>   libmultipath: handle clock_gettime failures in tur checker
>   kpartx: fail if dup() of dasd file descriptor fails
>   multipathd: fix REALLOC_REPLY with max length reply
>   multipathd: handle NULL return from genhelp_handler
> 
>  kpartx/dasd.c               |  2 ++
>  libmultipath/checkers/tur.c | 19 +++++++++++++++----
>  multipathd/cli.c            |  6 +++++-
>  multipathd/cli.h            | 17 +++++++++--------
>  4 files changed, 31 insertions(+), 13 deletions(-)
> 

ACK for the series, except 1/4.

Martin

Re: [PATCH 1/4] libmultipath: handle clock_gettime failures in tur checker

[Benjamin Marzinski]

On Fri, May 17, 2019 at 11:55:48PM +0200, Martin Wilck wrote:
> On Fri, 2019-05-17 at 11:14 -0500, Benjamin Marzinski wrote:
> > If clock_gettime() fails, and multipathd can't figure out when it
> > should
> > time out, it should just default to assuming that it has already
> > timed
> > out. Found by coverity.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/checkers/tur.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> I know coverity picks on this, but I don't like the result. It's
> superfluous IMO, and uglifies the code. 
> 
> Other than passing an invalid address (unlikely because basically every
> caller uses memory from the stack), the only possible reason for
> failure in clock_gettime is "EINVAL - The clk_id specified is not
> supported on this system".
> 
> We have this code in pthread_cond_init_mono():
> 
> 	res = pthread_condattr_setclock(&attr, CLOCK_MONOTONIC);
> 	assert(res == 0);
> 
> this is called when initializing config_cond. So multipathd at least
> won't even start if CLOCK_MONOTONIC is unsupported.
> 
> If that's not enough, I don't mind putting such a check in
> mpath_lib_init() and refuse to start on systems without
> CLOCK_MONOTONIC, then stop bothering with the return value of
> clock_gettime() in the rest of the code.
> 
> Regards,
> Martin

I'm fine with dropping this one. We just have so many other checks for
the return on this call that I thought that there was possibly some
important error condition that the man page didn't mention.

I'll repost the set without it.

-Ben

> 
> 
> > diff --git a/libmultipath/checkers/tur.c
> > b/libmultipath/checkers/tur.c
> > index 6b08dbbb..717353ef 100644
> > --- a/libmultipath/checkers/tur.c
> > +++ b/libmultipath/checkers/tur.c
> > @@ -290,7 +290,12 @@ static void *tur_thread(void *ctx)
> >  
> >  static void tur_timeout(struct timespec *tsp)
> >  {
> > -	clock_gettime(CLOCK_MONOTONIC, tsp);
> > +	if (clock_gettime(CLOCK_MONOTONIC, tsp) != 0) {
> > +		/* can't get time. clear tsp to not wait */
> > +		tsp->tv_sec = 0;
> > +		tsp->tv_nsec = 0;
> > +		return;
> > +	}
> >  	tsp->tv_nsec += 1000 * 1000; /* 1 millisecond */
> >  	normalize_timespec(tsp);
> >  }
> > @@ -300,8 +305,12 @@ static void tur_set_async_timeout(struct checker
> > *c)
> >  	struct tur_checker_context *ct = c->context;
> >  	struct timespec now;
> >  
> > -	clock_gettime(CLOCK_MONOTONIC, &now);
> > -	ct->time = now.tv_sec + c->timeout;
> > +	if (clock_gettime(CLOCK_MONOTONIC, &now) != 0)
> > +		/* can't get time. clear time to always timeout on
> > +		 * next path check */
> > +		ct->time = 0;
> > +	else
> > +		ct->time = now.tv_sec + c->timeout;
> >  }
> >  
> >  static int tur_check_async_timeout(struct checker *c)
> > @@ -309,7 +318,9 @@ static int tur_check_async_timeout(struct checker
> > *c)
> >  	struct tur_checker_context *ct = c->context;
> >  	struct timespec now;
> >  
> > -	clock_gettime(CLOCK_MONOTONIC, &now);
> > +	if (clock_gettime(CLOCK_MONOTONIC, &now) != 0)
> > +		/* can't get time. assume we've timed out */
> > +		return 1;
> >  	return (now.tv_sec > ct->time);
> >  }
> >  
>