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

runtime/bin/directory_linux.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "platform/globals.h"
 #if defined(TARGET_OS_LINUX)
 
 #include "bin/directory.h"
 
 #include <dirent.h>  // NOLINT
@@ skipping 227 matching lines @@
   if (NO_RETRY_EXPECTED(lstat64(path->AsString(), &st)) == -1) {
     return false;
   } else if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
     return (NO_RETRY_EXPECTED(unlink(path->AsString())) == 0);
   }
 
   if (!path->Add(File::PathSeparator())) return false;
 
   // 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;
 }
 
 
 Directory::ExistsResult Directory::Exists(const char* dir_name) {
   struct stat64 entry_info;
   int success = NO_RETRY_EXPECTED(stat64(dir_name, &entry_info));
   if (success == 0) {
     if (S_ISDIR(entry_info.st_mode)) {
       return EXISTS;
     } else {
@@ skipping 112 matching lines @@
 bool Directory::Rename(const char* path, const char* new_path) {
   ExistsResult exists = Exists(path);
   if (exists != EXISTS) return false;
   return NO_RETRY_EXPECTED(rename(path, new_path)) == 0;
 }
 
 }  // namespace bin
 }  // namespace dart
 
 #endif  // defined(TARGET_OS_LINUX)