Reject additinal KEY_DELIVERY calls
Bug: b:198377119
Change-Id: I0b03115cfafa3989f4ef5bcf8660643e716d3c64
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3163792
Reviewed-by: Zach Trudo <zatrudo@google.com>
Commit-Queue: Zach Trudo <zatrudo@google.com>
Commit-Queue: Leonid Baraz <lbaraz@chromium.org>
Auto-Submit: Leonid Baraz <lbaraz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#922229}
diff --git a/components/reporting/storage/storage.cc b/components/reporting/storage/storage.cc
index c66d3bf..ace5361 100644
--- a/components/reporting/storage/storage.cc
+++ b/components/reporting/storage/storage.cc
@@ -414,9 +414,8 @@
directory_,
/*recursive=*/false, base::FileEnumerator::FILES,
base::StrCat({kEncryptionKeyFilePrefix, FILE_PATH_LITERAL("*")}));
-
- for (base::FilePath full_name = dir_enum.Next(); !full_name.empty();
- full_name = dir_enum.Next()) {
+ base::FilePath full_name;
+ while (full_name = dir_enum.Next(), !full_name.empty()) {
const auto result = key_files_to_remove.emplace(full_name);
if (!result.second) {
// Duplicate file name. Should not happen.
@@ -441,8 +440,8 @@
key_files_to_remove.erase(result.first);
}
// Delete all files assigned for deletion.
- for (const auto& full_name : key_files_to_remove) {
- base::DeleteFile(full_name); // Ignore errors, if any.
+ for (const auto& file_to_remove : key_files_to_remove) {
+ base::DeleteFile(file_to_remove); // Ignore errors, if any.
}
}
diff --git a/components/reporting/storage/storage_queue.cc b/components/reporting/storage/storage_queue.cc
index 32100d6..ea664613 100644
--- a/components/reporting/storage/storage_queue.cc
+++ b/components/reporting/storage/storage_queue.cc
@@ -241,6 +241,43 @@
return last_record_digest_;
}
+Status StorageQueue::SetGenerationId(const base::FilePath& full_name) {
+ // Data file should have generation id as an extension too.
+ // For backwards compatibility we allow it to not be included.
+ // TODO(b/195786943): Encapsulate file naming assumptions in objects.
+ const auto generation_extension =
+ full_name.RemoveFinalExtension().FinalExtension();
+ if (generation_extension.empty()) {
+ // Backwards compatibility case - extension is absent.
+ return Status::StatusOK();
+ }
+
+ int64_t file_generation_id = 0;
+ const bool success =
+ base::StringToInt64(generation_extension.substr(1), &file_generation_id);
+ if (!success || file_generation_id <= 0) {
+ return Status(error::DATA_LOSS,
+ base::StrCat({"Data file generation corrupt: '",
+ full_name.MaybeAsASCII()}));
+ }
+
+ // Found valid generation [1, int64_max] in the data file name.
+ if (generation_id_ > 0) {
+ // Generation was already set, data file must match.
+ if (file_generation_id != generation_id_) {
+ return Status(error::DATA_LOSS,
+ base::StrCat({"Data file generation does not match: '",
+ full_name.MaybeAsASCII(), "', expected=",
+ base::NumberToString(generation_id_)}));
+ }
+ } else {
+ // No generation set in the queue. Use the one from this file and expect
+ // all other files to match.
+ generation_id_ = file_generation_id;
+ }
+ return Status::StatusOK();
+}
+
StatusOr<int64_t> StorageQueue::AddDataFile(
const base::FilePath& full_name,
const base::FileEnumerator::FileInfo& file_info) {
@@ -251,38 +288,16 @@
full_name.MaybeAsASCII(), "'"}));
}
int64_t file_sequencing_id = 0;
- bool success = base::StringToInt64(extension.substr(1), &file_sequencing_id);
+ const bool success =
+ base::StringToInt64(extension.substr(1), &file_sequencing_id);
if (!success) {
return Status(error::INTERNAL,
base::StrCat({"File extension does not parse: '",
full_name.MaybeAsASCII(), "'"}));
}
- // Data file should have generation id as an extension too.
- // For backwards compatibility we allow it to not be included.
- // TODO(b/195786943): Encapsulate file naming assumptions in objects.
- const auto generation_extension =
- full_name.RemoveFinalExtension().FinalExtension();
- if (!generation_extension.empty()) {
- int64_t file_generation_id = 0;
- success = base::StringToInt64(generation_extension.substr(1),
- &file_generation_id);
- if (success && file_generation_id > 0) {
- // Found valid generation [1, int64_max] in the data file name.
- if (generation_id_ > 0) {
- // Generation was already set, data file must match.
- if (file_generation_id != generation_id_) {
- return Status(error::DATA_LOSS,
- base::StrCat({"Data file generation does not match: '",
- full_name.MaybeAsASCII(), "', expected=",
- base::NumberToString(generation_id_)}));
- }
- } else {
- // No generation set in the queue. Use the one from this file and expect
- // all other files to match.
- generation_id_ = file_generation_id;
- }
- }
- }
+
+ RETURN_IF_ERROR(SetGenerationId(full_name));
+
auto file_or_status = SingleFile::Create(full_name, file_info.GetSize());
if (!file_or_status.ok()) {
return file_or_status.status();
diff --git a/components/reporting/storage/storage_queue.h b/components/reporting/storage/storage_queue.h
index 0f57b1f..98ee016 100644
--- a/components/reporting/storage/storage_queue.h
+++ b/components/reporting/storage/storage_queue.h
@@ -219,6 +219,10 @@
const base::FilePath& full_name,
const base::FileEnumerator::FileInfo& file_info);
+ // Helper method for Init(): sets generation id based on data file name.
+ // For backwards compatibility, accepts file name without generation too.
+ Status SetGenerationId(const base::FilePath& full_name);
+
// Helper method for Init(): enumerates all data files in the directory.
// Valid file names are <prefix>.<sequencing_id>, any other names are ignored.
// Adds used data files to the set.
diff --git a/components/reporting/storage/storage_queue_unittest.cc b/components/reporting/storage/storage_queue_unittest.cc
index 56eaf024..c56ab13 100644
--- a/components/reporting/storage/storage_queue_unittest.cc
+++ b/components/reporting/storage/storage_queue_unittest.cc
@@ -555,6 +555,7 @@
StorageQueueTest* self) {
const auto status = self->set_mock_uploader_expectations_.Call(
reason, uploader.get());
+ LOG(ERROR) << "Upload reason=" << reason << " " << status;
if (!status.ok()) {
std::move(start_uploader_cb).Run(status);
return;
diff --git a/components/reporting/storage/storage_unittest.cc b/components/reporting/storage/storage_unittest.cc
index 01f48626..bfd331d 100644
--- a/components/reporting/storage/storage_unittest.cc
+++ b/components/reporting/storage/storage_unittest.cc
@@ -818,6 +818,7 @@
StorageTest* self) {
const auto status = self->set_mock_uploader_expectations_.Call(
reason, uploader.get());
+ LOG(ERROR) << "Upload reason=" << reason << " " << status;
if (!status.ok()) {
std::move(start_uploader_cb).Run(status);
return;
@@ -1011,9 +1012,14 @@
test::TestCallbackAutoWaiter waiter;
EXPECT_CALL(set_mock_uploader_expectations_,
Call(Eq(UploaderInterface::KEY_DELIVERY), NotNull()))
- .WillRepeatedly(WithArg<1>(Invoke([](TestUploader* test_uploader) {
+ // Called once with empty queue.
+ .WillOnce(WithArg<1>(Invoke([](TestUploader* test_uploader) {
TestUploader::SetEmpty uploader(test_uploader);
return Status::StatusOK();
+ })))
+ // Can be called later again, reject it.
+ .WillRepeatedly(WithArg<1>(Invoke([](TestUploader* test_uploader) {
+ return Status(error::CANCELLED, "Repeated key delivery rejected");
})));
EXPECT_CALL(set_mock_uploader_expectations_,
Call(Eq(UploaderInterface::MANUAL), NotNull()))
