Fixes some memory leaks in //runtime/bin

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 d150bf276daff920e6a9b55ae0271bff133564c1..d2e556f807ece9508f9abc602c0a4e54fff0c032 100644
--- a/runtime/bin/directory_linux.cc
+++ b/runtime/bin/directory_linux.cc
@@ -15,33 +15,41 @@
 #include <sys/stat.h>  // NOLINT
 #include <unistd.h>  // NOLINT
 
-#include "platform/signal_blocker.h"
+#include "bin/dartutils.h"
 #include "bin/file.h"
 #include "bin/platform.h"
-
+#include "platform/signal_blocker.h"
 
 namespace dart {
 namespace bin {
 
-
 PathBuffer::PathBuffer() : length_(0) {
-  data_ = calloc(PATH_MAX + 1,  sizeof(char));  // NOLINT
+  data_ = calloc(PATH_MAX + 1, sizeof(char));  // NOLINT
 }
 
+
 bool PathBuffer::AddW(const wchar_t* name) {
   UNREACHABLE();
   return false;
 }
 
+
 char* PathBuffer::AsString() const {
   return reinterpret_cast<char*>(data_);
 }
 
+
 wchar_t* PathBuffer::AsStringW() const {
   UNREACHABLE();
   return NULL;
 }
 
+
+const char* PathBuffer::AsScopedString() const {
+  return DartUtils::ScopedCopyCString(AsString());
+}
+
+
 bool PathBuffer::Add(const char* name) {
   char* data = AsString();
   int written = snprintf(data + length_,
@@ -49,9 +57,9 @@ bool PathBuffer::Add(const char* name) {
                          "%s",
                          name);
   data[PATH_MAX] = '\0';
-  if (written <= PATH_MAX - length_ &&
-      written >= 0 &&
-      static_cast<size_t>(written) == strnlen(name, PATH_MAX + 1)) {
+  if ((written <= PATH_MAX - length_) &&
+      (written >= 0) &&
+      (static_cast<size_t>(written) == strnlen(name, PATH_MAX + 1))) {
     length_ += written;
     return true;
   } else {
@@ -60,7 +68,8 @@ bool PathBuffer::Add(const char* name) {
   }
 }
 
-void PathBuffer::Reset(int new_length) {
+
+void PathBuffer::Reset(intptr_t new_length) {
   length_ = new_length;
   AsString()[length_] = '\0';
 }
@@ -84,7 +93,7 @@ ListType DirectoryListingEntry::Next(DirectoryListing* listing) {
     do {
       lister_ = reinterpret_cast<intptr_t>(
           opendir(listing->path_buffer().AsString()));
-    } while (lister_ == 0 && errno == EINTR);
+    } while ((lister_ == 0) && (errno == EINTR));
 
     if (lister_ == 0) {
       done_ = true;
@@ -106,18 +115,19 @@ ListType DirectoryListingEntry::Next(DirectoryListing* listing) {
   int status = 0;
   dirent entry;
   dirent* result;
-  if ((status = NO_RETRY_EXPECTED(readdir_r(reinterpret_cast<DIR*>(lister_),
-                                  &entry,
-                                  &result))) == 0 &&
-      result != NULL) {
+  status = NO_RETRY_EXPECTED(readdir_r(
+      reinterpret_cast<DIR*>(lister_), &entry, &result));
+  if ((status == 0) && (result != NULL)) {
     if (!listing->path_buffer().Add(entry.d_name)) {
       done_ = true;
       return kListError;
     }
     switch (entry.d_type) {
       case DT_DIR:
-        if (strcmp(entry.d_name, ".") == 0) return Next(listing);
-        if (strcmp(entry.d_name, "..") == 0) return Next(listing);
+        if ((strcmp(entry.d_name, ".") == 0) ||
+            (strcmp(entry.d_name, "..") == 0)) {
+          return Next(listing);
+        }
         return kListDirectory;
       case DT_REG:
         return kListFile;
@@ -146,8 +156,8 @@ ListType DirectoryListingEntry::Next(DirectoryListing* listing) {
                                     link_ };
           LinkList* previous = link_;
           while (previous != NULL) {
-            if (previous->dev == current_link.dev &&
-                previous->ino == current_link.ino) {
+            if ((previous->dev == current_link.dev) &&
+                (previous->ino == current_link.ino)) {
               // Report the looping link as a link, rather than following it.
               return kListLink;
             }
@@ -163,14 +173,18 @@ ListType DirectoryListingEntry::Next(DirectoryListing* listing) {
             // Recurse into the subdirectory with current_link added to the
             // linked list of seen file system links.
             link_ = new LinkList(current_link);
-            if (strcmp(entry.d_name, ".") == 0) return Next(listing);
-            if (strcmp(entry.d_name, "..") == 0) return Next(listing);
+            if ((strcmp(entry.d_name, ".") == 0) ||
+                (strcmp(entry.d_name, "..") == 0)) {
+              return Next(listing);
+            }
             return kListDirectory;
           }
         }
         if (S_ISDIR(entry_info.st_mode)) {
-          if (strcmp(entry.d_name, ".") == 0) return Next(listing);
-          if (strcmp(entry.d_name, "..") == 0) return Next(listing);
+          if ((strcmp(entry.d_name, ".") == 0) ||
+              (strcmp(entry.d_name, "..") == 0)) {
+            return Next(listing);
+          }
           return kListDirectory;
         } else if (S_ISREG(entry_info.st_mode)) {
           return kListFile;
@@ -203,7 +217,7 @@ DirectoryListingEntry::~DirectoryListingEntry() {
 
 
 void DirectoryListingEntry::ResetLink() {
-  if (link_ != NULL && (parent_ == NULL || parent_->link_ != link_)) {
+  if ((link_ != NULL) && ((parent_ == NULL) || (parent_->link_ != link_))) {
     delete link_;
     link_ = NULL;
   }
@@ -219,14 +233,15 @@ static bool DeleteRecursively(PathBuffer* path);
 static bool DeleteFile(char* file_name,
                        PathBuffer* path) {
   return path->Add(file_name) &&
-      NO_RETRY_EXPECTED(unlink(path->AsString())) == 0;
+      (NO_RETRY_EXPECTED(unlink(path->AsString())) == 0);
 }
 
 
 static bool DeleteDir(char* dir_name,
                       PathBuffer* path) {
-  if (strcmp(dir_name, ".") == 0) return true;
-  if (strcmp(dir_name, "..") == 0) return true;
+  if ((strcmp(dir_name, ".") == 0) || (strcmp(dir_name, "..") == 0)) {
+    return true;
+  }
   return path->Add(dir_name) && DeleteRecursively(path);
 }
 
@@ -241,14 +256,16 @@ static bool DeleteRecursively(PathBuffer* path) {
     return (NO_RETRY_EXPECTED(unlink(path->AsString())) == 0);
   }
 
-  if (!path->Add(File::PathSeparator())) return false;
+  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);
+  } while ((dir_pointer == NULL) && (errno == EINTR));
   if (dir_pointer == NULL) {
     return false;
   }
@@ -260,8 +277,8 @@ static bool DeleteRecursively(PathBuffer* path) {
   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 &&
-          NO_RETRY_EXPECTED(remove(path->AsString())) == 0;
+      return (NO_RETRY_EXPECTED(closedir(dir_pointer)) == 0) &&
+             (NO_RETRY_EXPECTED(remove(path->AsString())) == 0);
     }
     bool ok = false;
     switch (entry.d_type) {
@@ -324,38 +341,38 @@ Directory::ExistsResult Directory::Exists(const char* dir_name) {
       return DOES_NOT_EXIST;
     }
   } else {
-    if (errno == EACCES ||
-        errno == EBADF ||
-        errno == EFAULT ||
-        errno == ENOMEM ||
-        errno == EOVERFLOW) {
+    if ((errno == EACCES) ||
+        (errno == EBADF) ||
+        (errno == EFAULT) ||
+        (errno == ENOMEM) ||
+        (errno == EOVERFLOW)) {
       // Search permissions denied for one of the directories in the
       // path or a low level error occured. We do not know if the
       // directory exists.
       return UNKNOWN;
     }
-    ASSERT(errno == ELOOP ||
-           errno == ENAMETOOLONG ||
-           errno == ENOENT ||
-           errno == ENOTDIR);
+    ASSERT((errno == ELOOP) ||
+           (errno == ENAMETOOLONG) ||
+           (errno == ENOENT) ||
+           (errno == ENOTDIR));
     return DOES_NOT_EXIST;
   }
 }
 
 
-char* Directory::Current() {
-  size_t size = PATH_MAX;
-  char* buffer = NULL;
-  for (char* result = NULL; result == NULL; size *= 2) {
-    if ((buffer = reinterpret_cast<char*>(realloc(buffer, size))) == NULL) {
-      return NULL;
-    }
-    result = getcwd(buffer, size);
-    if (result == NULL && errno != ERANGE) {
-      return NULL;
-    }
+char* Directory::CurrentNoScope() {
+  char buffer[PATH_MAX];
+  if (NULL == getcwd(buffer, PATH_MAX)) {
+    return NULL;
   }
-  return buffer;
+  return strdup(buffer);
+}
+
+
+const char* Directory::Current() {
+  char* result = DartUtils::ScopedCString(PATH_MAX);

[Ivan Posva, 2016/03/13 20:59:19]

Did you consider not having to over allocate from the scoped memory area
smilarly to CurrentNoScope()?

[zra, 2016/03/14 18:07:52]

Changed to be like CurrentNoScope but with ScopedCopyCString instead of strdup.

+  ASSERT(result != NULL);
+  return getcwd(result, PATH_MAX);
 }
 
 
@@ -369,14 +386,15 @@ bool Directory::Create(const char* dir_name) {
   // process umask.
   int result = NO_RETRY_EXPECTED(mkdir(dir_name, 0777));
   // If the directory already exists, treat it as a success.
-  if (result == -1 && errno == EEXIST) {
+  if ((result == -1) && (errno == EEXIST)) {
     return (Exists(dir_name) == EXISTS);
   }
   return (result == 0);
 }
 
 
-char* Directory::SystemTemp() {
+const char* Directory::SystemTemp() {
+  PathBuffer path;
   const char* temp_dir = getenv("TMPDIR");
   if (temp_dir == NULL) {
     temp_dir = getenv("TMP");
@@ -384,23 +402,29 @@ char* Directory::SystemTemp() {
   if (temp_dir == NULL) {
     temp_dir = "/tmp";
   }
-  char* result = strdup(temp_dir);
+  if (!path.Add(temp_dir)) {
+    return NULL;
+  }
+
   // Remove any trailing slash.
+  char* result = path.AsString();
   int length = strlen(result);
-  if (length > 1 && result[length - 1] == '/') {
+  if ((length > 1) && (result[length - 1] == '/')) {
     result[length - 1] = '\0';
   }
-  return result;
+  return path.AsScopedString();
 }
 
 
-char* Directory::CreateTemp(const char* prefix) {
+const char* Directory::CreateTemp(const char* prefix) {
   // Returns a new, unused directory name, adding characters to the end
   // of prefix.  Creates the directory with the permissions specified
   // by the process umask.
-  // The return value must be freed by the caller.
+  // The return value is Dart_ScopeAllocated.
   PathBuffer path;
-  path.Add(prefix);
+  if (!path.Add(prefix)) {
+    return NULL;
+  }
   if (!path.Add("XXXXXX")) {
     // Pattern has overflowed.
     return NULL;
@@ -408,18 +432,18 @@ char* Directory::CreateTemp(const char* prefix) {
   char* result;
   do {
     result = mkdtemp(path.AsString());
-  } while (result == NULL && errno == EINTR);
+  } while ((result == NULL) && (errno == EINTR));
   if (result == NULL) {
     return NULL;
   }
-  return strdup(result);
+  return path.AsScopedString();
 }
 
 
 bool Directory::Delete(const char* dir_name, bool recursive) {
   if (!recursive) {
-    if (File::GetType(dir_name, false) == File::kIsLink &&
-        File::GetType(dir_name, true) == File::kIsDirectory) {
+    if ((File::GetType(dir_name, false) == File::kIsLink) &&
+        (File::GetType(dir_name, true) == File::kIsDirectory)) {
       return NO_RETRY_EXPECTED(unlink(dir_name)) == 0;
     }
     return NO_RETRY_EXPECTED(rmdir(dir_name)) == 0;
@@ -435,7 +459,9 @@ bool Directory::Delete(const char* dir_name, bool recursive) {
 
 bool Directory::Rename(const char* path, const char* new_path) {
   ExistsResult exists = Exists(path);
-  if (exists != EXISTS) return false;
+  if (exists != EXISTS) {
+    return false;
+  }
   return NO_RETRY_EXPECTED(rename(path, new_path)) == 0;
 }