mdadm/SOURCES/0120-mdadm-Manage.c-fix-coverity-issues.patch

From 1b4b73fd535a6487075e98f620454ff2e13b5240 Mon Sep 17 00:00:00 2001
From: Nigel Croxon <ncroxon@redhat.com>
Date: Wed, 10 Jul 2024 08:55:08 -0400
Subject: [PATCH 120/157] mdadm: Manage.c fix coverity issues

Fixing the following coding errors the coverity tools found:

* Event parameter_hidden:	declaration hides parameter "dv".
* Event leaked_storage: Variable "mdi" going out of scope leaks the storage
it points to.
* Event overwrite_var: Overwriting "mdi" in "mdi = mdi->devs" leaks the
storage that "mdi" points to.
* Event leaked_handle: Handle variable "lfd" going out of scope leaks
the handle.
* Event leaked_handle: Returning without closing handle "fd" leaks it.
* Event fixed_size_dest: You might overrun the 32-character fixed-sizei
string "devnm" by copying the return value of "fd2devnm" without
checking the length.
* Event fixed_size_dest: You might overrun the 32-character fixed-size
string "nm" by copying "nmp" without checking the length.
* Event fixed_size_dest: You might overrun the 32-character fixed-size
string "devnm" by copying the return value of "fd2devnm" without
checking the length.
* Event assigned_value: Assigning value "-1" to "tfd" here, but that
stored value is overwritten before it can be used.

Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
---
 Manage.c | 149 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 71 insertions(+), 78 deletions(-)

diff --git a/Manage.c b/Manage.c
index 5db72b77..aa5e80b2 100644
--- a/Manage.c
+++ b/Manage.c
@@ -56,7 +56,7 @@ int Manage_ro(char *devname, int fd, int readonly)
 			vers[9] = '-';
 			sysfs_set_str(mdi, NULL, "metadata_version", vers);
 
-			close(fd);
+			close_fd(&fd);
 			rv = sysfs_set_str(mdi, NULL, "array_state", "readonly");
 
 			if (rv < 0) {
@@ -165,7 +165,7 @@ int Manage_run(char *devname, int fd, struct context *c)
 		pr_err("Cannot find %s in sysfs!!\n", devname);
 		return 1;
 	}
-	strcpy(nm, nmp);
+	snprintf(nm, sizeof(nm), "%s", nmp);
 	return IncrementalScan(c, nm);
 }
 
@@ -187,7 +187,7 @@ int Manage_stop(char *devname, int fd, int verbose, int will_retry)
 	if (will_retry && verbose == 0)
 		verbose = -1;
 
-	strcpy(devnm, fd2devnm(fd));
+	snprintf(devnm, sizeof(devnm), "%s", fd2devnm(fd));
 	/* Get EXCL access first.  If this fails, then attempting
 	 * to stop is probably a bad idea.
 	 */
@@ -195,7 +195,7 @@ int Manage_stop(char *devname, int fd, int verbose, int will_retry)
 	if (mdi && is_subarray(mdi->text_version))
 		sysfs_get_container_devnm(mdi, container);
 
-	close(fd);
+	close_fd(&fd);
 	count = 5;
 	while (((fd = ((devname[0] == '/')
 		       ?open(devname, O_RDONLY|O_EXCL)
@@ -206,14 +206,12 @@ int Manage_stop(char *devname, int fd, int verbose, int will_retry)
 		 * is a container, so we might be racing with mdmon, so
 		 * retry for a bit.
 		 */
-		if (fd >= 0)
-			close(fd);
+		close_fd(&fd);
 		flush_mdmon(container);
 		count--;
 	}
 	if (fd < 0 || strcmp(fd2devnm(fd), devnm) != 0) {
-		if (fd >= 0)
-			close(fd);
+		close_fd(&fd);
 		if (verbose >= 0)
 			pr_err("Cannot get exclusive access to %s:Perhaps a running process, mounted filesystem or active volume group?\n",
 			       devname);
@@ -228,7 +226,7 @@ int Manage_stop(char *devname, int fd, int verbose, int will_retry)
 	    is_subarray(mdi->text_version)) {
 		int err;
 		/* This is mdmon managed. */
-		close(fd);
+		close_fd(&fd);
 
 		/* As we had an O_EXCL open, any use of the device
 		 * which blocks STOP_ARRAY is probably a transient use,
@@ -430,8 +428,7 @@ int Manage_stop(char *devname, int fd, int verbose, int will_retry)
 				break;
 			sysfs_wait(scfd, &delay);
 		}
-		if (scfd >= 0)
-			close(scfd);
+		close_fd(&scfd);
 
 	}
 done:
@@ -469,6 +466,7 @@ done:
 	map_unlock(&map);
 out:
 	sysfs_free(mdi);
+	close_fd(&fd);
 
 	return rv;
 }
@@ -664,7 +662,7 @@ int attempt_re_add(int fd, int tfd, struct mddev_dev *dv,
 					devname, verbose, 0, NULL);
 			if (rv == 0)
 				rv = dev_st->ss->store_super(dev_st, tfd);
-			close(tfd);
+			close_fd(&tfd);
 			if (rv != 0) {
 				pr_err("failed to update superblock during re-add\n");
 				return -1;
@@ -766,15 +764,15 @@ mdadm_status_t manage_add_external(struct supertype *st, int fd, char *disk_name
 	rv = MDADM_STATUS_SUCCESS;
 
 out:
-	close(container_fd);
+	close_fd(&container_fd);
 	dev_policy_free(pols);
 
 	if (sra)
 		sysfs_free(sra);
 
-	if (rv != MDADM_STATUS_SUCCESS && is_fd_valid(disk_fd))
+	if (rv != MDADM_STATUS_SUCCESS)
 		/* Metadata handler records this descriptor, so release it only on failure. */
-		close(disk_fd);
+		close_fd(&disk_fd);
 
 	if (st->sb)
 		st->ss->free_super(st);
@@ -845,10 +843,10 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,
 					continue;
 				if (tst->ss->load_super(tst, dfd,
 							NULL)) {
-					close(dfd);
+					close_fd(&dfd);
 					continue;
 				}
-				close(dfd);
+				close_fd(&dfd);
 				break;
 			}
 		/* FIXME this is a bad test to be using */
@@ -1100,7 +1098,8 @@ int Manage_remove(struct supertype *tst, int fd, struct mddev_dev *dv,
 		 */
 		int ret;
 		char devnm[32];
-		strcpy(devnm, fd2devnm(fd));
+
+		snprintf(devnm, sizeof(devnm), "%s", fd2devnm(fd));
 		lfd = open_dev_excl(devnm);
 		if (lfd < 0) {
 			pr_err("Cannot get exclusive access  to container - odd\n");
@@ -1134,13 +1133,13 @@ int Manage_remove(struct supertype *tst, int fd, struct mddev_dev *dv,
 			if (ret == 0) {
 				pr_err("%s is not a member, cannot remove.\n",
 					dv->devname);
-				close(lfd);
+				close_fd(&lfd);
 				return -1;
 			}
 			if (ret >= 2) {
 				pr_err("%s is still in use, cannot remove.\n",
 					dv->devname);
-				close(lfd);
+				close_fd(&lfd);
 				return -1;
 			}
 		}
@@ -1157,26 +1156,27 @@ int Manage_remove(struct supertype *tst, int fd, struct mddev_dev *dv,
 			/* Old kernels rejected this if no personality
 			 * is registered */
 			struct mdinfo *sra = sysfs_read(fd, NULL, GET_DEVS);
-			struct mdinfo *dv = NULL;
-			if (sra)
-				dv = sra->devs;
-			for ( ; dv ; dv=dv->next)
-				if (dv->disk.major == (int)major(rdev) &&
-				    dv->disk.minor == (int)minor(rdev))
-					break;
-			if (dv)
-				err = sysfs_set_str(sra, dv,
-						    "state", "remove");
-			else
+			struct mdinfo *dev = NULL;
+
+			if (!sra) {
 				err = -1;
-			sysfs_free(sra);
+			} else {
+				for (dev = sra->devs; dev ; dev = dev->next)
+					if (dev->disk.major == (int)major(rdev) &&
+					    dev->disk.minor == (int)minor(rdev))
+						break;
+
+				if (dev)
+					err = sysfs_set_str(sra, dev,
+						    "state", "remove");
+				sysfs_free(sra);
+			}
 		}
 	}
 	if (err) {
 		pr_err("hot remove failed for %s: %s\n",	dv->devname,
 		       strerror(errno));
-		if (lfd >= 0)
-			close(lfd);
+		close_fd(&lfd);
 		return -1;
 	}
 	if (tst->ss->external) {
@@ -1190,13 +1190,13 @@ int Manage_remove(struct supertype *tst, int fd, struct mddev_dev *dv,
 
 		if (!devnm) {
 			pr_err("unable to get container name\n");
+			close_fd(&lfd);
 			return -1;
 		}
 
 		ping_manager(devnm);
 	}
-	if (lfd >= 0)
-		close(lfd);
+	close_fd(&lfd);
 	if (verbose >= 0)
 		pr_err("hot removed %s from %s\n",
 		       dv->devname, devname);
@@ -1218,7 +1218,7 @@ int Manage_replace(struct supertype *tst, int fd, struct mddev_dev *dv,
 	if (!mdi || !mdi->devs) {
 		pr_err("Cannot find status of %s to enable replacement - strange\n",
 		       devname);
-		return -1;
+		goto abort;
 	}
 	for (di = mdi->devs; di; di = di->next)
 		if (di->disk.major == (int)major(rdev) &&
@@ -1229,16 +1229,14 @@ int Manage_replace(struct supertype *tst, int fd, struct mddev_dev *dv,
 		if (di->disk.raid_disk < 0) {
 			pr_err("%s is not active and so cannot be replaced.\n",
 			       dv->devname);
-			sysfs_free(mdi);
-			return -1;
+			goto abort;
 		}
 		rv = sysfs_set_str(mdi, di,
 				   "state", "want_replacement");
 		if (rv) {
-			sysfs_free(mdi);
 			pr_err("Failed to request replacement for %s\n",
 			       dv->devname);
-			return -1;
+			goto abort;
 		}
 		if (verbose >= 0)
 			pr_err("Marked %s (device %d in %s) for replacement\n",
@@ -1252,11 +1250,13 @@ int Manage_replace(struct supertype *tst, int fd, struct mddev_dev *dv,
 			dv->disposition = 'w';
 			dv->used = di->disk.raid_disk;
 		}
+		sysfs_free(mdi);
 		return 1;
 	}
-	sysfs_free(mdi);
 	pr_err("%s not found in %s so cannot --replace it\n",
 	       dv->devname, devname);
+abort:
+	sysfs_free(mdi);
 	return -1;
 }
 
@@ -1269,7 +1269,7 @@ int Manage_with(struct supertype *tst, int fd, struct mddev_dev *dv,
 	if (!mdi || !mdi->devs) {
 		pr_err("Cannot find status of %s to enable replacement - strange\n",
 		       devname);
-		return -1;
+		goto abort;
 	}
 	for (di = mdi->devs; di; di = di->next)
 		if (di->disk.major == (int)major(rdev) &&
@@ -1280,31 +1280,30 @@ int Manage_with(struct supertype *tst, int fd, struct mddev_dev *dv,
 		if (di->disk.state & (1<<MD_DISK_FAULTY)) {
 			pr_err("%s is faulty and cannot be a replacement\n",
 			       dv->devname);
-			sysfs_free(mdi);
-			return -1;
+			goto abort;
 		}
 		if (di->disk.raid_disk >= 0) {
 			pr_err("%s is active and cannot be a replacement\n",
 			       dv->devname);
-			sysfs_free(mdi);
-			return -1;
+			goto abort;
 		}
 		rv = sysfs_set_num(mdi, di,
 				   "slot", dv->used);
 		if (rv) {
-			sysfs_free(mdi);
 			pr_err("Failed to set %s as preferred replacement.\n",
 			       dv->devname);
-			return -1;
+			goto abort;
 		}
 		if (verbose >= 0)
 			pr_err("Marked %s in %s as replacement for device %d\n",
 			       dv->devname, devname, dv->used);
+		sysfs_free(mdi);
 		return 1;
 	}
-	sysfs_free(mdi);
 	pr_err("%s not found in %s so cannot make it preferred replacement\n",
 	       dv->devname, devname);
+abort:
+	sysfs_free(mdi);
 	return -1;
 }
 
@@ -1324,6 +1323,7 @@ bool is_remove_safe(mdu_array_info_t *array, const int fd, char *devname, const
 {
 	dev_t devid = devnm2devid(devname + 5);
 	struct mdinfo *mdi = sysfs_read(fd, NULL, GET_DEVS | GET_DISKS | GET_STATE);
+	struct mdinfo *disk;
 
 	if (!mdi) {
 		if (verbose)
@@ -1333,14 +1333,14 @@ bool is_remove_safe(mdu_array_info_t *array, const int fd, char *devname, const
 
 	char *avail = xcalloc(array->raid_disks, sizeof(char));
 
-	for (mdi = mdi->devs; mdi; mdi = mdi->next) {
-		if (mdi->disk.raid_disk < 0)
+	for (disk = mdi->devs; disk; disk = mdi->next) {
+		if (disk->disk.raid_disk < 0)
 			continue;
-		if (!(mdi->disk.state & (1 << MD_DISK_SYNC)))
+		if (!(disk->disk.state & (1 << MD_DISK_SYNC)))
 			continue;
-		if (makedev(mdi->disk.major, mdi->disk.minor) == devid)
+		if (makedev(disk->disk.major, disk->disk.minor) == devid)
 			continue;
-		avail[mdi->disk.raid_disk] = 1;
+		avail[disk->disk.raid_disk] = 1;
 	}
 	sysfs_free(mdi);
 
@@ -1550,7 +1550,7 @@ int Manage_subdevs(char *devname, int fd,
 					rdev = makedev(mj,mn);
 					found = 1;
 				}
-				close(sysfd);
+				close_fd(&sysfd);
 				sysfd = -1;
 			}
 			if (!found) {
@@ -1572,7 +1572,7 @@ int Manage_subdevs(char *devname, int fd,
 			tfd = dev_open(dv->devname, O_RDONLY);
 			if (tfd >= 0) {
 				fstat_is_blkdev(tfd, dv->devname, &rdev);
-				close(tfd);
+				close_fd(&tfd);
 			} else {
 				int open_err = errno;
 				if (!stat_is_blkdev(dv->devname, &rdev)) {
@@ -1635,7 +1635,7 @@ int Manage_subdevs(char *devname, int fd,
 				 * need non-exclusive access to add it, so
 				 * do that now.
 				 */
-				close(tfd);
+				close_fd(&tfd);
 				tfd = dev_open(dv->devname, O_RDONLY);
 			}
 			if (tfd < 0) {
@@ -1654,8 +1654,7 @@ int Manage_subdevs(char *devname, int fd,
 			rv = Manage_add(fd, tfd, dv, tst, &array,
 					force, verbose, devname, update,
 					rdev, array_size, raid_slot);
-			close(tfd);
-			tfd = -1;
+			close_fd(&tfd);
 			if (rv < 0)
 				goto abort;
 			if (rv > 0)
@@ -1672,7 +1671,7 @@ int Manage_subdevs(char *devname, int fd,
 						   rdev, verbose, force,
 						   devname);
 			if (sysfd >= 0)
-				close(sysfd);
+				close_fd(&sysfd);
 			sysfd = -1;
 			if (rv < 0)
 				goto abort;
@@ -1684,8 +1683,7 @@ int Manage_subdevs(char *devname, int fd,
 			if (!is_remove_safe(&array, fd, dv->devname, verbose)) {
 				pr_err("Cannot remove %s from %s, array will be failed.\n",
 				       dv->devname, devname);
-				if (sysfd >= 0)
-					close(sysfd);
+				close_fd(&sysfd);
 				goto abort;
 			}
 		case 'I': /* incremental fail */
@@ -1696,13 +1694,10 @@ int Manage_subdevs(char *devname, int fd,
 					busy = 1;
 				pr_err("set device faulty failed for %s:  %s\n",
 					dv->devname, strerror(errno));
-				if (sysfd >= 0)
-					close(sysfd);
+				close_fd(&sysfd);
 				goto abort;
 			}
-			if (sysfd >= 0)
-				close(sysfd);
-			sysfd = -1;
+			close_fd(&sysfd);
 			count++;
 			if (verbose >= 0)
 				pr_err("set %s faulty in %s\n",
@@ -1762,7 +1757,7 @@ int autodetect(void)
 	if (fd >= 0) {
 		if (ioctl(fd, RAID_AUTORUN, 0) == 0)
 			rv = 0;
-		close(fd);
+		close_fd(&fd);
 	}
 	return rv;
 }
@@ -1825,7 +1820,7 @@ free_super:
 	if (info)
 		free(info);
 	st->ss->free_super(st);
-	close(fd);
+	close_fd(&fd);
 
 	return rv;
 }
@@ -1843,10 +1838,8 @@ int move_spare(char *from_devname, char *to_devname, dev_t devid)
 	int fd2 = open(from_devname, O_RDONLY);
 
 	if (fd1 < 0 || fd2 < 0) {
-		if (fd1 >= 0)
-			close(fd1);
-		if (fd2 >= 0)
-			close(fd2);
+		close_fd(&fd1);
+		close_fd(&fd2);
 		return 0;
 	}
 
@@ -1865,15 +1858,15 @@ int move_spare(char *from_devname, char *to_devname, dev_t devid)
 			/* make sure manager is aware of changes */
 			ping_manager(to_devname);
 			ping_manager(from_devname);
-			close(fd1);
-			close(fd2);
+			close_fd(&fd1);
+			close_fd(&fd2);
 			return 1;
 		}
 		else
 			Manage_subdevs(from_devname, fd2, &devlist,
 				       -1, 0, UOPT_UNDEFINED, 0);
 	}
-	close(fd1);
-	close(fd2);
+	close_fd(&fd1);
+	close_fd(&fd2);
 	return 0;
 }
-- 
2.41.0