[Libguestfs] [PATCH 14/14] daemon: Fix use-after-free in case-insensitive-path (found by valgrind).

[Richard W.M. Jones]

From: "Richard W.M. Jones" <rjones at redhat.com>

This commit tidies up the code by splitting out the path
element-searching code into a separate function.

Valgrind found that 'closedir' frees the 'struct dirent *', which
wasn't immediately obvious.  So now we do the 'closedir' after all
operations which touch 'd->d_name'.
---
 daemon/realpath.c |  119 ++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 81 insertions(+), 38 deletions(-)

diff --git a/daemon/realpath.c b/daemon/realpath.c
index c58fc6c..8ec9674 100644
--- a/daemon/realpath.c
+++ b/daemon/realpath.c
@@ -66,6 +66,8 @@ do_realpath (const char *path)
 #endif
 }
 
+static int find_path_element (int fd_cwd, char *name, size_t *name_len_ret);
+
 char *
 do_case_sensitive_path (const char *path)
 {
@@ -110,57 +112,26 @@ do_case_sensitive_path (const char *path)
     path += i;
 
     /* Read the current directory looking (case insensitively) for
-     * this element of the path.
+     * this element of the path.  This replaces 'name' with the
+     * correct case version.
      */
-    int fd2 = dup (fd_cwd); /* because closedir will close it */
-    if (fd2 == -1) {
-      reply_with_perror ("dup");
-      goto error;
-    }
-    DIR *dir = fdopendir (fd2);
-    if (dir == NULL) {
-      reply_with_perror ("opendir");
-      goto error;
-    }
-
-    struct dirent *d = NULL;
-
-    errno = 0;
-    while ((d = readdir (dir)) != NULL) {
-      if (STRCASEEQ (d->d_name, name))
-        break;
-    }
-
-    if (d == NULL && errno != 0) {
-      reply_with_perror ("readdir");
+    if (find_path_element (fd_cwd, name, &i) == -1)
       goto error;
-    }
-
-    if (closedir (dir) == -1) {
-      reply_with_perror ("closedir");
-      goto error;
-    }
-
-    if (d == NULL) {
-      reply_with_error ("%s: no file or directory found with this name", name);
-      goto error;
-    }
 
     /* Add the real name of this path element to the return value. */
     if (next > 1)
       ret[next++] = '/';
 
-    i = strlen (d->d_name);
     if (next + i >= PATH_MAX) {
       reply_with_error ("final path too long");
       goto error;
     }
 
-    strcpy (&ret[next], d->d_name);
+    strcpy (&ret[next], name);
     next += i;
 
     /* Is it a directory?  Try going into it. */
-    fd2 = openat (fd_cwd, d->d_name, O_RDONLY | O_DIRECTORY);
+    int fd2 = openat (fd_cwd, name, O_RDONLY | O_DIRECTORY);
     int err = errno;
     close (fd_cwd);
     fd_cwd = fd2;
@@ -168,12 +139,12 @@ do_case_sensitive_path (const char *path)
     if (fd_cwd == -1) {
       /* ENOTDIR is OK provided we've reached the end of the path. */
       if (errno != ENOTDIR) {
-        reply_with_perror ("openat: %s", d->d_name);
+        reply_with_perror ("openat: %s", name);
         goto error;
       }
 
       if (*path) {
-        reply_with_error ("%s: non-directory element in path", d->d_name);
+        reply_with_error ("%s: non-directory element in path", name);
         goto error;
       }
     }
@@ -196,3 +167,75 @@ do_case_sensitive_path (const char *path)
 
   return NULL;
 }
+
+/* 'fd_cwd' is a file descriptor pointing to an open directory.
+ * 'name' is a buffer of NAME_MAX+1 characters in size which initially
+ * contains the path element to search for.
+ *
+ * We search the directory looking for a path element that case
+ * insensitively matches 'name' and update the 'name' buffer.
+ *
+ * If this is successful, return 0.  If it fails, reply with an error
+ * and return -1.
+ */
+static int
+find_path_element (int fd_cwd, char *name, size_t *name_len_ret)
+{
+  int fd2;
+  DIR *dir;
+  struct dirent *d;
+
+  fd2 = dup (fd_cwd); /* because closedir will close it */
+  if (fd2 == -1) {
+    reply_with_perror ("dup");
+    return -1;
+  }
+  dir = fdopendir (fd2);
+  if (dir == NULL) {
+    reply_with_perror ("opendir");
+    close (fd2);
+    return -1;
+  }
+
+  for (;;) {
+    errno = 0;
+    d = readdir (dir);
+    if (d == NULL)
+      break;
+    if (STRCASEEQ (d->d_name, name))
+      break;
+  }
+
+  if (d == NULL && errno != 0) {
+    reply_with_perror ("readdir");
+    closedir (dir);
+    return -1;
+  }
+
+  if (d == NULL) {
+    reply_with_error ("%s: no file or directory found with this name", name);
+    closedir (dir);
+    return -1;
+  }
+
+  *name_len_ret = strlen (d->d_name);
+  if (*name_len_ret > NAME_MAX) {
+    /* Should never happen? */
+    reply_with_error ("path element longer than NAME_MAX");
+    closedir (dir);
+    return -1;
+  }
+
+  /* Safe because name is a buffer of NAME_MAX+1 characters. */
+  strcpy (name, d->d_name);
+
+  /* NB: closedir frees the structure associated with 'd', so we must
+   * do this last.
+   */
+  if (closedir (dir) == -1) {
+    reply_with_perror ("closedir");
+    return -1;
+  }
+
+  return 0;
+}
-- 
1.7.6