Fix recursive directory deletion, to not leak file descriptors (and pointers).

runtime/bin/directory_linux.cc

Index: runtime/bin/directory_linux.cc
diff --git a/runtime/bin/directory_linux.cc b/runtime/bin/directory_linux.cc
index f6b09c436d3008d690f5d67980addc030b183190..17fa29c8ff491de3ee6d59e3ec7312f1c885da83 100644
--- a/runtime/bin/directory_linux.cc
+++ b/runtime/bin/directory_linux.cc
@@ -245,74 +245,67 @@ static bool DeleteRecursively(PathBuffer* path) {
 
   // Not a link. Attempt to open as a directory and recurse into the
   // directory.
-  DIR* dir_pointer;
-  do {
-    dir_pointer = opendir(path->AsString());
-  } while (dir_pointer == NULL && errno == EINTR);
-
+  DIR* dir_pointer = opendir(path->AsString());

[Søren Gjesse, 2014/04/23 11:58:47]

NO_RETRY_EXPECTED here?

[Anders Johnsen, 2014/04/23 12:09:03]

Not compatible with pointers, sadly.

   if (dir_pointer == NULL) {
     return false;
   }
 
   // Iterate the directory and delete all files and directories.
   int path_length = path->length();
-  int read = 0;
-  bool success = true;
   dirent entry;
   dirent* result;
-  while ((read = NO_RETRY_EXPECTED(
-      readdir_r(dir_pointer, &entry, &result))) == 0 &&
-                result != NULL &&
-                success) {
+  while (NO_RETRY_EXPECTED(readdir_r(dir_pointer, &entry, &result)) == 0) {
+    if (result == NULL) {
+      // End of directory.
+      return NO_RETRY_EXPECTED(closedir(dir_pointer)) == 0 &&

[Søren Gjesse, 2014/04/23 11:58:47]

closedir is using VOID_NO_RETRY_EXPECTED below

[Anders Johnsen, 2014/04/23 12:09:03]

Yes, that's because we already failed in that case (we can't double-report
errors).

+          NO_RETRY_EXPECTED(remove(path->AsString())) == 0;
+    }
+    bool ok = false;
     switch (entry.d_type) {
       case DT_DIR:
-        success = success && DeleteDir(entry.d_name, path);
+        ok = DeleteDir(entry.d_name, path);
         break;
       case DT_REG:
       case DT_LNK:
         // Treat all links as files. This will delete the link which
         // is what we want no matter if the link target is a file or a
         // directory.
-        success = success && DeleteFile(entry.d_name, path);
+        ok = DeleteFile(entry.d_name, path);
         break;
       case DT_UNKNOWN: {
         if (!path->Add(entry.d_name)) {
-          success = false;
           break;
         }
         // On some file systems the entry type is not determined by
         // readdir_r. For those we use lstat to determine the entry
         // type.
         struct stat64 entry_info;
-        int lstat_success = NO_RETRY_EXPECTED(
-            lstat64(path->AsString(), &entry_info));
-        if (lstat_success == -1) {
-          success = false;
+        if (NO_RETRY_EXPECTED(lstat64(path->AsString(), &entry_info)) == -1) {
           break;
         }
         path->Reset(path_length);
         if (S_ISDIR(entry_info.st_mode)) {
-          success = success && DeleteDir(entry.d_name, path);
+          ok = DeleteDir(entry.d_name, path);
         } else if (S_ISREG(entry_info.st_mode) || S_ISLNK(entry_info.st_mode)) {
           // Treat links as files. This will delete the link which is
           // what we want no matter if the link target is a file or a
           // directory.
-          success = success && DeleteFile(entry.d_name, path);
+          ok = DeleteFile(entry.d_name, path);
         }
         break;
       }
       default:
         break;
     }
+    if (!ok) {
+      break;
+    }
     path->Reset(path_length);
   }

[Søren Gjesse, 2014/04/23 11:58:47]

Add comment that we only get here if there is an error. Maybe assert that errno
is not 0.

[Anders Johnsen, 2014/04/23 12:09:03]

Done.

-
-  if ((read != 0) ||
-      (NO_RETRY_EXPECTED(closedir(dir_pointer)) == -1) ||
-      (NO_RETRY_EXPECTED(remove(path->AsString())) == -1)) {
-    return false;
-  }
-  return success;
+  int err = errno;
+  VOID_NO_RETRY_EXPECTED(closedir(dir_pointer));
+  errno = err;
+  return false;
 }