Fix TOCTOU race in zpool_do_labelclear()

Coverity reported a TOCTOU race in `zpool_do_labelclear()`. This is not
believed to be a real security issue, but fixing it reduces the number
of syscalls we do and will prevent other static analyzers from
complaining about this.

The code is expected to be equivalent. However, under rare
circumstances, such as ELOOP, ENAMETOOLONG, ENOMEM, ENOTDIR and
EOVERFLOW, we will display the error message that we currently display
for the `open()` syscall rather than the one that we currently display
for the `stat()` syscall. This is considered to be an improvement.

Reported-by: Coverity (CID-1524188)
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Closes #14575

diff --git a/cmd/zpool/zpool_main.c b/cmd/zpool/zpool_main.c
index efb2d10e591bb6173ab4db800082be6d3358670c..d2614411df3ff3c2037bccdd6cbe15c2c7cb5729 100644 (file)
--- a/cmd/zpool/zpool_main.c
+++ b/cmd/zpool/zpool_main.c
@@ -1288,7 +1288,6 @@ zpool_do_labelclear(int argc, char **argv)
 {
        char vdev[MAXPATHLEN];
        char *name = NULL;
-       struct stat st;
        int c, fd = -1, ret = 0;
        nvlist_t *config;
        pool_state_t state;
@@ -1321,14 +1320,20 @@ zpool_do_labelclear(int argc, char **argv)
                usage(B_FALSE);
        }
 
+       (void) strlcpy(vdev, argv[0], sizeof (vdev));
+
        /*
-        * Check if we were given absolute path and use it as is.
+        * If we cannot open an absolute path, we quit.
         * Otherwise if the provided vdev name doesn't point to a file,
         * try prepending expected disk paths and partition numbers.
         */
-       (void) strlcpy(vdev, argv[0], sizeof (vdev));
-       if (vdev[0] != '/' && stat(vdev, &st) != 0) {
+       if ((fd = open(vdev, O_RDWR)) < 0) {
                int error;
+               if (vdev[0] == '/') {
+                       (void) fprintf(stderr, gettext("failed to open "
+                           "%s: %s\n"), vdev, strerror(errno));
+                       return (1);
+               }
 
                error = zfs_resolve_shortname(argv[0], vdev, MAXPATHLEN);
                if (error == 0 && zfs_dev_is_whole_disk(vdev)) {
@@ -1336,20 +1341,21 @@ zpool_do_labelclear(int argc, char **argv)
                                error = ENOENT;
                }
 
-               if (error || (stat(vdev, &st) != 0)) {
-                       (void) fprintf(stderr, gettext(
-                           "failed to find device %s, try specifying absolute "
-                           "path instead\n"), argv[0]);
+               if (error || ((fd = open(vdev, O_RDWR)) < 0)) {
+                       if (errno == ENOENT) {
+                               (void) fprintf(stderr, gettext(
+                                   "failed to find device %s, try "
+                                   "specifying absolute path instead\n"),
+                                   argv[0]);
+                               return (1);
+                       }
+
+                       (void) fprintf(stderr, gettext("failed to open %s:"
+                           " %s\n"), vdev, strerror(errno));
                        return (1);
                }
        }
 
-       if ((fd = open(vdev, O_RDWR)) < 0) {
-               (void) fprintf(stderr, gettext("failed to open %s: %s\n"),
-                   vdev, strerror(errno));
-               return (1);
-       }
-
        /*
         * Flush all dirty pages for the block device.  This should not be
         * fatal when the device does not support BLKFLSBUF as would be the