commit 76583910d6685e6e75c6edaa3f17795e5d85405f
author Jae Hoon Kim <kimjae@chromium.org> Tue Jun 23 17:24:03 2020
committer Commit Bot <commit-bot@chromium.org> Thu Jun 25 22:47:20 2020
tree ed3c456ff294199a50f077cf5b97b39ba8a35930
parent f4d1196edca97a666e20d16a334aa67f47bbcf0a

update_engine: Skip post DownloadAction exclusions

Post |DownloadAction| don't have direct reference to |Payload|s held
within the |PayloadState|. Hence it's required to halt exclusions for
|Action|s post |DownloadAction|.

This is done by setting the |payload_index_| within |PayloadState| >= to
the |candidate_urls_|/|response_.packages| size.

DCHECKs added where |payload_index_| is used as usage may cause out of
bounds indexing.

This change removes the dangling reference to the last |Payload|, as
previously |NextPayload()| kept |payload_index_| pointing to the last
|Payload| within |PayloadState|.

BUG=chromium:928805
TEST=FEATURES=test emerge-$B update_engine

Change-Id: I3f6a9a3cc26bb84f94506e45e1d6e906624e5dd7
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2261292
Tested-by: Jae Hoon Kim <kimjae@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
Commit-Queue: Amin Hassani <ahassani@chromium.org>
Auto-Submit: Jae Hoon Kim <kimjae@chromium.org>

payload_state.cc

diff --git a/payload_state.cc b/payload_state.cc
index 4b4120c..d626056 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -462,6 +462,7 @@
 }
 
 void PayloadState::IncrementFullPayloadAttemptNumber() {
+  DCHECK(payload_index_ < response_.packages.size());
   // Update the payload attempt number for full payloads and the backoff time.
   if (response_.packages[payload_index_].is_delta) {
     LOG(INFO) << "Not incrementing payload attempt number for delta payloads";
@@ -474,6 +475,7 @@
 }
 
 void PayloadState::IncrementUrlIndex() {
+  DCHECK(payload_index_ < candidate_urls_.size());
   size_t next_url_index = url_index_ + 1;
   size_t max_url_size = candidate_urls_[payload_index_].size();
   if (next_url_index < max_url_size) {
@@ -510,6 +512,10 @@
 }
 
 void PayloadState::ExcludeCurrentPayload() {
+  if (payload_index_ >= response_.packages.size()) {
+    LOG(INFO) << "Skipping exclusion of the current payload.";
+    return;
+  }
   const auto& package = response_.packages[payload_index_];
   if (!package.can_exclude) {
     LOG(INFO) << "Not excluding as marked non-excludable for package hash="
@@ -923,10 +929,12 @@
 }
 
 bool PayloadState::NextPayload() {
-  if (payload_index_ + 1 >= candidate_urls_.size())
+  if (payload_index_ >= candidate_urls_.size())
+    return false;
+  SetPayloadIndex(payload_index_ + 1);
+  if (payload_index_ >= candidate_urls_.size())
     return false;
   SetUrlIndex(0);
-  SetPayloadIndex(payload_index_ + 1);
   return true;
 }
 

payload_state.h

diff --git a/payload_state.h b/payload_state.h
index d13c642..5713a54 100644
--- a/payload_state.h
+++ b/payload_state.h
@@ -161,6 +161,8 @@
   FRIEND_TEST(PayloadStateTest, ExcludeNoopForNonExcludables);
   FRIEND_TEST(PayloadStateTest, ExcludeOnlyCanExcludables);
   FRIEND_TEST(PayloadStateTest, IncrementFailureExclusionTest);
+  FRIEND_TEST(PayloadStateTest, HaltExclusionPostPayloadExhaustion);
+  FRIEND_TEST(PayloadStateTest, NonInfinitePayloadIndexIncrement);
 
   // Helper called when an attempt has begun, is called by
   // UpdateResumed(), UpdateRestarted() and Rollback().

payload_state_unittest.cc

diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index bf9aed4..0bf52d9 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -1778,4 +1778,49 @@
   payload_state.IncrementFailureCount();
 }
 
+TEST(PayloadStateTest, HaltExclusionPostPayloadExhaustion) {
+  PayloadState payload_state;
+  FakeSystemState fake_system_state;
+  StrictMock<MockExcluder> mock_excluder;
+  EXPECT_CALL(*fake_system_state.mock_update_attempter(), GetExcluder())
+      .WillOnce(Return(&mock_excluder));
+  EXPECT_TRUE(payload_state.Initialize(&fake_system_state));
+
+  OmahaResponse response;
+  // Non-critical package.
+  response.packages.push_back(
+      {.payload_urls = {"http://test1a", "http://test2a"},
+       .size = 123456789,
+       .metadata_size = 58123,
+       .metadata_signature = "msign",
+       .hash = "hash",
+       .can_exclude = true});
+  payload_state.SetResponse(response);
+
+  // Exclusion should be called when excluded.
+  EXPECT_CALL(mock_excluder, Exclude(utils::GetExclusionName("http://test1a")))
+      .WillOnce(Return(true));
+  payload_state.ExcludeCurrentPayload();
+
+  // No more paylods to go through.
+  EXPECT_FALSE(payload_state.NextPayload());
+
+  // Exclusion should not be called as all |Payload|s are exhausted.
+  payload_state.ExcludeCurrentPayload();
+}
+
+TEST(PayloadStateTest, NonInfinitePayloadIndexIncrement) {
+  PayloadState payload_state;
+  FakeSystemState fake_system_state;
+  EXPECT_TRUE(payload_state.Initialize(&fake_system_state));
+
+  payload_state.SetResponse({});
+
+  EXPECT_FALSE(payload_state.NextPayload());
+  int payload_index = payload_state.payload_index_;
+
+  EXPECT_FALSE(payload_state.NextPayload());
+  EXPECT_EQ(payload_index, payload_state.payload_index_);
+}
+
 }  // namespace chromeos_update_engine