[Chromecast] Remove usage of nonreentrant functions.

chromecast/crash/linux/synchronized_minidump_manager.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromecast/crash/linux/synchronized_minidump_manager.h"
 
-#include <dirent.h>
 #include <errno.h>
-#include <fcntl.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <string.h>
-#include <sys/file.h>
 #include <sys/stat.h>

[bcf, 2016/08/03 02:40:24]

Can usage of stat.h, types,h, unistd.h be removed?

[ameyak, 2016/08/03 18:50:32]

time_t/size_t are required and defined in sys/types.h. I have attempted to
remove time.h functions (time(1), difftime(2)) in lieu of base::Time

sys/stat.h is no longer necessary and has been removed
unistd.h is no longer necessary and has been removed

[bcf, 2016/08/03 20:45:10]

Can you try to remove time_t usage?

[ameyak, 2016/08/04 15:25:30]

Done.

 #include <sys/types.h>
 #include <time.h>
 #include <unistd.h>
 
 #include <utility>
 
 #include "base/files/dir_reader_posix.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
+#include "chromecast/base/file_utils.h"
 #include "chromecast/base/path_utils.h"
 #include "chromecast/base/serializers.h"
 #include "chromecast/crash/linux/dump_info.h"
 
 // if |cond| is false, returns |retval|.
 #define RCHECK(cond, retval) \
   do {                       \
     if (!(cond)) {           \
       return (retval);       \
     }                        \
   } while (0)
 
 namespace chromecast {
 
 namespace {
 
-const mode_t kDirMode = 0770;
-const mode_t kFileMode = 0660;
 const char kLockfileName[] = "lockfile";
 const char kMetadataName[] = "metadata";
 const char kMinidumpsDir[] = "minidumps";
 
 const char kLockfileRatelimitKey[] = "ratelimit";
 const char kLockfileRatelimitPeriodStartKey[] = "period_start";
 const char kLockfileRatelimitPeriodDumpsKey[] = "period_dumps";
 const std::size_t kLockfileNumRatelimitParams = 2;
 
 // Gets the ratelimit parameter dictionary given a deserialized |metadata|.
@@ skipping 14 matching lines @@
 time_t GetRatelimitPeriodStart(base::Value* metadata) {
   time_t result = static_cast<time_t>(-1);
   base::DictionaryValue* ratelimit_params = GetRatelimitParams(metadata);
   RCHECK(ratelimit_params, result);
 
   std::string period_start_str;
   RCHECK(ratelimit_params->GetString(kLockfileRatelimitPeriodStartKey,
                                      &period_start_str),
          result);
 
-  errno = 0;
-  result = static_cast<time_t>(strtoll(period_start_str.c_str(), nullptr, 0));
-  if (errno != 0) {
-    return static_cast<time_t>(-1);
-  }
-
+  base::StringToInt64(period_start_str, &result);

[bcf, 2016/08/03 02:40:23]

Check return value.

[ameyak, 2016/08/03 18:50:32]

Done. Overlooked that result can be changed even on a failure.

   return result;
 }
 
 // Sets the time of the current ratelimit period's start in |metadata| to
 // |period_start|. Returns 0 on success, < 0 on error.
 int SetRatelimitPeriodStart(base::Value* metadata, time_t period_start) {
   DCHECK_GE(period_start, 0);
 
   base::DictionaryValue* ratelimit_params = GetRatelimitParams(metadata);
   RCHECK(ratelimit_params, -1);
@@ skipping 46 matching lines @@
          GetRatelimitPeriodDumps(metadata) >= 0;
 }
 
 }  // namespace
 
 // One day
 const int SynchronizedMinidumpManager::kRatelimitPeriodSeconds = 24 * 3600;
 const int SynchronizedMinidumpManager::kRatelimitPeriodMaxDumps = 100;
 
 SynchronizedMinidumpManager::SynchronizedMinidumpManager()
-    : non_blocking_(false),
-      dump_path_(GetHomePathASCII(kMinidumpsDir)),
+    : dump_path_(GetHomePathASCII(kMinidumpsDir)),
       lockfile_path_(dump_path_.Append(kLockfileName).value()),
       metadata_path_(dump_path_.Append(kMetadataName).value()),
-      lockfile_fd_(-1) {
-}
+      initialized(false) {}

[bcf, 2016/08/03 02:40:24]

How about instead call it "lock_held_"

[ameyak, 2016/08/03 18:50:32]

Done.

 
 SynchronizedMinidumpManager::~SynchronizedMinidumpManager() {
   // Release the lock if held.
   ReleaseLockFile();
 }
 
 // TODO(slan): Move some of this pruning logic to ReleaseLockFile?
 int SynchronizedMinidumpManager::GetNumDumps(bool delete_all_dumps) {
-  DIR* dirp;
-  struct dirent* dptr;
   int num_dumps = 0;
 
-  // folder does not exist
-  dirp = opendir(dump_path_.value().c_str());
-  if (dirp == NULL) {
+  base::DirReaderPosix reader(dump_path_.value().c_str());
+  if (!reader.IsValid()) {
     LOG(ERROR) << "Unable to open directory " << dump_path_.value();
     return 0;
   }
 
-  while ((dptr = readdir(dirp)) != NULL) {
-    struct stat buf;
-    const std::string file_fullname = dump_path_.value() + "/" + dptr->d_name;
-    if (lstat(file_fullname.c_str(), &buf) == -1 || !S_ISREG(buf.st_mode)) {
-      // if we cannot lstat this file, it is probably bad, so skip
-      // if the file is not regular, skip
+  while (reader.Next()) {
+    if (strcmp(reader.name(), ".") == 0 || strcmp(reader.name(), "..") == 0)
       continue;
-    }
 
-    // 'lockfile' and 'metadata' is not counted
-    if (lockfile_path_ != file_fullname && metadata_path_ != file_fullname) {
+    const base::FilePath dump_file(dump_path_.Append(reader.name()));
+    // If file cannot be found, skip.
+    if (!base::PathExists(dump_file))
+      continue;
+
+    // Do not count |lockfile_path_| and |metadata_path_|.
+    if (lockfile_path_ != dump_file && metadata_path_ != dump_file) {
       ++num_dumps;
       if (delete_all_dumps) {
-        LOG(INFO) << "Removing " << dptr->d_name
+        LOG(INFO) << "Removing " << reader.name()
                   << "which was not in the lockfile";
-        if (remove(file_fullname.c_str()) < 0) {
+        if (!base::DeleteFile(dump_file, false)) {
           LOG(INFO) << "remove failed. error " << strerror(errno);
         }
       }
     }
   }
 
-  closedir(dirp);
   return num_dumps;
 }
 
 int SynchronizedMinidumpManager::AcquireLockAndDoWork() {
   int success = -1;
   if (AcquireLockFile() >= 0) {
     success = DoWork();
     ReleaseLockFile();
   }
   return success;
 }
 
 int SynchronizedMinidumpManager::AcquireLockFile() {
-  DCHECK_LT(lockfile_fd_, 0);
+  DCHECK(!initialized);
   // Make the directory for the minidumps if it does not exist.
-  if (mkdir(dump_path_.value().c_str(), kDirMode) < 0 && errno != EEXIST) {
-    LOG(ERROR) << "mkdir for " << dump_path_.value().c_str()
-               << " failed. error = " << strerror(errno);
+  if (!CreateDirectory(dump_path_)) {

[bcf, 2016/08/03 02:40:24]

This uses mode 0700. Can you make sure this works?
https://cs.chromium.org/chromium/src/base/files/file_util_posix.cc?sq=package...

[ameyak, 2016/08/03 18:50:32]

Acknowledged. Seems ok, as discussed.

+    LOG(ERROR) << "mkdir for " << dump_path_.value() << " failed.";
     return -1;
   }
 
   // Open the lockfile. Create it if it does not exist.
-  lockfile_fd_ = open(lockfile_path_.c_str(), O_RDWR | O_CREAT, kFileMode);
+  base::File lockfile(lockfile_path_, base::File::FLAG_OPEN_ALWAYS);
 
   // If opening or creating the lockfile failed, we don't want to proceed
   // with dump writing for fear of exhausting up system resources.
-  if (lockfile_fd_ < 0) {
-    LOG(ERROR) << "open lockfile failed " << lockfile_path_;
+  if (!lockfile.IsValid()) {
+    LOG(ERROR) << "open lockfile failed " << lockfile_path_.value();
     return -1;
   }
 
-  // Acquire the lock on the file. Whether or not we are in non-blocking mode,
-  // flock failure means that we did not acquire it and this method should fail.
-  int operation_mode = non_blocking_ ? (LOCK_EX | LOCK_NB) : LOCK_EX;
-  if (flock(lockfile_fd_, operation_mode) < 0) {
+  if (!LockFile(lockfile_path_)) {
     ReleaseLockFile();
-    LOG(INFO) << "flock lockfile failed, error = " << strerror(errno);
     return -1;
   }
 
+  initialized = true;
+
   // The lockfile is open and locked. Parse it to provide subclasses with a
   // record of all the current dumps.
   if (ParseFiles() < 0) {
     LOG(ERROR) << "Lockfile did not parse correctly. ";
     if (InitializeFiles() < 0 || ParseFiles() < 0) {
       LOG(ERROR) << "Failed to create a new lock file!";
       return -1;
     }
   }
 
   DCHECK(dumps_);
   DCHECK(metadata_);
 
   // We successfully have acquired the lock.
   return 0;
 }
 
 int SynchronizedMinidumpManager::ParseFiles() {
-  DCHECK_GE(lockfile_fd_, 0);
+  DCHECK(initialized);
   DCHECK(!dumps_);
   DCHECK(!metadata_);
 
   std::string lockfile;
-  RCHECK(ReadFileToString(base::FilePath(lockfile_path_), &lockfile), -1);
+  RCHECK(ReadFileToString(lockfile_path_, &lockfile), -1);
 
   std::vector<std::string> lines = base::SplitString(
       lockfile, "\n", base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
 
   std::unique_ptr<base::ListValue> dumps =
       base::WrapUnique(new base::ListValue());
 
   // Validate dumps
   for (const std::string& line : lines) {
     if (line.size() == 0)
       continue;
     std::unique_ptr<base::Value> dump_info = DeserializeFromJson(line);
     DumpInfo info(dump_info.get());
     RCHECK(info.valid(), -1);
     dumps->Append(std::move(dump_info));
   }
 
   std::unique_ptr<base::Value> metadata =
-      DeserializeJsonFromFile(base::FilePath(metadata_path_));
+      DeserializeJsonFromFile(metadata_path_);
   RCHECK(ValidateMetadata(metadata.get()), -1);
 
   dumps_ = std::move(dumps);
   metadata_ = std::move(metadata);
   return 0;
 }
 
 int SynchronizedMinidumpManager::WriteFiles(const base::ListValue* dumps,
                                             const base::Value* metadata) {
   DCHECK(dumps);
   DCHECK(metadata);
   std::string lockfile;
 
   for (const auto& elem : *dumps) {
     std::unique_ptr<std::string> dump_info = SerializeToJson(*elem);
     RCHECK(dump_info, -1);
     lockfile += *dump_info;
     lockfile += "\n";  // Add line seperatators
   }
 
-  if (WriteFile(base::FilePath(lockfile_path_),
-                lockfile.c_str(),
-                lockfile.size()) < 0) {
+  if (WriteFile(lockfile_path_, lockfile.c_str(), lockfile.size()) < 0) {
     return -1;
   }
 
-  return SerializeJsonToFile(base::FilePath(metadata_path_), *metadata) ? 0
-                                                                        : -1;
+  return SerializeJsonToFile(metadata_path_, *metadata) ? 0 : -1;
 }
 
 int SynchronizedMinidumpManager::InitializeFiles() {
   std::unique_ptr<base::DictionaryValue> metadata =
       base::WrapUnique(new base::DictionaryValue());
 
   base::DictionaryValue* ratelimit_fields = new base::DictionaryValue();
   metadata->Set(kLockfileRatelimitKey, base::WrapUnique(ratelimit_fields));
   ratelimit_fields->SetString(kLockfileRatelimitPeriodStartKey, "0");
   ratelimit_fields->SetInteger(kLockfileRatelimitPeriodDumpsKey, 0);
 
   std::unique_ptr<base::ListValue> dumps =
       base::WrapUnique(new base::ListValue());
 
   return WriteFiles(dumps.get(), metadata.get());
 }
 
 int SynchronizedMinidumpManager::AddEntryToLockFile(const DumpInfo& dump_info) {
-  DCHECK_LE(0, lockfile_fd_);
+  DCHECK(initialized);
   DCHECK(dumps_);
 
   // Make sure dump_info is valid.
   if (!dump_info.valid()) {
     LOG(ERROR) << "Entry to be added is invalid";
     return -1;
   }
 
   dumps_->Append(dump_info.GetAsValue());
 
   return 0;
 }
 
 int SynchronizedMinidumpManager::RemoveEntryFromLockFile(int index) {
   return dumps_->Remove(static_cast<size_t>(index), nullptr) ? 0 : -1;
 }
 
 void SynchronizedMinidumpManager::ReleaseLockFile() {
   // flock is associated with the fd entry in the open fd table, so closing
   // all fd's will release the lock. To be safe, we explicitly unlock.
-  if (lockfile_fd_ >= 0) {
+  if (initialized) {
     if (dumps_)
       WriteFiles(dumps_.get(), metadata_.get());
 
-    flock(lockfile_fd_, LOCK_UN);
-    close(lockfile_fd_);
-
-    // We may use this object again, so we should reset this.
-    lockfile_fd_ = -1;
+    chromecast::UnlockFile(lockfile_path_);
+    initialized = false;
   }
 
   dumps_.reset();
   metadata_.reset();
 }
 
 ScopedVector<DumpInfo> SynchronizedMinidumpManager::GetDumps() {
   ScopedVector<DumpInfo> dumps;
 
   for (const auto& elem : *dumps_) {
@@ skipping 39 matching lines @@
     SetRatelimitPeriodStart(metadata_.get(), period_start);
     SetRatelimitPeriodDumps(metadata_.get(), period_dumps_count);
   }
 
   return period_dumps_count < kRatelimitPeriodMaxDumps;
 }
 
 bool SynchronizedMinidumpManager::HasDumps() {
   // Check if lockfile has entries.
   int64_t size = 0;
-  if (GetFileSize(base::FilePath(lockfile_path_), &size) && size > 0)
+  if (base::GetFileSize(lockfile_path_, &size) && size > 0)
     return true;
 
   // Check if any files are in minidump directory
   base::DirReaderPosix reader(dump_path_.value().c_str());
   if (!reader.IsValid()) {
     DLOG(FATAL) << "Could not open minidump dir: " << dump_path_.value();
     return false;
   }
 
   while (reader.Next()) {
     if (strcmp(reader.name(), ".") == 0 || strcmp(reader.name(), "..") == 0)
       continue;
 
-    const std::string file_path = dump_path_.Append(reader.name()).value();
+    const base::FilePath file_path = dump_path_.Append(reader.name());
     if (file_path != lockfile_path_ && file_path != metadata_path_)
       return true;
   }
 
   return false;
 }
 
 }  // namespace chromecast