Copy only accessed PersistentHistogramData fields when validating.

base/metrics/persistent_histogram_allocator.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 "base/metrics/persistent_histogram_allocator.h"
 
 #include <memory>
 
 #include "base/atomicops.h"
 #include "base/files/file_path.h"
@@ skipping 550 matching lines @@
                                           &histogram_data_ptr->logged_metadata);
     DCHECK(histogram);
     histogram->SetFlags(histogram_data_ptr->flags);
     RecordCreateHistogramResult(CREATE_HISTOGRAM_SUCCESS);
     return histogram;
   }
 
   // Copy the histogram_data to local storage because anything in persistent
   // memory cannot be trusted as it could be changed at any moment by a
   // malicious actor that shares access. The contents of histogram_data are
-  // validated below; the local copy is to ensure that the contents cannot
-  // be externally changed between validation and use.
-  PersistentHistogramData histogram_data = *histogram_data_ptr;
+  // validated below; the local copy of relevant fields is to ensure that the
+  // contents cannot be externally changed between validation and use. A direct
+  // copy isn't possible because of the counts_ref field that must be read
+  // using an atomic operation.
+  PersistentHistogramData histogram_data;

[Alexei Svitkine (slow), 2017/05/11 20:20:07]

Can you add a CopyTo() function instead? This way, someone changing the struct
can be sure to update it instead of the code living elsewhere (here).

[bcwhite, 2017/05/11 20:23:16]

I looked at that path...  but it was going to get complicated because the
metadata structures should also use atomic copies and they're not needed for
this code.

Perhaps I should remove the local PHD instance and create individual variables
instead?

[Alexei Svitkine (slow), 2017/05/11 20:27:55]

That SGTM.

[bcwhite, 2017/05/11 21:03:29]

Done.

+  histogram_data.histogram_type = histogram_data_ptr->histogram_type;
+  histogram_data.flags = histogram_data_ptr->flags;
+  histogram_data.minimum = histogram_data_ptr->minimum;
+  histogram_data.maximum = histogram_data_ptr->maximum;
+  histogram_data.bucket_count = histogram_data_ptr->bucket_count;
+  histogram_data.ranges_ref = histogram_data_ptr->ranges_ref;
+  histogram_data.ranges_checksum = histogram_data_ptr->ranges_checksum;
 
   HistogramBase::Sample* ranges_data =
       memory_allocator_->GetAsArray<HistogramBase::Sample>(
           histogram_data.ranges_ref, kTypeIdRangesArray,
           PersistentMemoryAllocator::kSizeAny);
 
   const uint32_t max_buckets =
       std::numeric_limits<uint32_t>::max() / sizeof(HistogramBase::Sample);
   size_t required_bytes =
       (histogram_data.bucket_count + 1) * sizeof(HistogramBase::Sample);
@@ skipping 15 matching lines @@
     NOTREACHED();
     return nullptr;
   }
   const BucketRanges* ranges =
       StatisticsRecorder::RegisterOrDeleteDuplicateRanges(
           created_ranges.release());
 
   size_t counts_bytes =
       CalculateRequiredCountsBytes(histogram_data.bucket_count);
   PersistentMemoryAllocator::Reference counts_ref =
-      subtle::NoBarrier_Load(&histogram_data.counts_ref);
+      subtle::NoBarrier_Load(&histogram_data_ptr->counts_ref);
   if (counts_bytes == 0 ||
       (counts_ref != 0 &&
        memory_allocator_->GetAllocSize(counts_ref) < counts_bytes)) {
     RecordCreateHistogramResult(CREATE_HISTOGRAM_INVALID_COUNTS_ARRAY);
     NOTREACHED();
     return nullptr;
   }
 
   // The "counts" data (including both samples and logged samples) is a delayed
   // persistent allocation meaning that though its size and storage for a
@@ skipping 340 matching lines @@
   while (true) {
     std::unique_ptr<HistogramBase> histogram =
         import_iterator_.GetNextWithIgnore(record_to_ignore);
     if (!histogram)
       break;
     StatisticsRecorder::RegisterOrDeleteDuplicate(histogram.release());
   }
 }
 
 }  // namespace base