[PATCH v2 0/8] multipath-tools: static analyzer fixes

[PATCH v2 0/8] multipath-tools: static analyzer fixes

[Martin Wilck]

Xose has kindly informed me about the warnings from Fedora's static
code analysis tool.

I'm sending a couple of related fixes here.

In the cover letter of v1 of this series [1], I'd included a discussion
of what I considered false positives, but many of those might actually
be real, as subsequent discussion with Ben Marzinski has revealed.

So I'm not repeating this list here.

Regards
Martin

Changes v1 -> v2:

- Added links to specific anchors on the static analysis page in
  commit messages
- 04/08: replaced this one after discussion with Ben
- 07/08, 08/08: new minor cleanups.

[1] https://lore.kernel.org/dm-devel/20250505163007.59352-1-mwilck@suse.com/

Martin Wilck (8):
  kpartx_id: fix shellcheck reported errors
  kpartx: fix file descriptor leak
  libmpathpersist: fix memory leak in mpath_prout_rel()
  libmpathutil: vector_del_slot: modify v->allocated if realloc fails
  libmultipath: fix undefined behavior in 31-bit shift
  libmultipath: prioritizers/iet: fix possible NULL dereference
  libmpathutil: remove vector_repack()
  libmpathutil: remove VECTOR_DEFAULT_SIZE macro

 kpartx/kpartx.c                     | 14 ++++++-------
 kpartx/kpartx_id                    |  8 ++++----
 libmpathpersist/mpath_persist_int.c | 12 +++++------
 libmpathutil/vector.c               | 31 +++++++++++------------------
 libmpathutil/vector.h               |  4 +---
 libmultipath/nvme/nvme-ioctl.c      |  2 +-
 libmultipath/prioritizers/iet.c     |  3 +++
 7 files changed, 34 insertions(+), 40 deletions(-)

-- 
2.49.0

[PATCH v2 1/8] kpartx_id: fix shellcheck reported errors

[Martin Wilck]

Found by Fedora's static analysis [1].

[1] https://openscanhub.fedoraproject.org/task/51915/log/device-mapper-multipath-0.11.1-1.fc43/scan-results.html#def41

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 kpartx/kpartx_id | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kpartx/kpartx_id b/kpartx/kpartx_id
index f9211d2..ffd3424 100755
--- a/kpartx/kpartx_id
+++ b/kpartx/kpartx_id
@@ -25,7 +25,7 @@ MAJOR=$1
 MINOR=$2
 UUID=$3
 
-if [ -z "$MAJOR" -o -z "$MINOR" ]; then
+if [ -z "$MAJOR" ] || [ -z "$MINOR" ]; then
     echo "usage: $0 major minor UUID"
     exit 1;
 fi
@@ -53,13 +53,13 @@ fi
 # Set the name of the table. We're only interested in dmraid,
 # multipath, and kpartx tables; everything else is ignored.
 if [ "$dmtbl" = "part" ] ; then
-    dmname=$($DMSETUP info  -c --noheadings -o name -u $dmuuid)
+    dmname=$($DMSETUP info  -c --noheadings -o name -u "$dmuuid")
     echo "DM_MPATH=$dmname"
     # We need the dependencies of the parent table to figure out
     # the type if the parent is a multipath table
     case "$dmuuid" in
 	mpath-*)
-	    dmdeps=$($DMSETUP deps -u $dmuuid)
+	    dmdeps=$($DMSETUP deps -u "$dmuuid")
 	    dmserial=${dmuuid#mpath-}
 	    ;;
     esac
@@ -67,7 +67,7 @@ elif [ "$dmtbl" = "mpath" ] ; then
     dmname="$dmuuid"
     dmserial="$dmuuid"
     # We need the dependencies of the table to figure out the type
-    dmdeps=$($DMSETUP deps -u $UUID)
+    dmdeps=$($DMSETUP deps -u "$UUID")
 fi
 
 [ -n "$dmpart" ] && echo "DM_PART=$dmpart"
-- 
2.49.0

[PATCH v2 2/8] kpartx: fix file descriptor leak

[Martin Wilck]

Found by Fedora's static analysis [1].

[1] https://openscanhub.fedoraproject.org/task/51915/log/device-mapper-multipath-0.11.1-1.fc43/scan-results.html#def43

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 kpartx/kpartx.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 46cb76b..a1495e5 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -404,13 +404,6 @@ main(int argc, char **argv){
 		set_delimiter(mapname, delim);
 	}
 
-	fd = open(device, O_RDONLY | O_DIRECT);
-
-	if (fd == -1) {
-		perror(device);
-		exit(1);
-	}
-
 	/* add/remove partitions to the kernel devmapper tables */
 	int r = 0;
 
@@ -429,6 +422,13 @@ main(int argc, char **argv){
 		goto end;
 	}
 
+	fd = open(device, O_RDONLY | O_DIRECT);
+
+	if (fd == -1) {
+		perror(device);
+		exit(1);
+	}
+
 	for (i = 0; i < ptct; i++) {
 		ptp = &pts[i];
 
-- 
2.49.0

[PATCH v2 3/8] libmpathpersist: fix memory leak in mpath_prout_rel()

[Martin Wilck]

Found by Fedora's static analysis [1].

[1] https://openscanhub.fedoraproject.org/task/51915/log/device-mapper-multipath-0.11.1-1.fc43/scan-results.html#def44

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 0576429..0c5cd60 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -450,10 +450,10 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 	int count = 0;
 	int status = MPATH_PR_SUCCESS;
 	struct prin_resp resp;
-	struct prout_param_descriptor *pamp;
+	struct prout_param_descriptor *pamp = NULL;
 	struct prin_resp *pr_buff;
 	int length;
-	struct transportid *pptr;
+	struct transportid *pptr = NULL;
 
 	if (!mpp)
 		return MPATH_PR_DMMP_ERROR;
@@ -560,7 +560,7 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 	pamp = (struct prout_param_descriptor *)malloc (length);
 	if (!pamp){
 		condlog (0, "%s: failed to alloc pr out parameter.", mpp->wwid);
-		goto out1;
+		goto out;
 	}
 
 	memset(pamp, 0, length);
@@ -570,6 +570,7 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 		condlog (0, "%s: failed to alloc pr out transportid.", mpp->wwid);
 		goto out1;
 	}
+	pptr = pamp->trnptid_list[0];
 
 	if (get_be64(mpp->reservation_key)){
 		memcpy (pamp->key, &mpp->reservation_key, 8);
@@ -581,11 +582,10 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 
 	if (status) {
 		condlog(0, "%s: failed to send CLEAR_SA", mpp->wwid);
-		goto out1;
+		goto out2;
 	}
 
 	pamp->num_transportid = 1;
-	pptr=pamp->trnptid_list[0];
 
 	for (i = 0; i < num; i++){
 		if (get_be64(mpp->reservation_key) &&
@@ -629,7 +629,7 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 		status = mpath_prout_reg(mpp, MPATH_PROUT_REG_SA, rq_scope, rq_type, pamp, noisy);
 	}
 
-
+out2:
 	free(pptr);
 out1:
 	free (pamp);
-- 
2.49.0

[PATCH v2 4/8] libmpathutil: vector_del_slot: modify v->allocated if realloc fails

[Martin Wilck]

We use v->allocated with the semantics of "length" or "number of used
elements" of a vector. If realloc() fails while deleting an element,
we should keep the previously allocated v->slot array but still
decrement "allocated" in order to avoid accessing invalid vector
elements.

The next successful realloc will bring v->allocated and the actual
size of the array in sync again.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathutil/vector.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/libmpathutil/vector.c b/libmpathutil/vector.c
index 7f763cb..f69d644 100644
--- a/libmpathutil/vector.c
+++ b/libmpathutil/vector.c
@@ -113,7 +113,7 @@ vector_del_slot(vector v, int slot)
 		return;
 
 	for (i = slot + 1; i < VECTOR_SIZE(v); i++)
-		v->slot[i-1] = v->slot[i];
+		v->slot[i - 1] = v->slot[i];
 
 	v->allocated -= VECTOR_DEFAULT_SIZE;
 
@@ -125,9 +125,15 @@ vector_del_slot(vector v, int slot)
 		void *new_slot;
 
 		new_slot = realloc(v->slot, sizeof (void *) * v->allocated);
-		if (!new_slot)
-			v->allocated += VECTOR_DEFAULT_SIZE;
-		else
+		/*
+		 * If realloc() fails, v->allocated will be smaller than the
+		 * actual allocated size vector size.
+		 * This is intentional; we want VECTOR_SIZE() to return
+		 * the number of used elements. Otherwise, vector_for_each_slot()
+		 * et al. wouldn't work as intended; there might be duplicate
+		 * or stale elements at the end of the vector.
+		 */
+		if (new_slot)
 			v->slot = new_slot;
 	}
 }
-- 
2.49.0

[PATCH v2 5/8] libmultipath: fix undefined behavior in 31-bit shift

[Martin Wilck]

Error: CPPCHECK_WARNING (CWE-758): [#def61]
multipath-tools-0.11.1/libmultipath/nvme-ioctl.c:572:
error[shiftTooManyBitsSigned]: Shifting signed 32-bit value by 31 bits
is undefined behaviour

Found by Fedora's static analysis [1].

[1] https://openscanhub.fedoraproject.org/task/51915/log/device-mapper-multipath-0.11.1-1.fc43/scan-results.html#def61

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/nvme/nvme-ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/nvme/nvme-ioctl.c b/libmultipath/nvme/nvme-ioctl.c
index 6959976..445ae2b 100644
--- a/libmultipath/nvme/nvme-ioctl.c
+++ b/libmultipath/nvme/nvme-ioctl.c
@@ -569,7 +569,7 @@ int nvme_feature(int fd, __u8 opcode, __u32 nsid, __u32 cdw10, __u32 cdw11,
 int nvme_set_feature(int fd, __u32 nsid, __u8 fid, __u32 value, __u32 cdw12,
 		     bool save, __u32 data_len, void *data, __u32 *result)
 {
-	__u32 cdw10 = fid | (save ? 1 << 31 : 0);
+	__u32 cdw10 = fid | (save ? 1U << 31 : 0);
 
 	return nvme_feature(fd, nvme_admin_set_features, nsid, cdw10, value,
 			    cdw12, data_len, data, result);
-- 
2.49.0

[PATCH v2 6/8] libmultipath: prioritizers/iet: fix possible NULL dereference

[Martin Wilck]

Found by Fedora's static analysis [1].

[1] https://openscanhub.fedoraproject.org/task/51915/log/device-mapper-multipath-0.11.1-1.fc43/scan-results.html#def68

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/prioritizers/iet.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libmultipath/prioritizers/iet.c b/libmultipath/prioritizers/iet.c
index f3bf64c..f5fdd42 100644
--- a/libmultipath/prioritizers/iet.c
+++ b/libmultipath/prioritizers/iet.c
@@ -101,6 +101,8 @@ int iet_prio(const char *dev, char * args)
 	char buffer[BUFFERSIZE];
 	char fullpath[BUFFERSIZE] = "/dev/disk/by-path/";
 	dir_p = opendir(fullpath);
+	if (!dir_p)
+		goto out;
 
 	// loop to find device in /dev/disk/by-path
 	while( NULL != (dir_entry_p = readdir(dir_p))) {
@@ -135,6 +137,7 @@ int iet_prio(const char *dev, char * args)
 	}
 	// nothing found, low prio
 	closedir(dir_p);
+out:
 	return 10;
 }
 
-- 
2.49.0

[PATCH v2 7/8] libmpathutil: remove vector_repack()

[Martin Wilck]

This function is unused, and its implementation looks odd.
Remove it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathutil/vector.c | 13 -------------
 libmpathutil/vector.h |  1 -
 2 files changed, 14 deletions(-)

diff --git a/libmpathutil/vector.c b/libmpathutil/vector.c
index f69d644..6ad5dd5 100644
--- a/libmpathutil/vector.c
+++ b/libmpathutil/vector.c
@@ -138,19 +138,6 @@ vector_del_slot(vector v, int slot)
 	}
 }
 
-void
-vector_repack(vector v)
-{
-	int i;
-
-	if (!v || !v->allocated)
-		return;
-
-	for (i = 0; i < VECTOR_SIZE(v); i++)
-		if (i > 0 && v->slot[i] == NULL)
-			vector_del_slot(v, i--);
-}
-
 vector
 vector_reset(vector v)
 {
diff --git a/libmpathutil/vector.h b/libmpathutil/vector.h
index 85127ad..171ab6a 100644
--- a/libmpathutil/vector.h
+++ b/libmpathutil/vector.h
@@ -86,7 +86,6 @@ extern void vector_del_slot(vector v, int slot);
 extern void *vector_insert_slot(vector v, int slot, void *value);
 int find_slot(vector v, const void *addr);
 int vector_find_or_add_slot(vector v, void *value);
-extern void vector_repack(vector v);
 extern void vector_dump(vector v);
 extern void dump_strvec(vector strvec);
 extern int vector_move_up(vector v, int src, int dest);
-- 
2.49.0

[PATCH v2 8/8] libmpathutil: remove VECTOR_DEFAULT_SIZE macro

[Martin Wilck]

We always add or remove a single element in vector_add_slot() and
vector_remove_slot(). Use of the VECTOR_DEFAULT_SIZE macro suggests that
its value can be changed, but this is not the case.

Remove the macro.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathutil/vector.c | 4 ++--
 libmpathutil/vector.h | 3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/libmpathutil/vector.c b/libmpathutil/vector.c
index 6ad5dd5..34a8512 100644
--- a/libmpathutil/vector.c
+++ b/libmpathutil/vector.c
@@ -45,7 +45,7 @@ vector_alloc_slot(vector v)
 	if (!v)
 		return false;
 
-	new_allocated = v->allocated + VECTOR_DEFAULT_SIZE;
+	new_allocated = v->allocated + 1;
 	new_slot = realloc(v->slot, sizeof (void *) * new_allocated);
 	if (!new_slot)
 		return false;
@@ -115,7 +115,7 @@ vector_del_slot(vector v, int slot)
 	for (i = slot + 1; i < VECTOR_SIZE(v); i++)
 		v->slot[i - 1] = v->slot[i];
 
-	v->allocated -= VECTOR_DEFAULT_SIZE;
+	v->allocated--;
 
 	if (v->allocated <= 0) {
 		free(v->slot);
diff --git a/libmpathutil/vector.h b/libmpathutil/vector.h
index 171ab6a..0a5b32e 100644
--- a/libmpathutil/vector.h
+++ b/libmpathutil/vector.h
@@ -32,8 +32,7 @@ struct vector_s {
 };
 typedef struct vector_s *vector;
 
-#define VECTOR_DEFAULT_SIZE 1
-#define VECTOR_SIZE(V)   ((V) ? ((V)->allocated) / VECTOR_DEFAULT_SIZE : 0)
+#define VECTOR_SIZE(V)   ((V) ? (V)->allocated : 0)
 #define VECTOR_SLOT(V,E) (((V) && (E) < VECTOR_SIZE(V) && (E) >= 0) ? (V)->slot[(E)] : NULL)
 #define VECTOR_LAST_SLOT(V)   (((V) && VECTOR_SIZE(V) > 0) ? (V)->slot[(VECTOR_SIZE(V) - 1)] : NULL)
 
-- 
2.49.0

Re: [PATCH v2 4/8] libmpathutil: vector_del_slot: modify v->allocated if realloc fails

[Benjamin Marzinski]

On Wed, May 07, 2025 at 09:05:48PM +0200, Martin Wilck wrote:
> We use v->allocated with the semantics of "length" or "number of used
> elements" of a vector. If realloc() fails while deleting an element,
> we should keep the previously allocated v->slot array but still
> decrement "allocated" in order to avoid accessing invalid vector
> elements.
> 
> The next successful realloc will bring v->allocated and the actual
> size of the array in sync again.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> ---
>  libmpathutil/vector.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/libmpathutil/vector.c b/libmpathutil/vector.c
> index 7f763cb..f69d644 100644
> --- a/libmpathutil/vector.c
> +++ b/libmpathutil/vector.c
> @@ -113,7 +113,7 @@ vector_del_slot(vector v, int slot)
>  		return;
>  
>  	for (i = slot + 1; i < VECTOR_SIZE(v); i++)
> -		v->slot[i-1] = v->slot[i];
> +		v->slot[i - 1] = v->slot[i];
>  
>  	v->allocated -= VECTOR_DEFAULT_SIZE;
>  
> @@ -125,9 +125,15 @@ vector_del_slot(vector v, int slot)
>  		void *new_slot;
>  
>  		new_slot = realloc(v->slot, sizeof (void *) * v->allocated);
> -		if (!new_slot)
> -			v->allocated += VECTOR_DEFAULT_SIZE;
> -		else
> +		/*
> +		 * If realloc() fails, v->allocated will be smaller than the
> +		 * actual allocated size vector size.
> +		 * This is intentional; we want VECTOR_SIZE() to return
> +		 * the number of used elements. Otherwise, vector_for_each_slot()
> +		 * et al. wouldn't work as intended; there might be duplicate
> +		 * or stale elements at the end of the vector.
> +		 */
> +		if (new_slot)
>  			v->slot = new_slot;
>  	}
>  }
> -- 
> 2.49.0

Re: [PATCH v2 7/8] libmpathutil: remove vector_repack()

[Benjamin Marzinski]

On Wed, May 07, 2025 at 09:05:51PM +0200, Martin Wilck wrote:
> This function is unused, and its implementation looks odd.
> Remove it.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> ---
>  libmpathutil/vector.c | 13 -------------
>  libmpathutil/vector.h |  1 -
>  2 files changed, 14 deletions(-)
> 
> diff --git a/libmpathutil/vector.c b/libmpathutil/vector.c
> index f69d644..6ad5dd5 100644
> --- a/libmpathutil/vector.c
> +++ b/libmpathutil/vector.c
> @@ -138,19 +138,6 @@ vector_del_slot(vector v, int slot)
>  	}
>  }
>  
> -void
> -vector_repack(vector v)
> -{
> -	int i;
> -
> -	if (!v || !v->allocated)
> -		return;
> -
> -	for (i = 0; i < VECTOR_SIZE(v); i++)
> -		if (i > 0 && v->slot[i] == NULL)
> -			vector_del_slot(v, i--);
> -}
> -
>  vector
>  vector_reset(vector v)
>  {
> diff --git a/libmpathutil/vector.h b/libmpathutil/vector.h
> index 85127ad..171ab6a 100644
> --- a/libmpathutil/vector.h
> +++ b/libmpathutil/vector.h
> @@ -86,7 +86,6 @@ extern void vector_del_slot(vector v, int slot);
>  extern void *vector_insert_slot(vector v, int slot, void *value);
>  int find_slot(vector v, const void *addr);
>  int vector_find_or_add_slot(vector v, void *value);
> -extern void vector_repack(vector v);
>  extern void vector_dump(vector v);
>  extern void dump_strvec(vector strvec);
>  extern int vector_move_up(vector v, int src, int dest);
> -- 
> 2.49.0

Re: [PATCH v2 8/8] libmpathutil: remove VECTOR_DEFAULT_SIZE macro

[Benjamin Marzinski]

On Wed, May 07, 2025 at 09:05:52PM +0200, Martin Wilck wrote:
> We always add or remove a single element in vector_add_slot() and
> vector_remove_slot(). Use of the VECTOR_DEFAULT_SIZE macro suggests that
> its value can be changed, but this is not the case.
> 
> Remove the macro.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> ---
>  libmpathutil/vector.c | 4 ++--
>  libmpathutil/vector.h | 3 +--
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/libmpathutil/vector.c b/libmpathutil/vector.c
> index 6ad5dd5..34a8512 100644
> --- a/libmpathutil/vector.c
> +++ b/libmpathutil/vector.c
> @@ -45,7 +45,7 @@ vector_alloc_slot(vector v)
>  	if (!v)
>  		return false;
>  
> -	new_allocated = v->allocated + VECTOR_DEFAULT_SIZE;
> +	new_allocated = v->allocated + 1;
>  	new_slot = realloc(v->slot, sizeof (void *) * new_allocated);
>  	if (!new_slot)
>  		return false;
> @@ -115,7 +115,7 @@ vector_del_slot(vector v, int slot)
>  	for (i = slot + 1; i < VECTOR_SIZE(v); i++)
>  		v->slot[i - 1] = v->slot[i];
>  
> -	v->allocated -= VECTOR_DEFAULT_SIZE;
> +	v->allocated--;
>  
>  	if (v->allocated <= 0) {
>  		free(v->slot);
> diff --git a/libmpathutil/vector.h b/libmpathutil/vector.h
> index 171ab6a..0a5b32e 100644
> --- a/libmpathutil/vector.h
> +++ b/libmpathutil/vector.h
> @@ -32,8 +32,7 @@ struct vector_s {
>  };
>  typedef struct vector_s *vector;
>  
> -#define VECTOR_DEFAULT_SIZE 1
> -#define VECTOR_SIZE(V)   ((V) ? ((V)->allocated) / VECTOR_DEFAULT_SIZE : 0)
> +#define VECTOR_SIZE(V)   ((V) ? (V)->allocated : 0)
>  #define VECTOR_SLOT(V,E) (((V) && (E) < VECTOR_SIZE(V) && (E) >= 0) ? (V)->slot[(E)] : NULL)
>  #define VECTOR_LAST_SLOT(V)   (((V) && VECTOR_SIZE(V) > 0) ? (V)->slot[(VECTOR_SIZE(V) - 1)] : NULL)
>  
> -- 
> 2.49.0