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

[PATCH 0/6] 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. Most of the warnings
appear to be false positives though, see below (a second look by
another eye pair would be appreciated).

Regards
Martin

On Sat, 2025-05-03 at 23:29 +0200, Xose Vazquez Perez wrote:
> Findings by static analyzers in Fedora 43 Critical Path Packages:
> https://marc.info/?l=fedora-devel-list&m=174582743823723
> 
> device-mapper-multipath-0.11.1-1.fc43:
> https://openscanhub.fedoraproject.org/task/51915/
> 
> device-mapper-multipath-0.11.1-1.fc43 List of Findings:
> https://openscanhub.fedoraproject.org/task/51915/log/device-mapper-multipath-0.11.1-1.fc43/scan-results.html

I'll leave the shellcheck errors in mpathconf to Ben.

> Error: GCC_ANALYZER_WARNING (CWE-775): [#def42]
> multipath-tools-0.11.1/kpartx/dasd.c:89:24: warning[-Wanalyzer-fd-leak]: leak of file descriptor ‘fd_dasd’
> multipath-tools-0.11.1/kpartx/dasd.c:69:1: enter_function: entry to ‘read_dasd_pt’
> ...
> multipath-tools-0.11.1/kpartx/dasd.c:134:20: branch_true: following ‘true’ branch (when ‘fd_dasd < 0’)...
> branch_true: ...to here

This one looks like a false positive to me. If fd_dasd < 0, it can't be leaked.

> Error: GCC_ANALYZER_WARNING (CWE-476): [#def45]
> multipath-tools-0.11.1/libmpathutil/parser.c:139:29: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘keyword’

False positive, too. VECTOR_SLOT won't return NULL here. 
We could add a test to make the compiler happy, but I don't quite see the point.

> Error: GCC_ANALYZER_WARNING (CWE-457): [#def49]
> multipath-tools-0.11.1/libmultipath/alias.c:201:31: warning[-Wanalyzer-use-of-uninitialized-value]: use of uninitialized value ‘bdg’

False positive. bdg won't be NULL here.

> Error: GCC_ANALYZER_WARNING (CWE-465): [#def50]
> multipath-tools-0.11.1/libmultipath/dmparser.c:31:12: warning[-Wanalyzer-deref-before-check]: check of ‘p’ for NULL after already dereferencing it

False positive. *dst is non-NULL when we call merge_words(). We must double-check
after calling realloc().
(That said, we should reimplement this using strbuf functions).

> Error: GCC_ANALYZER_WARNING (CWE-476): [#def51]
> multipath-tools-0.11.1/libmultipath/dmparser.c:436:25: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘pgp’

False positive. VECTOR_SLOT won't return NULL here. Same for the next 3 errors.

> Error: GCC_ANALYZER_WARNING (CWE-476): [#def55]
> multipath-tools-0.11.1/libmultipath/dmparser.c:501:33: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘pp’

Likewise. Same for the next 2 errors.

> Error: GCC_ANALYZER_WARNING (CWE-476): [#def58]
> multipath-tools-0.11.1/libmultipath/foreign/nvme.c:494:28: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘path’

The nvme_pathgroup is a member of struct nvme_path. nvme_pg_to_path() doesn't return NULL. Same for the next 2 errors in nvme.c

> Error: GCC_ANALYZER_WARNING (CWE-476): [#def62]
> multipath-tools-0.11.1/libmultipath/pgpolicies.c:191:17: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘pp1’

Again, false positives; VECTOR_SLOT() doesn't return NULL here.
Same for the next 3 errors.

> Error: GCC_ANALYZER_WARNING (CWE-401): [#def66]
> multipath-tools-0.11.1/libmultipath/print.c:920:16: warning[-Wanalyzer-malloc-leak]: leak of ‘alloc_path_layout()’

False positive. Strange one - gcc doesn't understand its own __attribute__((cleanup))?
Or am I blind? Same for the next one.

> Error: GCC_ANALYZER_WARNING (CWE-126): [#def70]
multipath-tools-0.11.1/libmultipath/uevent.c:122:9: warning[-Wanalyzer-out-of-bounds]: stack-based buffer over-read

This one is somewhat harder because it involves list handling macros, but I believe that our code is correct and that this is a false positive, too.

> Error: GCC_ANALYZER_WARNING (CWE-775): [#def69]
> multipath-tools-0.11.1/libmultipath/sysfs.c:309:22: warning[-Wanalyzer-fd-leak]: leak of file descriptor ‘open(&pathbuf, 0)’

Again, false positive; there's a cleanup function for fd.

> Error: GCC_ANALYZER_WARNING (CWE-775): [#def71]
> multipath-tools-0.11.1/libmultipath/valid.c:208:34: warning[-Wanalyzer-file-leak]: leak of FILE ‘fopen(&mountinfo, "r")’

False positive, gcc doesn't understand the cleanup_fclose().
Same for the next error.

> Error: GCC_ANALYZER_WARNING (CWE-415): [#def73]
> multipath-tools-0.11.1/multipathd/fpin_handlers.c:508:17: warning[-Wanalyzer-double-free]: double-‘free’ of ‘els_marginal_list_head.next + -2056’

I don't see an error in this code. Looks like another false positive to me.

> Error: GCC_ANALYZER_WARNING: [#def74]
> multipath-tools-0.11.1/multipathd/fpin_handlers.c:624:23: warning[-Wanalyzer-fd-use-without-check]: ‘read’ on possibly invalid file descriptor ‘fd’

False positive, too. fd == -1 is checked beforehand.

> Error: GCC_ANALYZER_WARNING (CWE-688): [#def75]
> multipath-tools-0.11.1/multipathd/main.c:3286:21: warning[-Wanalyzer-null-argument]: use of NULL ‘new’ where non-null expected

Similar to other false positives above, VECTOR_SLOT() won't return NULL in a  loop like this.

> Error: GCC_ANALYZER_WARNING (CWE-775): [#def77]
> multipath-tools-0.11.1/multipathd/main.c:3973:12: warning[-Wanalyzer-fd-leak]: leak of file descriptor ‘dup2(open("/dev/null", 2), 0)’

Yet another false positive, the fd is closed in the cleanup handler. Same for the next 3 errors.

> Error: GCC_ANALYZER_WARNING (CWE-476): [#def81]
multipath-tools-0.11.1/multipathd/multipathc.c:97:30: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘kw’

Another case of VECTOR_SLOT() false positive. Same for the next error.

Regards
Martin

Martin Wilck (6):
  kpartx_id: fix shellcheck reported errors
  kpartx: fix file descriptor leak
  libmpathpersist: fix memory leak in mpath_prout_rel()
  libmpathutil: silence compiler warning in vector_del_slot()
  libmultipath: fix undefined behavior in 31-bit shift
  libmultipath: prioritizers/iet: fix possible NULL dereference

 kpartx/kpartx.c                     | 14 +++++++-------
 kpartx/kpartx_id                    |  8 ++++----
 libmpathpersist/mpath_persist_int.c | 12 ++++++------
 libmpathutil/vector.c               | 16 ++++++++--------
 libmultipath/nvme/nvme-ioctl.c      |  2 +-
 libmultipath/prioritizers/iet.c     |  3 +++
 6 files changed, 29 insertions(+), 26 deletions(-)

-- 
2.49.0

[PATCH 1/6] 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

Signed-off-by: Martin Wilck <mwilck@suse.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 2/6] 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

Signed-off-by: Martin Wilck <mwilck@suse.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 3/6] 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

Signed-off-by: Martin Wilck <mwilck@suse.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 4/6] libmpathutil: silence compiler warning in vector_del_slot()

[Martin Wilck]

Try to silence a gcc warning. Also, replace the wrong-looking
VECTOR_DEFAULT_SIZE by 1 (after all, we've just deleted a single
element).

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

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

diff --git a/libmpathutil/vector.c b/libmpathutil/vector.c
index 7f763cb..3386651 100644
--- a/libmpathutil/vector.c
+++ b/libmpathutil/vector.c
@@ -107,28 +107,28 @@ int find_slot(vector v, const void *addr)
 void
 vector_del_slot(vector v, int slot)
 {
-	int i;
+	int i, allocated;
 
 	if (!v || !v->allocated || slot < 0 || slot >= VECTOR_SIZE(v))
 		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;
+	allocated = v->allocated - 1;
 
-	if (v->allocated <= 0) {
+	if (allocated <= 0) {
 		free(v->slot);
 		v->slot = NULL;
 		v->allocated = 0;
 	} else {
 		void *new_slot;
 
-		new_slot = realloc(v->slot, sizeof (void *) * v->allocated);
-		if (!new_slot)
-			v->allocated += VECTOR_DEFAULT_SIZE;
-		else
+		new_slot = realloc(v->slot, sizeof(void *) * allocated);
+		if (new_slot) {
 			v->slot = new_slot;
+			v->allocated = allocated;
+		}
 	}
 }
 
-- 
2.49.0

[PATCH 5/6] 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

Signed-off-by: Martin Wilck <mwilck@suse.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 6/6] 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

Signed-off-by: Martin Wilck <mwilck@suse.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

Re: [PATCH 4/6] libmpathutil: silence compiler warning in vector_del_slot()

[Benjamin Marzinski]

On Mon, May 05, 2025 at 06:29:56PM +0200, Martin Wilck wrote:
> Try to silence a gcc warning. Also, replace the wrong-looking
> VECTOR_DEFAULT_SIZE by 1 (after all, we've just deleted a single
> element).

I agree that VECTOR_DEFAULT_SIZE is pointless. But we shoud also stop
adding VECTOR_DEFAULT_SIZE to v->allocated in vector_alloc_slot() and
dividing v->allocated by VECTOR_DEFAULT_SIZE in VECTOR_SIZE(), otherwise
vector_del_slot() doesn't match vector_alloc_slot() and VECTOR_SIZE().
My vote is for VECTOR_DEFAULT_SIZE to go away completely.

-Ben

> 
> 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
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmpathutil/vector.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/libmpathutil/vector.c b/libmpathutil/vector.c
> index 7f763cb..3386651 100644
> --- a/libmpathutil/vector.c
> +++ b/libmpathutil/vector.c
> @@ -107,28 +107,28 @@ int find_slot(vector v, const void *addr)
>  void
>  vector_del_slot(vector v, int slot)
>  {
> -	int i;
> +	int i, allocated;
>  
>  	if (!v || !v->allocated || slot < 0 || slot >= VECTOR_SIZE(v))
>  		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;
> +	allocated = v->allocated - 1;
>  
> -	if (v->allocated <= 0) {
> +	if (allocated <= 0) {
>  		free(v->slot);
>  		v->slot = NULL;
>  		v->allocated = 0;
>  	} else {
>  		void *new_slot;
>  
> -		new_slot = realloc(v->slot, sizeof (void *) * v->allocated);
> -		if (!new_slot)
> -			v->allocated += VECTOR_DEFAULT_SIZE;
> -		else
> +		new_slot = realloc(v->slot, sizeof(void *) * allocated);
> +		if (new_slot) {
>  			v->slot = new_slot;
> +			v->allocated = allocated;
> +		}
>  	}
>  }
>  
> -- 
> 2.49.0

Re: [PATCH 4/6] libmpathutil: silence compiler warning in vector_del_slot()

[Benjamin Marzinski]

On Mon, May 05, 2025 at 06:29:56PM +0200, Martin Wilck wrote:
> Try to silence a gcc warning. Also, replace the wrong-looking
> VECTOR_DEFAULT_SIZE by 1 (after all, we've just deleted a single
> element).
> 
> 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
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmpathutil/vector.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/libmpathutil/vector.c b/libmpathutil/vector.c
> index 7f763cb..3386651 100644
> --- a/libmpathutil/vector.c
> +++ b/libmpathutil/vector.c
> @@ -107,28 +107,28 @@ int find_slot(vector v, const void *addr)
>  void
>  vector_del_slot(vector v, int slot)
>  {
> -	int i;
> +	int i, allocated;
>  
>  	if (!v || !v->allocated || slot < 0 || slot >= VECTOR_SIZE(v))
>  		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;
> +	allocated = v->allocated - 1;
>  
> -	if (v->allocated <= 0) {
> +	if (allocated <= 0) {
>  		free(v->slot);
>  		v->slot = NULL;
>  		v->allocated = 0;
>  	} else {
>  		void *new_slot;
>  
> -		new_slot = realloc(v->slot, sizeof (void *) * v->allocated);
> -		if (!new_slot)
> -			v->allocated += VECTOR_DEFAULT_SIZE;
> -		else
> +		new_slot = realloc(v->slot, sizeof(void *) * allocated);
> +		if (new_slot) {
>  			v->slot = new_slot;
> +			v->allocated = allocated;
> +		}

Actually, now that I think about this a bit more, I don't think that
this was ever the right thing to do. If the realloc() fails, I see no
harm in keeping the smaller v->allocated value. Future realloc()s and
free()s will still work correctly. On the other hand, if we restore the
old, larger v->allocated value (or with your code, never reduce it in
the first place) there will be a duplicate value on the list, which I
can definitely see causing problems.

-Ben

>  	}
>  }
>  
> -- 
> 2.49.0

Re: [PATCH 0/6] multipath-tools: static analyzer fixes

[Benjamin Marzinski]

On Mon, May 05, 2025 at 06:29:52PM +0200, Martin Wilck wrote:
> Xose has kindly informed me about the warnings from Fedora's static
> code analysis tool.
> 
> I'm sending a couple of related fixes here. Most of the warnings
> appear to be false positives though, see below (a second look by
> another eye pair would be appreciated).

I mostly agree with you, except that I wouldn't call all the errors
related to VECTOR_SLOT() returning NULL false postitives (so maybe I
mostly disagree with you. I didn't count). If we are in a
vector_foreach_slot_*() loop and we are looking at
VECTOR_SLOT(<vector_we_are_looping_over>, <current_index_value>), then I
agree, we can't see a NULL value. But most, if not all, of the "NULL
value from a vector" errors that got flagged here were not protected by
the checks in vector_foreach_slot_*(). Now I don't know of any cases
where we do have NULLs in a vector, but we certainly could. We
manipulate them all over the code. If we ever call vector_alloc_slot()
and not vector_set_slot() we would have a NULL.  There's also no check
to make sure that we don't call vector_set_slot() with a NULL value.

We do have checks for NULL values when looping through vectors, and we
even have vector_repack() to get rid of them. So, it's completely
reasonable that a static analyzer would think that we might have a NULL
value in a vector. And I'm pretty sure that NULL values in a vector are
always a bug. They make vector_foreach_slot() unable to access any
values past them, despite the fact that we can keep adding values to the
vector.

Actually, cleaning this up, so that the code keeps us from having NULL
values in vectors as long as we only manipulate them with the functions
in vector.h has been on my todo list for a bit. It would take some code
auditing to make sure that there isn't a case where we do use vectors
where NULL values are expected. But if there is, I think that should use
a seperate set of functions, to clearly mark it out as special.

-Ben

> 
> Regards
> Martin
> 
> On Sat, 2025-05-03 at 23:29 +0200, Xose Vazquez Perez wrote:
> > Findings by static analyzers in Fedora 43 Critical Path Packages:
> > https://marc.info/?l=fedora-devel-list&m=174582743823723
> > 
> > device-mapper-multipath-0.11.1-1.fc43:
> > https://openscanhub.fedoraproject.org/task/51915/
> > 
> > device-mapper-multipath-0.11.1-1.fc43 List of Findings:
> > https://openscanhub.fedoraproject.org/task/51915/log/device-mapper-multipath-0.11.1-1.fc43/scan-results.html
> 
> I'll leave the shellcheck errors in mpathconf to Ben.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-775): [#def42]
> > multipath-tools-0.11.1/kpartx/dasd.c:89:24: warning[-Wanalyzer-fd-leak]: leak of file descriptor ‘fd_dasd’
> > multipath-tools-0.11.1/kpartx/dasd.c:69:1: enter_function: entry to ‘read_dasd_pt’
> > ...
> > multipath-tools-0.11.1/kpartx/dasd.c:134:20: branch_true: following ‘true’ branch (when ‘fd_dasd < 0’)...
> > branch_true: ...to here
> 
> This one looks like a false positive to me. If fd_dasd < 0, it can't be leaked.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-476): [#def45]
> > multipath-tools-0.11.1/libmpathutil/parser.c:139:29: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘keyword’
> 
> False positive, too. VECTOR_SLOT won't return NULL here. 
> We could add a test to make the compiler happy, but I don't quite see the point.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-457): [#def49]
> > multipath-tools-0.11.1/libmultipath/alias.c:201:31: warning[-Wanalyzer-use-of-uninitialized-value]: use of uninitialized value ‘bdg’
> 
> False positive. bdg won't be NULL here.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-465): [#def50]
> > multipath-tools-0.11.1/libmultipath/dmparser.c:31:12: warning[-Wanalyzer-deref-before-check]: check of ‘p’ for NULL after already dereferencing it
> 
> False positive. *dst is non-NULL when we call merge_words(). We must double-check
> after calling realloc().
> (That said, we should reimplement this using strbuf functions).
> 
> > Error: GCC_ANALYZER_WARNING (CWE-476): [#def51]
> > multipath-tools-0.11.1/libmultipath/dmparser.c:436:25: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘pgp’
> 
> False positive. VECTOR_SLOT won't return NULL here. Same for the next 3 errors.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-476): [#def55]
> > multipath-tools-0.11.1/libmultipath/dmparser.c:501:33: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘pp’
> 
> Likewise. Same for the next 2 errors.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-476): [#def58]
> > multipath-tools-0.11.1/libmultipath/foreign/nvme.c:494:28: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘path’
> 
> The nvme_pathgroup is a member of struct nvme_path. nvme_pg_to_path() doesn't return NULL. Same for the next 2 errors in nvme.c
> 
> > Error: GCC_ANALYZER_WARNING (CWE-476): [#def62]
> > multipath-tools-0.11.1/libmultipath/pgpolicies.c:191:17: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘pp1’
> 
> Again, false positives; VECTOR_SLOT() doesn't return NULL here.
> Same for the next 3 errors.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-401): [#def66]
> > multipath-tools-0.11.1/libmultipath/print.c:920:16: warning[-Wanalyzer-malloc-leak]: leak of ‘alloc_path_layout()’
> 
> False positive. Strange one - gcc doesn't understand its own __attribute__((cleanup))?
> Or am I blind? Same for the next one.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-126): [#def70]
> multipath-tools-0.11.1/libmultipath/uevent.c:122:9: warning[-Wanalyzer-out-of-bounds]: stack-based buffer over-read
> 
> This one is somewhat harder because it involves list handling macros, but I believe that our code is correct and that this is a false positive, too.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-775): [#def69]
> > multipath-tools-0.11.1/libmultipath/sysfs.c:309:22: warning[-Wanalyzer-fd-leak]: leak of file descriptor ‘open(&pathbuf, 0)’
> 
> Again, false positive; there's a cleanup function for fd.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-775): [#def71]
> > multipath-tools-0.11.1/libmultipath/valid.c:208:34: warning[-Wanalyzer-file-leak]: leak of FILE ‘fopen(&mountinfo, "r")’
> 
> False positive, gcc doesn't understand the cleanup_fclose().
> Same for the next error.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-415): [#def73]
> > multipath-tools-0.11.1/multipathd/fpin_handlers.c:508:17: warning[-Wanalyzer-double-free]: double-‘free’ of ‘els_marginal_list_head.next + -2056’
> 
> I don't see an error in this code. Looks like another false positive to me.
> 
> > Error: GCC_ANALYZER_WARNING: [#def74]
> > multipath-tools-0.11.1/multipathd/fpin_handlers.c:624:23: warning[-Wanalyzer-fd-use-without-check]: ‘read’ on possibly invalid file descriptor ‘fd’
> 
> False positive, too. fd == -1 is checked beforehand.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-688): [#def75]
> > multipath-tools-0.11.1/multipathd/main.c:3286:21: warning[-Wanalyzer-null-argument]: use of NULL ‘new’ where non-null expected
> 
> Similar to other false positives above, VECTOR_SLOT() won't return NULL in a  loop like this.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-775): [#def77]
> > multipath-tools-0.11.1/multipathd/main.c:3973:12: warning[-Wanalyzer-fd-leak]: leak of file descriptor ‘dup2(open("/dev/null", 2), 0)’
> 
> Yet another false positive, the fd is closed in the cleanup handler. Same for the next 3 errors.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-476): [#def81]
> multipath-tools-0.11.1/multipathd/multipathc.c:97:30: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘kw’
> 
> Another case of VECTOR_SLOT() false positive. Same for the next error.
> 
> Regards
> Martin
> 
> Martin Wilck (6):
>   kpartx_id: fix shellcheck reported errors
>   kpartx: fix file descriptor leak
>   libmpathpersist: fix memory leak in mpath_prout_rel()
>   libmpathutil: silence compiler warning in vector_del_slot()
>   libmultipath: fix undefined behavior in 31-bit shift
>   libmultipath: prioritizers/iet: fix possible NULL dereference
> 
>  kpartx/kpartx.c                     | 14 +++++++-------
>  kpartx/kpartx_id                    |  8 ++++----
>  libmpathpersist/mpath_persist_int.c | 12 ++++++------
>  libmpathutil/vector.c               | 16 ++++++++--------
>  libmultipath/nvme/nvme-ioctl.c      |  2 +-
>  libmultipath/prioritizers/iet.c     |  3 +++
>  6 files changed, 29 insertions(+), 26 deletions(-)
> 
> -- 
> 2.49.0

Re: [PATCH 0/6] multipath-tools: static analyzer fixes

[Martin Wilck]

On Tue, 2025-05-06 at 18:26 -0400, Benjamin Marzinski wrote:
> On Mon, May 05, 2025 at 06:29:52PM +0200, Martin Wilck wrote:
> > Xose has kindly informed me about the warnings from Fedora's static
> > code analysis tool.
> > 
> > I'm sending a couple of related fixes here. Most of the warnings
> > appear to be false positives though, see below (a second look by
> > another eye pair would be appreciated).
> 
> I mostly agree with you, except that I wouldn't call all the errors
> related to VECTOR_SLOT() returning NULL false postitives (so maybe I
> mostly disagree with you. I didn't count). If we are in a
> vector_foreach_slot_*() loop and we are looking at
> VECTOR_SLOT(<vector_we_are_looping_over>, <current_index_value>),
> then I
> agree, we can't see a NULL value. But most, if not all, of the "NULL
> value from a vector" errors that got flagged here were not protected
> by
> the checks in vector_foreach_slot_*(). 

No not all. I double checked quite a few. Maybe I got too sloppy at
some point and started dismissing these warnings prematurely.

> Now I don't know of any cases
> where we do have NULLs in a vector, but we certainly could. We
> manipulate them all over the code. If we ever call
> vector_alloc_slot()
> and not vector_set_slot() we would have a NULL.

I've been thinking for some time that we should have a single function
vector_alloc_and_set_slot().

>   There's also no check
> to make sure that we don't call vector_set_slot() with a NULL value.
> 
> We do have checks for NULL values when looping through vectors, and
> we
> even have vector_repack() to get rid of them. So, it's completely
> reasonable that a static analyzer would think that we might have a
> NULL
> value in a vector. And I'm pretty sure that NULL values in a vector
> are
> always a bug. 

Right. Then we should add assert() statements rather than NULL checks,
both when adding slots and when reading from vectors.


> They make vector_foreach_slot() unable to access any
> values past them, despite the fact that we can keep adding values to
> the
> vector.
> 
> Actually, cleaning this up, so that the code keeps us from having
> NULL
> values in vectors as long as we only manipulate them with the
> functions
> in vector.h has been on my todo list for a bit

Does this mean you intend to work on it and send patches?

Regards,
Martin


> . It would take some code
> auditing to make sure that there isn't a case where we do use vectors
> where NULL values are expected. But if there is, I think that should
> use
> a seperate set of functions, to clearly mark it out as special.




> 
> -Ben
> 
> > 
> > Regards
> > Martin
> > 
> > On Sat, 2025-05-03 at 23:29 +0200, Xose Vazquez Perez wrote:
> > > Findings by static analyzers in Fedora 43 Critical Path Packages:
> > > https://marc.info/?l=fedora-devel-list&m=174582743823723
> > > 
> > > device-mapper-multipath-0.11.1-1.fc43:
> > > https://openscanhub.fedoraproject.org/task/51915/
> > > 
> > > device-mapper-multipath-0.11.1-1.fc43 List of Findings:
> > > https://openscanhub.fedoraproject.org/task/51915/log/device-mapper-multipath-0.11.1-1.fc43/scan-results.html
> > 
> > I'll leave the shellcheck errors in mpathconf to Ben.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-775): [#def42]
> > > multipath-tools-0.11.1/kpartx/dasd.c:89:24: warning[-Wanalyzer-
> > > fd-leak]: leak of file descriptor ‘fd_dasd’
> > > multipath-tools-0.11.1/kpartx/dasd.c:69:1: enter_function: entry
> > > to ‘read_dasd_pt’
> > > ...
> > > multipath-tools-0.11.1/kpartx/dasd.c:134:20: branch_true:
> > > following ‘true’ branch (when ‘fd_dasd < 0’)...
> > > branch_true: ...to here
> > 
> > This one looks like a false positive to me. If fd_dasd < 0, it
> > can't be leaked.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-476): [#def45]
> > > multipath-tools-0.11.1/libmpathutil/parser.c:139:29: warning[-
> > > Wanalyzer-null-dereference]: dereference of NULL ‘keyword’
> > 
> > False positive, too. VECTOR_SLOT won't return NULL here. 
> > We could add a test to make the compiler happy, but I don't quite
> > see the point.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-457): [#def49]
> > > multipath-tools-0.11.1/libmultipath/alias.c:201:31: warning[-
> > > Wanalyzer-use-of-uninitialized-value]: use of uninitialized value
> > > ‘bdg’
> > 
> > False positive. bdg won't be NULL here.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-465): [#def50]
> > > multipath-tools-0.11.1/libmultipath/dmparser.c:31:12: warning[-
> > > Wanalyzer-deref-before-check]: check of ‘p’ for NULL after
> > > already dereferencing it
> > 
> > False positive. *dst is non-NULL when we call merge_words(). We
> > must double-check
> > after calling realloc().
> > (That said, we should reimplement this using strbuf functions).
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-476): [#def51]
> > > multipath-tools-0.11.1/libmultipath/dmparser.c:436:25: warning[-
> > > Wanalyzer-null-dereference]: dereference of NULL ‘pgp’
> > 
> > False positive. VECTOR_SLOT won't return NULL here. Same for the
> > next 3 errors.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-476): [#def55]
> > > multipath-tools-0.11.1/libmultipath/dmparser.c:501:33: warning[-
> > > Wanalyzer-null-dereference]: dereference of NULL ‘pp’
> > 
> > Likewise. Same for the next 2 errors.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-476): [#def58]
> > > multipath-tools-0.11.1/libmultipath/foreign/nvme.c:494:28:
> > > warning[-Wanalyzer-null-dereference]: dereference of NULL ‘path’
> > 
> > The nvme_pathgroup is a member of struct nvme_path.
> > nvme_pg_to_path() doesn't return NULL. Same for the next 2 errors
> > in nvme.c
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-476): [#def62]
> > > multipath-tools-0.11.1/libmultipath/pgpolicies.c:191:17:
> > > warning[-Wanalyzer-null-dereference]: dereference of NULL ‘pp1’
> > 
> > Again, false positives; VECTOR_SLOT() doesn't return NULL here.
> > Same for the next 3 errors.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-401): [#def66]
> > > multipath-tools-0.11.1/libmultipath/print.c:920:16: warning[-
> > > Wanalyzer-malloc-leak]: leak of ‘alloc_path_layout()’
> > 
> > False positive. Strange one - gcc doesn't understand its own
> > __attribute__((cleanup))?
> > Or am I blind? Same for the next one.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-126): [#def70]
> > multipath-tools-0.11.1/libmultipath/uevent.c:122:9: warning[-
> > Wanalyzer-out-of-bounds]: stack-based buffer over-read
> > 
> > This one is somewhat harder because it involves list handling
> > macros, but I believe that our code is correct and that this is a
> > false positive, too.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-775): [#def69]
> > > multipath-tools-0.11.1/libmultipath/sysfs.c:309:22: warning[-
> > > Wanalyzer-fd-leak]: leak of file descriptor ‘open(&pathbuf, 0)’
> > 
> > Again, false positive; there's a cleanup function for fd.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-775): [#def71]
> > > multipath-tools-0.11.1/libmultipath/valid.c:208:34: warning[-
> > > Wanalyzer-file-leak]: leak of FILE ‘fopen(&mountinfo, "r")’
> > 
> > False positive, gcc doesn't understand the cleanup_fclose().
> > Same for the next error.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-415): [#def73]
> > > multipath-tools-0.11.1/multipathd/fpin_handlers.c:508:17:
> > > warning[-Wanalyzer-double-free]: double-‘free’ of
> > > ‘els_marginal_list_head.next + -2056’
> > 
> > I don't see an error in this code. Looks like another false
> > positive to me.
> > 
> > > Error: GCC_ANALYZER_WARNING: [#def74]
> > > multipath-tools-0.11.1/multipathd/fpin_handlers.c:624:23:
> > > warning[-Wanalyzer-fd-use-without-check]: ‘read’ on possibly
> > > invalid file descriptor ‘fd’
> > 
> > False positive, too. fd == -1 is checked beforehand.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-688): [#def75]
> > > multipath-tools-0.11.1/multipathd/main.c:3286:21: warning[-
> > > Wanalyzer-null-argument]: use of NULL ‘new’ where non-null
> > > expected
> > 
> > Similar to other false positives above, VECTOR_SLOT() won't return
> > NULL in a  loop like this.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-775): [#def77]
> > > multipath-tools-0.11.1/multipathd/main.c:3973:12: warning[-
> > > Wanalyzer-fd-leak]: leak of file descriptor
> > > ‘dup2(open("/dev/null", 2), 0)’
> > 
> > Yet another false positive, the fd is closed in the cleanup
> > handler. Same for the next 3 errors.
> > 
> > > Error: GCC_ANALYZER_WARNING (CWE-476): [#def81]
> > multipath-tools-0.11.1/multipathd/multipathc.c:97:30: warning[-
> > Wanalyzer-null-dereference]: dereference of NULL ‘kw’
> > 
> > Another case of VECTOR_SLOT() false positive. Same for the next
> > error.
> > 
> > Regards
> > Martin
> > 
> > Martin Wilck (6):
> >   kpartx_id: fix shellcheck reported errors
> >   kpartx: fix file descriptor leak
> >   libmpathpersist: fix memory leak in mpath_prout_rel()
> >   libmpathutil: silence compiler warning in vector_del_slot()
> >   libmultipath: fix undefined behavior in 31-bit shift
> >   libmultipath: prioritizers/iet: fix possible NULL dereference
> > 
> >  kpartx/kpartx.c                     | 14 +++++++-------
> >  kpartx/kpartx_id                    |  8 ++++----
> >  libmpathpersist/mpath_persist_int.c | 12 ++++++------
> >  libmpathutil/vector.c               | 16 ++++++++--------
> >  libmultipath/nvme/nvme-ioctl.c      |  2 +-
> >  libmultipath/prioritizers/iet.c     |  3 +++
> >  6 files changed, 29 insertions(+), 26 deletions(-)
> > 
> > -- 
> > 2.49.0

Re: [PATCH 4/6] libmpathutil: silence compiler warning in vector_del_slot()

[Martin Wilck]

On Tue, 2025-05-06 at 18:01 -0400, Benjamin Marzinski wrote:
> On Mon, May 05, 2025 at 06:29:56PM +0200, Martin Wilck wrote:
> > Try to silence a gcc warning. Also, replace the wrong-looking
> > VECTOR_DEFAULT_SIZE by 1 (after all, we've just deleted a single
> > element).
> > 
> > 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
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmpathutil/vector.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/libmpathutil/vector.c b/libmpathutil/vector.c
> > index 7f763cb..3386651 100644
> > --- a/libmpathutil/vector.c
> > +++ b/libmpathutil/vector.c
> > @@ -107,28 +107,28 @@ int find_slot(vector v, const void *addr)
> >  void
> >  vector_del_slot(vector v, int slot)
> >  {
> > -	int i;
> > +	int i, allocated;
> >  
> >  	if (!v || !v->allocated || slot < 0 || slot >=
> > VECTOR_SIZE(v))
> >  		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;
> > +	allocated = v->allocated - 1;
> >  
> > -	if (v->allocated <= 0) {
> > +	if (allocated <= 0) {
> >  		free(v->slot);
> >  		v->slot = NULL;
> >  		v->allocated = 0;
> >  	} else {
> >  		void *new_slot;
> >  
> > -		new_slot = realloc(v->slot, sizeof (void *) * v-
> > >allocated);
> > -		if (!new_slot)
> > -			v->allocated += VECTOR_DEFAULT_SIZE;
> > -		else
> > +		new_slot = realloc(v->slot, sizeof(void *) *
> > allocated);
> > +		if (new_slot) {
> >  			v->slot = new_slot;
> > +			v->allocated = allocated;
> > +		}
> 
> Actually, now that I think about this a bit more, I don't think that
> this was ever the right thing to do. If the realloc() fails, I see no
> harm in keeping the smaller v->allocated value. Future realloc()s and
> free()s will still work correctly. On the other hand, if we restore
> the
> old, larger v->allocated value (or with your code, never reduce it in
> the first place) there will be a duplicate value on the list, which I
> can definitely see causing problems.

This is a fundamental problem of our vector implementation, IMO. We
don't distinguish between "allocated" and "used", or IOW, we use the
term "allocated" while we mean "used".

My thinking was that "allocated" should track the actual size of the
allocated memory. But you are right, it's better to have the C library
track the allocated size and keep in mind that v->allocated actually
has the semantics of "used".

I'll send a v2 with this change.

Regards
Martin

PS: I've wanted to improve the vector implementation for years.
IMO we should separate "allocated" and "used", and should cease
reallocating the vectors whenever an element is added or removed, which
is horribly inefficient. I'd actually started working on that a while
ago but didn't finish. Anyway, that's future material.

Re: [PATCH 0/6] multipath-tools: static analyzer fixes

[Benjamin Marzinski]

On Mon, May 05, 2025 at 06:29:52PM +0200, Martin Wilck wrote:
> Xose has kindly informed me about the warnings from Fedora's static
> code analysis tool.
> 
> I'm sending a couple of related fixes here. Most of the warnings
> appear to be false positives though, see below (a second look by
> another eye pair would be appreciated).

For all but patch 4/6:
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> 
> Regards
> Martin
> 
> On Sat, 2025-05-03 at 23:29 +0200, Xose Vazquez Perez wrote:
> > Findings by static analyzers in Fedora 43 Critical Path Packages:
> > https://marc.info/?l=fedora-devel-list&m=174582743823723
> > 
> > device-mapper-multipath-0.11.1-1.fc43:
> > https://openscanhub.fedoraproject.org/task/51915/
> > 
> > device-mapper-multipath-0.11.1-1.fc43 List of Findings:
> > https://openscanhub.fedoraproject.org/task/51915/log/device-mapper-multipath-0.11.1-1.fc43/scan-results.html
> 
> I'll leave the shellcheck errors in mpathconf to Ben.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-775): [#def42]
> > multipath-tools-0.11.1/kpartx/dasd.c:89:24: warning[-Wanalyzer-fd-leak]: leak of file descriptor ‘fd_dasd’
> > multipath-tools-0.11.1/kpartx/dasd.c:69:1: enter_function: entry to ‘read_dasd_pt’
> > ...
> > multipath-tools-0.11.1/kpartx/dasd.c:134:20: branch_true: following ‘true’ branch (when ‘fd_dasd < 0’)...
> > branch_true: ...to here
> 
> This one looks like a false positive to me. If fd_dasd < 0, it can't be leaked.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-476): [#def45]
> > multipath-tools-0.11.1/libmpathutil/parser.c:139:29: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘keyword’
> 
> False positive, too. VECTOR_SLOT won't return NULL here. 
> We could add a test to make the compiler happy, but I don't quite see the point.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-457): [#def49]
> > multipath-tools-0.11.1/libmultipath/alias.c:201:31: warning[-Wanalyzer-use-of-uninitialized-value]: use of uninitialized value ‘bdg’
> 
> False positive. bdg won't be NULL here.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-465): [#def50]
> > multipath-tools-0.11.1/libmultipath/dmparser.c:31:12: warning[-Wanalyzer-deref-before-check]: check of ‘p’ for NULL after already dereferencing it
> 
> False positive. *dst is non-NULL when we call merge_words(). We must double-check
> after calling realloc().
> (That said, we should reimplement this using strbuf functions).
> 
> > Error: GCC_ANALYZER_WARNING (CWE-476): [#def51]
> > multipath-tools-0.11.1/libmultipath/dmparser.c:436:25: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘pgp’
> 
> False positive. VECTOR_SLOT won't return NULL here. Same for the next 3 errors.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-476): [#def55]
> > multipath-tools-0.11.1/libmultipath/dmparser.c:501:33: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘pp’
> 
> Likewise. Same for the next 2 errors.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-476): [#def58]
> > multipath-tools-0.11.1/libmultipath/foreign/nvme.c:494:28: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘path’
> 
> The nvme_pathgroup is a member of struct nvme_path. nvme_pg_to_path() doesn't return NULL. Same for the next 2 errors in nvme.c
> 
> > Error: GCC_ANALYZER_WARNING (CWE-476): [#def62]
> > multipath-tools-0.11.1/libmultipath/pgpolicies.c:191:17: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘pp1’
> 
> Again, false positives; VECTOR_SLOT() doesn't return NULL here.
> Same for the next 3 errors.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-401): [#def66]
> > multipath-tools-0.11.1/libmultipath/print.c:920:16: warning[-Wanalyzer-malloc-leak]: leak of ‘alloc_path_layout()’
> 
> False positive. Strange one - gcc doesn't understand its own __attribute__((cleanup))?
> Or am I blind? Same for the next one.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-126): [#def70]
> multipath-tools-0.11.1/libmultipath/uevent.c:122:9: warning[-Wanalyzer-out-of-bounds]: stack-based buffer over-read
> 
> This one is somewhat harder because it involves list handling macros, but I believe that our code is correct and that this is a false positive, too.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-775): [#def69]
> > multipath-tools-0.11.1/libmultipath/sysfs.c:309:22: warning[-Wanalyzer-fd-leak]: leak of file descriptor ‘open(&pathbuf, 0)’
> 
> Again, false positive; there's a cleanup function for fd.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-775): [#def71]
> > multipath-tools-0.11.1/libmultipath/valid.c:208:34: warning[-Wanalyzer-file-leak]: leak of FILE ‘fopen(&mountinfo, "r")’
> 
> False positive, gcc doesn't understand the cleanup_fclose().
> Same for the next error.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-415): [#def73]
> > multipath-tools-0.11.1/multipathd/fpin_handlers.c:508:17: warning[-Wanalyzer-double-free]: double-‘free’ of ‘els_marginal_list_head.next + -2056’
> 
> I don't see an error in this code. Looks like another false positive to me.
> 
> > Error: GCC_ANALYZER_WARNING: [#def74]
> > multipath-tools-0.11.1/multipathd/fpin_handlers.c:624:23: warning[-Wanalyzer-fd-use-without-check]: ‘read’ on possibly invalid file descriptor ‘fd’
> 
> False positive, too. fd == -1 is checked beforehand.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-688): [#def75]
> > multipath-tools-0.11.1/multipathd/main.c:3286:21: warning[-Wanalyzer-null-argument]: use of NULL ‘new’ where non-null expected
> 
> Similar to other false positives above, VECTOR_SLOT() won't return NULL in a  loop like this.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-775): [#def77]
> > multipath-tools-0.11.1/multipathd/main.c:3973:12: warning[-Wanalyzer-fd-leak]: leak of file descriptor ‘dup2(open("/dev/null", 2), 0)’
> 
> Yet another false positive, the fd is closed in the cleanup handler. Same for the next 3 errors.
> 
> > Error: GCC_ANALYZER_WARNING (CWE-476): [#def81]
> multipath-tools-0.11.1/multipathd/multipathc.c:97:30: warning[-Wanalyzer-null-dereference]: dereference of NULL ‘kw’
> 
> Another case of VECTOR_SLOT() false positive. Same for the next error.
> 
> Regards
> Martin
> 
> Martin Wilck (6):
>   kpartx_id: fix shellcheck reported errors
>   kpartx: fix file descriptor leak
>   libmpathpersist: fix memory leak in mpath_prout_rel()
>   libmpathutil: silence compiler warning in vector_del_slot()
>   libmultipath: fix undefined behavior in 31-bit shift
>   libmultipath: prioritizers/iet: fix possible NULL dereference
> 
>  kpartx/kpartx.c                     | 14 +++++++-------
>  kpartx/kpartx_id                    |  8 ++++----
>  libmpathpersist/mpath_persist_int.c | 12 ++++++------
>  libmpathutil/vector.c               | 16 ++++++++--------
>  libmultipath/nvme/nvme-ioctl.c      |  2 +-
>  libmultipath/prioritizers/iet.c     |  3 +++
>  6 files changed, 29 insertions(+), 26 deletions(-)
> 
> -- 
> 2.49.0