[remoting host] Only watch for material changes to NSS DB files

remoting/host/linux/certificate_watcher.cc

 // Copyright 2016 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 "remoting/host/linux/certificate_watcher.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/files/file_util.h"
+#include "base/hash.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/thread_task_runner_handle.h"
 
 namespace remoting {
 
+namespace {
+
 // Delay time to restart the host when a change of certificate is detected.
 // This is to avoid repeating restarts when continuous writes to the database
 // occur.
 const int kRestartDelayInSecond = 2;
 
 // Full Path: $HOME/.pki/nssdb
 const char kCertDirectoryPath[] = ".pki/nssdb";
 
+const char* const kCertFiles[] = {"cert9.db", "key4.db", "pkcs11.txt"};

[Sergey Ulanov, 2016/05/13 22:03:51]

Declare it as const char kCertFiles[][]

[Lambros, 2016/05/14 01:27:52]

This line fails to compile:
const char kCertFiles[][] = {"cert9.db", "key4.db", "pkcs11.txt"};
../../remoting/host/linux/certificate_watcher.cc:26:22: error: array has
incomplete element type 'const char []'

I brought this question up a few years ago, see:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/abtbE...

const char* const kFoo[] = {...}
seems to be the preferred way.

+
+}  // namespace
+
+// This class lives on the IO thread, watches the certificate database files,
+// and notifies the caller_task_runner thread of any updates.
+class CertificateWatcherImpl {

[Sergey Ulanov, 2016/05/13 22:03:51]

It's confusing to have CertificateWatcherImpl while CertificateWatcher is not an
interface. rename this, e.g. CertDbContentWatcher

[Lambros, 2016/05/14 01:27:52]

Done.

+ public:
+  CertificateWatcherImpl(
+      base::WeakPtr<CertificateWatcher> watcher,
+      scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner,
+      base::FilePath cert_watch_path);
+  ~CertificateWatcherImpl();
+
+  void StartWatching();
+
+ private:
+  // size_t is the return type of base::HashInts() which is used to accumulate
+  // a hash-code while iterating over the database files.
+  typedef size_t HashType;

[Sergey Ulanov, 2016/05/13 22:03:51]

Call this HashValue? We normally don't use Type suffix for types and this name
is confusing (at first I thought it's used to indicate type of a hash function)

[Lambros, 2016/05/14 01:27:52]

Done.

+
+  // Called by the FileWatcher when it detects any changes.
+  void OnCertDirectoryChanged(const base::FilePath& path,
+                              bool error);
+
+  // Reads the certificate database files and returns a hash of their contents.
+  HashType ComputeHash();
+
+  base::WeakPtr<CertificateWatcher> watcher_;
+
+  // The TaskRunner to be notified of any changes.
+  scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner_;
+
+  // The file watcher to watch changes inside the certificate folder.
+  std::unique_ptr<base::FilePathWatcher> file_watcher_;
+
+  // Path of the certificate files/directories.
+  base::FilePath cert_watch_path_;
+
+  // The hash code of the current certificate database contents. When the
+  // FileWatcher detects changes, the code is re-computed and compared with
+  // this stored value.
+  HashType current_hash_;
+
+  DISALLOW_COPY_AND_ASSIGN(CertificateWatcherImpl);
+};
+
+CertificateWatcherImpl::CertificateWatcherImpl(
+    base::WeakPtr<CertificateWatcher> watcher,
+    scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner,
+    base::FilePath cert_watch_path)
+    : watcher_(watcher),
+      caller_task_runner_(caller_task_runner),
+      cert_watch_path_(cert_watch_path) {
+}
+
+CertificateWatcherImpl::~CertificateWatcherImpl() {}
+
+void CertificateWatcherImpl::StartWatching() {
+  DCHECK(!cert_watch_path_.empty());
+
+  file_watcher_.reset(new base::FilePathWatcher());
+
+  // Initialize hash value.
+  current_hash_ = ComputeHash();
+
+  // base::Unretained() is safe since this class owns the FileWatcher.
+  file_watcher_->Watch(
+      cert_watch_path_,
+      true,

[Sergey Ulanov, 2016/05/13 22:03:51]

this can fit on the previous line. clang-format please (git cl format)

[Lambros, 2016/05/14 01:27:52]

Done.

+      base::Bind(&CertificateWatcherImpl::OnCertDirectoryChanged,
+                 base::Unretained(this)));
+}
+
+void CertificateWatcherImpl::OnCertDirectoryChanged(const base::FilePath& path,
+                                                    bool error) {
+  HashType new_hash = ComputeHash();

[Sergey Ulanov, 2016/05/13 22:03:51]

whenever we get notification that the directory has changed we need to wait
before we actually read the data, in case the process that updates cert
databases in several steps and we get multiple notification.
If a process changes the DB and then reverts it back quickly it shouldn't cause
the host to be restarted.

Move restart_timer_ to this class?

[Lambros, 2016/05/14 01:27:51]

Done.

+  if (new_hash != current_hash_) {
+    current_hash_ = new_hash;
+    caller_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&CertificateWatcher::DatabaseChanged, watcher_, path,
+                   error));
+  } else {
+    VLOG(1) << "Directory changed but contents are the same.";
+  }
+}
+
+CertificateWatcherImpl::HashType CertificateWatcherImpl::ComputeHash() {
+  HashType result = 0;
+
+  for (const char* file : kCertFiles) {
+    base::FilePath path = cert_watch_path_.AppendASCII(file);
+    std::string content;
+    HashType file_hash = 0;
+
+    // It's possible the file might not exist. Compute the overall hash in a
+    // consistent way for the set of files that do exist. If a new file comes
+    // into existence, the resulting hash-code should change.
+    if (base::ReadFileToString(path, &content)) {
+      file_hash = base::Hash(content);
+    }
+    result = base::HashInts(result, file_hash);
+  }
+  return result;
+}
+
 CertificateWatcher::CertificateWatcher(
     const base::Closure& restart_action,
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
     : restart_action_(restart_action),
       caller_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       io_task_runner_(io_task_runner),
       delay_(base::TimeDelta::FromSeconds(kRestartDelayInSecond)),
       weak_factory_(this) {
   if (!base::PathService::Get(base::DIR_HOME, &cert_watch_path_)) {
     LOG(FATAL) << "Failed to get path of the home directory.";
   }
   cert_watch_path_ = cert_watch_path_.AppendASCII(kCertDirectoryPath);
 }
 
 CertificateWatcher::~CertificateWatcher() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
   if (!is_started()) {
     return;
   }
   if (monitor_) {
     monitor_->RemoveStatusObserver(this);
   }
-  io_task_runner_->DeleteSoon(FROM_HERE, file_watcher_.release());
+  io_task_runner_->DeleteSoon(FROM_HERE, watcher_impl_.release());
 
   VLOG(1) << "Stopped watching certificate changes.";
 }
 
 void CertificateWatcher::Start() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
   DCHECK(!cert_watch_path_.empty());
 
-  file_watcher_.reset(new base::FilePathWatcher());
+  watcher_impl_.reset(new CertificateWatcherImpl(weak_factory_.GetWeakPtr(),
+                                                 caller_task_runner_,
+                                                 cert_watch_path_));
+
   io_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(base::IgnoreResult(&base::FilePathWatcher::Watch),
-                 base::Unretained(file_watcher_.get()), cert_watch_path_, true,
-                 base::Bind(&CertificateWatcher::OnCertDirectoryChanged,
-                            caller_task_runner_, weak_factory_.GetWeakPtr())));
+      base::Bind(&CertificateWatcherImpl::StartWatching,
+                 base::Unretained(watcher_impl_.get())));
+
   restart_timer_.reset(new base::DelayTimer(FROM_HERE, delay_, this,
                                             &CertificateWatcher::OnTimer));
 
   VLOG(1) << "Started watching certificate changes.";
 }
 
 void CertificateWatcher::SetMonitor(base::WeakPtr<HostStatusMonitor> monitor) {
   DCHECK(is_started());
   if (monitor_) {
     monitor_->RemoveStatusObserver(this);
@@ skipping 23 matching lines @@
   delay_ = delay;
 }
 
 void CertificateWatcher::SetWatchPathForTests(
     const base::FilePath& watch_path) {
   DCHECK(!is_started());
   cert_watch_path_ = watch_path;
 }
 
 bool CertificateWatcher::is_started() const {
-  return file_watcher_ != nullptr;
+  return watcher_impl_ != nullptr;
 }
 
-// static
-void CertificateWatcher::OnCertDirectoryChanged(
-    scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
-    base::WeakPtr<CertificateWatcher> watcher_,
-    const base::FilePath& path,
-    bool error) {
-  network_task_runner->PostTask(
-      FROM_HERE,
-      base::Bind(&CertificateWatcher::DirectoryChanged, watcher_, path, error));
-}
-
-void CertificateWatcher::DirectoryChanged(const base::FilePath& path,
-                                          bool error) {
+void CertificateWatcher::DatabaseChanged(const base::FilePath& path,
+                                         bool error) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
   DCHECK(path == cert_watch_path_);
 
   if (error) {
-    LOG(FATAL) << "Error occurs when watching changes of file "
+    LOG(FATAL) << "Error occurred while watching for changes of file: "
                << cert_watch_path_.MaybeAsASCII();
   }
 
   restart_timer_->Reset();
 }
 
 void CertificateWatcher::OnTimer() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
   if (inhibit_mode_) {
     restart_pending_ = true;
     return;
   }
 
   VLOG(1) << "Certificate was updated. Calling restart...";
   restart_action_.Run();
 }
 
 }  // namespace remoting