Define the Settings interface for a CrashReportDatabase and provide a Mac implementation.

client/settings.cc

Index: client/settings.cc
diff --git a/client/settings.cc b/client/settings.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ecc83a58d65b995e1fc5c86ce6b8331414dab8cd
--- /dev/null
+++ b/client/settings.cc
@@ -0,0 +1,224 @@
+// Copyright 2015 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "client/settings.h"
+
+#include <fcntl.h>
+#include <unistd.h>
+#include <uuid/uuid.h>
+
+#include "base/logging.h"
+#include "base/posix/eintr_wrapper.h"
+
+namespace crashpad {
+
+namespace {
+
+const uint16_t kSettingsVersion = 1;

[Mark Mentovai, 2015/03/09 19:12:36]

Why not make this a static const inside Settings::Data?

[Robert Sesek, 2015/03/09 21:16:27]

Done.

+
+}  // namespace
+
+struct Settings::Data {
+  Data() : version(kSettingsVersion),
+           client_id(),
+           uploads_enabled(false),
+           last_upload_attempt_time(0) {}
+
+  uint16_t version;

[Mark Mentovai, 2015/03/09 19:12:36]

Revise the struct order for better packing, to ensure that the 32-bit and 64-bit
layouts are identical, and to ensure that unused parts of the struct are always
initialized and don’t contain memory garbage. I would suggest:

  uint32_t version;
  uint32_t options;  // uploads-enabled are a bit in here
  uint64_t last_upload_attempt_time;
  UUID client_id;

Alternative 1: Keep bool uploads_enabled, but stick it in a union with options
and initialize options.

Alternative 2: Use a bit field that gives you uploads_enabled : 1 and reserved :
31.

Otherwise, if you just have the bool by itself, you’ll need to memset the struct
to fill the inaccessible bytes with zeroes in the constructor.

[Robert Sesek, 2015/03/09 21:16:27]

Done.

+  UUID client_id;
+  bool uploads_enabled;
+  time_t last_upload_attempt_time;

[Mark Mentovai, 2015/03/09 19:12:35]

time_t has a variable size in 32-bit and 64-bit environments. This should
explicitly be uint64_t.

[Robert Sesek, 2015/03/09 21:16:27]

Done.

+};
+
+Settings::Settings(const base::FilePath& file_path)
+    : file_path_(file_path) {
+}
+
+Settings::~Settings() {
+}
+
+bool Settings::Initialize() {

[Mark Mentovai, 2015/03/09 19:12:35]

InitializationStateDcheck here and in public entry points to guard against API
misuse?

[Robert Sesek, 2015/03/09 21:16:27]

I think that clutters up Initialize more than it helps. Considering that
construction is not allowed beyond that of the database, API misuse should not
happen.

[Mark Mentovai, 2015/03/09 22:00:57]

Robert Sesek wrote:
Well, it’s optional, but doing it doesn’t just guard that the public methods are
called correctly. It also guards against accidental double-Initialize() and
provides a pretty unambiguous signal of use-after-free problems when the memory
for the object hasn’t yet been reclaimed.

[Robert Sesek, 2015/03/09 22:11:00]

Sure, but it means not being able to return the result of the other calls
directly and results in code like:

bool rv = InitializeSettings(handle.get());
if (rv) {
  INITIALIZATION_STATE_DCHECK_SET_VALID(initialized_);
} else {
  INITIALIZATINO_STATE_DCHECK_SET_INVALID(initialized_);
}
return rv;

[Mark Mentovai, 2015/03/09 22:26:29]

Robert Sesek wrote:
You don’t need to set invalid on failure, just leave it set to initializing.

And, as you pointed out, nothing currently cares if someone forgot to call
Initialize(), although this will produce log spam. An InitializationStateDcheck
will actually guard that the interface is used as expected, in a
non-log-spam-producing way.

+  ScopedFileHandle handle(HANDLE_EINTR(
+      open(file_path(),
+           O_CREAT | O_EXCL | O_WRONLY | O_EXLOCK,
+           0644)));
+
+  // The file was created, so this is a new database that needs to be
+  // initialized with a client ID.
+  if (handle.is_valid()) {
+    return InitializeSettings(handle.get());
+  }
+
+  // The file wasn't created, try opening it for a write operation.

[Mark Mentovai, 2015/03/09 19:12:36]

Since you only do a read in most cases, say in the comment that you’re opening
it for a write so that you have the right handle for RecoverSettings() if that
becomes necessary. Also, trying to open it for a write has a nice property that
if the file is unwritable (permission problem), Initialize() will fail. Whatever
your intent was, say so in the comment, because the happy path appears to only
actually require the handle be open for reading.

[Robert Sesek, 2015/03/09 21:16:27]

Done.

+  handle.reset(OpenForWriting().release());
+  if (!handle.is_valid()) {
+    return false;
+  }
+
+  // Ensure that the file is valid. If it fails to read, re-initialize the
+  // settings.
+  Data settings;
+  if (!ReadSettings(handle.get(), &settings)) {
+    return RecoverSettings(handle.get(), &settings);
+  }
+
+  return true;
+}
+
+bool Settings::GetClientID(UUID* client_id) {
+  Data settings;
+  if (!OpenAndReadSettings(&settings)) {
+    return false;
+  }
+
+  *client_id = settings.client_id;
+  return true;
+}
+
+bool Settings::GetUploadsEnabled(bool* enabled) {
+  Data settings;
+  if (!OpenAndReadSettings(&settings)) {
+    return false;
+  }
+
+  *enabled = settings.uploads_enabled;
+  return true;
+}
+
+bool Settings::SetUploadsEnabled(bool enabled) {
+  ScopedFileHandle handle = OpenForWriting();
+  if (!handle.is_valid())
+    return false;
+
+  Data settings;
+  if (!ReadSettings(handle.get(), &settings)) {
+    if (!RecoverSettings(handle.get(), &settings))
+      return false;
+  }
+
+  settings.uploads_enabled = enabled;
+
+  return WriteSettings(handle.get(), &settings);
+}
+
+bool Settings::GetLastUploadAttemptTime(time_t* time) {
+  Data settings;
+  if (!OpenAndReadSettings(&settings))
+    return false;
+
+  *time = settings.last_upload_attempt_time;
+  return true;
+}
+
+bool Settings::SetLastUploadAttemptTime(time_t time) {
+  ScopedFileHandle handle = OpenForWriting();

[Mark Mentovai, 2015/03/09 19:12:35]

The bulk of this function is identical to SetUploadsEnabled() (and, for that
matter, similar to OpenAndReadSettings()). You should refactor into a common
“ScopedFileHandle OpenForWritingAndReadSettings(Data* out_data);” function.
Perhaps better, modify OpenAndReadSettings() to have a bool argument controlling
whether to open for reading or writing.

[Robert Sesek, 2015/03/09 21:16:27]

Done.

+  if (!handle.is_valid())
+    return false;
+
+  Data settings;
+  if (!ReadSettings(handle.get(), &settings)) {
+    if (!RecoverSettings(handle.get(), &settings))
+      return false;
+  }
+
+  settings.last_upload_attempt_time = time;
+
+  return WriteSettings(handle.get(), &settings);
+}
+
+ScopedFileHandle Settings::OpenForReading() {
+  ScopedFileHandle handle(HANDLE_EINTR(open(file_path(), O_RDONLY | O_SHLOCK)));
+  PLOG_IF(ERROR, !handle.is_valid()) << "open for reading";
+  return handle.Pass();
+}
+
+ScopedFileHandle Settings::OpenForWriting() {
+  ScopedFileHandle handle(HANDLE_EINTR(open(file_path(), O_RDWR | O_EXLOCK)));

[Mark Mentovai, 2015/03/09 19:12:36]

Add O_CREAT so that we can recover when the file gets removed accidentally just
like all of the other things that we can now recover from.

[Robert Sesek, 2015/03/09 21:16:27]

Done.

+  PLOG_IF(ERROR, !handle.is_valid()) << "open for writing";
+  return handle.Pass();
+}
+
+bool Settings::OpenAndReadSettings(Data* out_data) {
+  ScopedFileHandle handle = OpenForReading();
+  if (!handle.is_valid())
+    return false;
+
+  if (ReadSettings(handle.get(), out_data))
+    return true;
+
+  // The settings file is corrupt, so reinitialize it.
+  handle.reset();
+
+  // The settings failed to be read, so re-initialize them.
+  return RecoverSettings(kInvalidFileHandle, out_data);
+}
+
+bool Settings::ReadSettings(FileHandle handle, Data* out_data) {
+  if (LoggingSeekFile(handle, 0, SEEK_SET) != 0)
+    return false;
+
+  if (!LoggingReadFile(handle, out_data, sizeof(*out_data)))
+    return false;
+
+  if (out_data->version != kSettingsVersion) {
+    LOG(ERROR) << "Settings version is not " << kSettingsVersion;
+    return false;
+  }
+
+  return true;
+}
+
+bool Settings::WriteSettings(FileHandle handle, const Data* data) {
+  if (LoggingSeekFile(handle, 0, SEEK_SET) != 0)
+    return false;
+
+  if (HANDLE_EINTR(ftruncate(handle, 0)) != 0) {
+    PLOG(ERROR) << "ftruncate settings file";
+    return false;
+  }
+
+  return LoggingWriteFile(handle, data, sizeof(*data));
+}
+
+bool Settings::RecoverSettings(FileHandle handle, Data* out_data) {
+  LOG(INFO) << "Recovering settings file " << file_path();
+
+  ScopedFileHandle scoped_handle;
+  if (handle == kInvalidFileHandle) {
+    scoped_handle.reset(OpenForWriting().release());

[Mark Mentovai, 2015/03/09 19:12:35]

If you open the handle yourself here, you need to try ReadSettings(), and if it
succeeds, just return true. The exclusive write lock that you acquire here
guarantees that nobody else can touch the file while you hold it, but someone
else could have fixed the settings in between the time that this method’s caller
decided that recovery was required and the time that you acquire the exclusive
lock to actually do the recovery. You wouldn’t want to do recovery twice in that
case.

[Robert Sesek, 2015/03/09 21:16:27]

Done.

+    handle = scoped_handle.get();
+  }
+
+  if (handle == kInvalidFileHandle) {
+    LOG(ERROR) << "Invalid file handle";
+    return false;
+  }
+
+  if (!InitializeSettings(handle))

[Mark Mentovai, 2015/03/09 19:12:35]

You can make InitializeSettings() accept a Data* out-parameter to avoid having
to call ReadSettings() immediately after WriteSettings().

[Robert Sesek, 2015/03/09 21:16:27]

It would be unnecessary to do that for the other caller.

+    return false;
+
+  return ReadSettings(handle, out_data);
+}
+
+bool Settings::InitializeSettings(FileHandle handle) {
+  uuid_t uuid;
+  uuid_generate(uuid);
+
+  Data settings;
+  settings.client_id.InitializeFromBytes(uuid);
+
+  return WriteSettings(handle, &settings);
+}
+
+}  // namespace crashpad