commit 9f98658fcd4164d131d67085b88ae6ae767095b3
author Zach Bush <zmbush@google.com> Wed Oct 14 17:37:25 2020 +0000
committer CQ Bot <commit-bot@chromium.org> Wed Oct 14 17:37:25 2020 +0000
tree a9fd4bf03d03601a8770657e6efe6b8cfa1e01d5
parent 6b6b6b7d510b339a8796ce9801884c3eb3d1b406

[MetadataBuilder] Protect SystemProfileHistory

This value may be modified/read by multiple threads. This change wraps
the SystemProfileHistory in a util::ProtectedFields.

Fixed: 61775
Change-Id: I6c81b494284d929a6518fb946cd69854599ebde1
Reviewed-on: https://fuchsia-review.googlesource.com/c/cobalt/+/438303
Reviewed-by: Steve Fung <stevefung@google.com>
Commit-Queue: Zach Bush <zmbush@google.com>

src/pb/BUILD.gn

diff --git a/src/pb/BUILD.gn b/src/pb/BUILD.gn
index e9b6c1f..b0cbaa2 100644
--- a/src/pb/BUILD.gn
+++ b/src/pb/BUILD.gn
@@ -53,6 +53,7 @@
     "$cobalt_root/src/lib/util:consistent_proto_store",
     "$cobalt_root/src/lib/util:datetime_util",
     "$cobalt_root/src/lib/util:file_system",
+    "$cobalt_root/src/lib/util:protected_fields",
     "$cobalt_root/src/logger:project_context",
     "$cobalt_root/src/system_data",
   ]

src/pb/metadata_builder.cc

diff --git a/src/pb/metadata_builder.cc b/src/pb/metadata_builder.cc
index fec8adc..9e7b3e7 100644
--- a/src/pb/metadata_builder.cc
+++ b/src/pb/metadata_builder.cc
@@ -35,7 +35,7 @@
   if (!system_profile_cache_dir.empty()) {
     history_store_ =
         std::make_unique<util::ConsistentProtoStore>(std::move(system_profile_cache_dir), fs);
-    history_store_->Read(&system_profile_history_);
+    history_store_->Read(&*system_profile_history_.lock());
   } else {
     LOG(WARNING) << "No system_profile_cache_dir provided to MetadataBuilder. The system profile "
                     "cache will not be persisted to disk";
@@ -151,17 +151,17 @@
           selected_day_index -= report.local_aggregation_period() - 1;
           break;
       }
-      profile = FindProfileForSelectFirst(system_profile_history_, report.report_type(),
-                                          metric.metric_definition()->time_zone_policy(),
-                                          selected_day_index, hour_id);
+      profile = FindProfileForSelectFirst(
+          *system_profile_history_.const_lock(), report.report_type(),
+          metric.metric_definition()->time_zone_policy(), selected_day_index, hour_id);
 
     } break;
     case SystemProfileSelectionPolicy::SELECT_LAST:
     case SystemProfileSelectionPolicy::SELECT_DEFAULT:
     default:
-      profile = FindProfileForSelectLast(system_profile_history_, report.report_type(),
-                                         metric.metric_definition()->time_zone_policy(), day_index,
-                                         hour_id);
+      profile = FindProfileForSelectLast(
+          *system_profile_history_.const_lock(), report.report_type(),
+          metric.metric_definition()->time_zone_policy(), day_index, hour_id);
       break;
   }
 
@@ -225,15 +225,15 @@
     return;
   }
 
-  if (system_profile_history_.snapshots_size() > 0 &&
-      system_profile_history_.snapshots().rbegin()->SerializeAsString() ==
-          system_data_->system_profile().SerializeAsString()) {
+  auto history = system_profile_history_.lock();
+  if (history->snapshots_size() > 0 && history->snapshots().rbegin()->SerializeAsString() ==
+                                           system_data_->system_profile().SerializeAsString()) {
     // The most recent snapshot is the same as the current system_profile. No need to snapshot
     // again.
     return;
   }
 
-  SystemProfileSnapshot* snapshot = system_profile_history_.add_snapshots();
+  SystemProfileSnapshot* snapshot = history->add_snapshots();
   snapshot->mutable_system_profile()->CopyFrom(system_data_->system_profile());
 
   util::TimeInfo utc = util::TimeInfo::FromTimePoint(*now, MetricDefinition::UTC);
@@ -247,12 +247,13 @@
 
   snapshot->set_timestamp(util::ToUnixSeconds(*now));
 
-  WriteHistory();
+  WriteHistory(std::move(history));
 }
 
-void MetadataBuilder::WriteHistory() {
+void MetadataBuilder::WriteHistory(
+    util::ProtectedFields<SystemProfileHistory>::LockedFieldsPtr history) {
   if (history_store_) {
-    history_store_->Write(system_profile_history_);
+    history_store_->Write(*history);
   }
 }
 
@@ -267,15 +268,16 @@
   uint32_t delete_before_timestamp = util::ToUnixSeconds(delete_before);
 
   google::protobuf::RepeatedPtrField<SystemProfileSnapshot> kept_snapshots;
-  for (auto& snapshot : *system_profile_history_.mutable_snapshots()) {
+  auto history = system_profile_history_.lock();
+  for (auto& snapshot : *history->mutable_snapshots()) {
     if (snapshot.timestamp() >= delete_before_timestamp) {
       kept_snapshots.Add()->Swap(&snapshot);
     }
   }
 
-  system_profile_history_.mutable_snapshots()->Swap(&kept_snapshots);
+  history->mutable_snapshots()->Swap(&kept_snapshots);
 
-  WriteHistory();
+  WriteHistory(std::move(history));
 }
 
 }  // namespace cobalt

src/pb/metadata_builder.h

diff --git a/src/pb/metadata_builder.h b/src/pb/metadata_builder.h
index 472086e..cb7adae 100644
--- a/src/pb/metadata_builder.h
+++ b/src/pb/metadata_builder.h
@@ -7,6 +7,7 @@
 
 #include "src/lib/util/clock.h"
 #include "src/lib/util/consistent_proto_store.h"
+#include "src/lib/util/protected_fields.h"
 #include "src/logger/project_context.h"
 #include "src/pb/observation_batch.pb.h"
 #include "src/pb/system_data_history.pb.h"
@@ -40,9 +41,9 @@
   static void FilteredSystemProfile(const ReportDefinition &report, const SystemProfile &profile_in,
                                     SystemProfile *profile_out);
 
-  void WriteHistory();
+  void WriteHistory(util::ProtectedFields<SystemProfileHistory>::LockedFieldsPtr history);
 
-  SystemProfileHistory system_profile_history_;
+  util::ProtectedFields<SystemProfileHistory> system_profile_history_;
 
   system_data::SystemDataInterface *system_data_;
   std::unique_ptr<util::AlwaysAccurateClock> owned_clock_;