update_engine: Don't sent <ping a=-1 r=-1> once the device was powerwashed.
When a device is first activated, a <ping a=-1 r=-1> is sent to Omaha
indicating this is a new device activation. After that point, the
active days count will be positive since Omaha sends back the daystart
tag which will be used in the future to compute the non-negative
"a=" and "r=" values.
Nevertheless, when a device is powerwashed, the daystart data is lost
and the first login will trigger a "new device" activation, counting
the same device as two activations in the Omaha side.
This patch uses the powerwash_count file stored in stateful partition
to detect if the new device activation is performed on a device that
was powerwashed at some point and thus doesn't send the a=-1 and r=-1
ping to Omaha. The powerwash_count is generated or incremented
whenever a "safe" powerwash is performed (such as the one that
update_engine triggers on some channel changes). Going to dev mode
and back, going through recovery with an USB key or doing a
"factory" powerwash (done in the factory) *will* wipe this powerwash
count and send the device back to a factory state.
BUG=chromium:428792
TEST=Manual test.
Manual test procude:
1. Run recovery. Check /mnt/stateful_partition/unencrypted/preserve/powerwash_count
is not present.
2. Login for the first time.
3. Check /var/log/update_engine.log shows a '<ping' with 'a="-1" r="-1"' being sent.
4. Powerwash ( echo "safe fast keepimg" > /mnt/stateful_partition/factory_install_reset )
5. Reboot
6. Login for the first time again.
7. Check /var/log/update_engine.log shows doesn't show a '<ping' with 'a="-1" r="-1"'
Change-Id: I1a823040d2da43b93f14241bc521087ce2a2d4f2
Reviewed-on: https://chromium-review.googlesource.com/226716
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
Tested-by: Alex Deymo <deymo@chromium.org>
diff --git a/fake_hardware.h b/fake_hardware.h
index ff80009..13c3341 100644
--- a/fake_hardware.h
+++ b/fake_hardware.h
@@ -18,6 +18,11 @@
// Implements a fake hardware interface used for testing.
class FakeHardware : public HardwareInterface {
public:
+ // Value used to signal that the powerwash_count file is not present. When
+ // this value is used in SetPowerwashCount(), GetPowerwashCount() will return
+ // false.
+ static const int kPowerwashCountNotSet = -1;
+
FakeHardware()
: kernel_device_("/dev/sdz4"),
boot_device_("/dev/sdz5"),
@@ -28,7 +33,8 @@
is_oobe_complete_(false),
hardware_class_("Fake HWID BLAH-1234"),
firmware_version_("Fake Firmware v1.0.1"),
- ec_version_("Fake EC v1.0a") {}
+ ec_version_("Fake EC v1.0a"),
+ powerwash_count_(kPowerwashCountNotSet) {}
// HardwareInterface methods.
std::string BootKernelDevice() const override { return kernel_device_; }
@@ -71,6 +77,8 @@
std::string GetECVersion() const override { return ec_version_; }
+ int GetPowerwashCount() const override { return powerwash_count_; }
+
// Setters
void SetBootDevice(const std::string& boot_device) {
boot_device_ = boot_device;
@@ -111,6 +119,10 @@
ec_version_ = ec_version;
}
+ void SetPowerwashCount(int powerwash_count) {
+ powerwash_count_ = powerwash_count;
+ }
+
private:
std::string kernel_device_;
std::string boot_device_;
@@ -124,6 +136,7 @@
std::string hardware_class_;
std::string firmware_version_;
std::string ec_version_;
+ int powerwash_count_;
DISALLOW_COPY_AND_ASSIGN(FakeHardware);
};
diff --git a/hardware.cc b/hardware.cc
index 8fbd90e..fd774ca 100644
--- a/hardware.cc
+++ b/hardware.cc
@@ -6,6 +6,7 @@
#include <base/files/file_util.h>
#include <base/logging.h>
+#include <base/strings/string_number_conversions.h>
#include <base/strings/string_util.h>
#include <rootdev/rootdev.h>
#include <vboot/crossystem.h>
@@ -25,6 +26,12 @@
static const char kOOBECompletedMarker[] = "/home/chronos/.oobe_completed";
+// The powerwash_count marker file contains the number of times the device was
+// powerwashed. This value is incremented by the clobber-state script when
+// a powerwash is performed.
+static const char kPowerwashCountMarker[] =
+ "/mnt/stateful_partition/unencrypted/preserve/powerwash_count";
+
} // namespace
namespace chromeos_update_engine {
@@ -204,4 +211,15 @@
return utils::ParseECVersion(input_line);
}
+int Hardware::GetPowerwashCount() const {
+ int powerwash_count;
+ string contents;
+ if (!utils::ReadFile(kPowerwashCountMarker, &contents))
+ return -1;
+ base::TrimWhitespaceASCII(contents, base::TRIM_TRAILING, &contents);
+ if (!base::StringToInt(contents, &powerwash_count))
+ return -1;
+ return powerwash_count;
+}
+
} // namespace chromeos_update_engine
diff --git a/hardware.h b/hardware.h
index 82b94e4..c573117 100644
--- a/hardware.h
+++ b/hardware.h
@@ -34,6 +34,7 @@
std::string GetHardwareClass() const override;
std::string GetFirmwareVersion() const override;
std::string GetECVersion() const override;
+ int GetPowerwashCount() const override;
private:
DISALLOW_COPY_AND_ASSIGN(Hardware);
diff --git a/hardware_interface.h b/hardware_interface.h
index 8248553..2c9c100 100644
--- a/hardware_interface.h
+++ b/hardware_interface.h
@@ -66,6 +66,11 @@
// running a custom chrome os ec.
virtual std::string GetECVersion() const = 0;
+ // Returns the powerwash_count from the stateful. If the file is not found
+ // or is invalid, returns -1. Brand new machines out of the factory or after
+ // recovery don't have this value set.
+ virtual int GetPowerwashCount() const = 0;
+
virtual ~HardwareInterface() {}
};
diff --git a/mock_hardware.h b/mock_hardware.h
index 1c2d4a7..325b81a 100644
--- a/mock_hardware.h
+++ b/mock_hardware.h
@@ -55,6 +55,9 @@
ON_CALL(*this, GetECVersion())
.WillByDefault(testing::Invoke(&fake_,
&FakeHardware::GetECVersion));
+ ON_CALL(*this, GetPowerwashCount())
+ .WillByDefault(testing::Invoke(&fake_,
+ &FakeHardware::GetPowerwashCount));
}
virtual ~MockHardware() {}
@@ -74,6 +77,7 @@
MOCK_CONST_METHOD0(GetHardwareClass, std::string());
MOCK_CONST_METHOD0(GetFirmwareVersion, std::string());
MOCK_CONST_METHOD0(GetECVersion, std::string());
+ MOCK_CONST_METHOD0(GetPowerwashCount, int());
// Returns a reference to the underlying FakeHardware.
FakeHardware& fake() {
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index e4b8a7b..fad9ad1 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -63,19 +63,12 @@
static const char* const kGupdateVersion = "ChromeOSUpdateEngine-0.1.0.0";
-// Returns true if |ping_days| has a value that needs to be sent,
-// false otherwise.
-bool ShouldPing(int ping_days) {
- return ping_days > 0 || ping_days == OmahaRequestAction::kNeverPinged;
-}
-
// Returns an XML ping element attribute assignment with attribute
// |name| and value |ping_days| if |ping_days| has a value that needs
// to be sent, or an empty string otherwise.
string GetPingAttribute(const string& name, int ping_days) {
- if (ShouldPing(ping_days)) {
+ if (ping_days > 0 || ping_days == OmahaRequestAction::kNeverPinged)
return base::StringPrintf(" %s=\"%d\"", name.c_str(), ping_days);
- }
return "";
}
@@ -86,8 +79,8 @@
string ping_roll_call = GetPingAttribute("r", ping_roll_call_days);
if (!ping_active.empty() || !ping_roll_call.empty()) {
return base::StringPrintf(" <ping active=\"1\"%s%s></ping>\n",
- ping_active.c_str(),
- ping_roll_call.c_str());
+ ping_active.c_str(),
+ ping_roll_call.c_str());
}
return "";
}
@@ -97,12 +90,14 @@
string GetAppBody(const OmahaEvent* event,
OmahaRequestParams* params,
bool ping_only,
+ bool include_ping,
int ping_active_days,
int ping_roll_call_days,
PrefsInterface* prefs) {
string app_body;
if (event == nullptr) {
- app_body = GetPingXml(ping_active_days, ping_roll_call_days);
+ if (include_ping)
+ app_body = GetPingXml(ping_active_days, ping_roll_call_days);
if (!ping_only) {
app_body += base::StringPrintf(
" <updatecheck targetversionprefix=\"%s\""
@@ -151,12 +146,14 @@
string GetAppXml(const OmahaEvent* event,
OmahaRequestParams* params,
bool ping_only,
+ bool include_ping,
int ping_active_days,
int ping_roll_call_days,
int install_date_in_days,
SystemState* system_state) {
- string app_body = GetAppBody(event, params, ping_only, ping_active_days,
- ping_roll_call_days, system_state->prefs());
+ string app_body = GetAppBody(event, params, ping_only, include_ping,
+ ping_active_days, ping_roll_call_days,
+ system_state->prefs());
string app_versions;
// If we are upgrading to a more stable channel and we are allowed to do
@@ -221,14 +218,15 @@
string GetRequestXml(const OmahaEvent* event,
OmahaRequestParams* params,
bool ping_only,
+ bool include_ping,
int ping_active_days,
int ping_roll_call_days,
int install_date_in_days,
SystemState* system_state) {
string os_xml = GetOsXml(params);
- string app_xml = GetAppXml(event, params, ping_only, ping_active_days,
- ping_roll_call_days, install_date_in_days,
- system_state);
+ string app_xml = GetAppXml(event, params, ping_only, include_ping,
+ ping_active_days, ping_roll_call_days,
+ install_date_in_days, system_state);
string install_source = base::StringPrintf("installsource=\"%s\" ",
(params->interactive() ? "ondemandupdate" : "scheduler"));
@@ -415,11 +413,25 @@
}
// TODO(petkov): Figure a way to distinguish active use pings
// vs. roll call pings. Currently, the two pings are identical. A
- // fix needs to change this code as well as UpdateLastPingDays.
+ // fix needs to change this code as well as UpdateLastPingDays and ShouldPing.
ping_active_days_ = CalculatePingDays(kPrefsLastActivePingDay);
ping_roll_call_days_ = CalculatePingDays(kPrefsLastRollCallPingDay);
}
+bool OmahaRequestAction::ShouldPing() const {
+ if (ping_active_days_ == OmahaRequestAction::kNeverPinged &&
+ ping_roll_call_days_ == OmahaRequestAction::kNeverPinged) {
+ int powerwash_count = system_state_->hardware()->GetPowerwashCount();
+ if (powerwash_count > 0) {
+ LOG(INFO) << "Not sending ping with a=-1 r=-1 to omaha because "
+ << "powerwash_count is " << powerwash_count;
+ return false;
+ }
+ return true;
+ }
+ return ping_active_days_ > 0 || ping_roll_call_days_ > 0;
+}
+
// static
int OmahaRequestAction::GetInstallDate(SystemState* system_state) {
PrefsInterface* prefs = system_state->prefs();
@@ -487,9 +499,7 @@
void OmahaRequestAction::PerformAction() {
http_fetcher_->set_delegate(this);
InitPingDays();
- if (ping_only_ &&
- !ShouldPing(ping_active_days_) &&
- !ShouldPing(ping_roll_call_days_)) {
+ if (ping_only_ && !ShouldPing()) {
processor_->ActionComplete(this, ErrorCode::kSuccess);
return;
}
@@ -497,6 +507,7 @@
string request_post(GetRequestXml(event_.get(),
params_,
ping_only_,
+ ShouldPing(), // include_ping
ping_active_days_,
ping_roll_call_days_,
GetInstallDate(system_state_),
@@ -800,15 +811,11 @@
return;
}
- // If a ping was sent, update the last ping day preferences based on
- // the server daystart response.
- if (ShouldPing(ping_active_days_) ||
- ShouldPing(ping_roll_call_days_) ||
- ping_active_days_ == kPingTimeJump ||
- ping_roll_call_days_ == kPingTimeJump) {
- LOG_IF(ERROR, !UpdateLastPingDays(&parser_data, system_state_->prefs()))
- << "Failed to update the last ping day preferences!";
- }
+ // Update the last ping day preferences based on the server daystart response
+ // even if we didn't send a ping. Omaha always includes the daystart in the
+ // response, but log the error if it didn't.
+ LOG_IF(ERROR, !UpdateLastPingDays(&parser_data, system_state_->prefs()))
+ << "Failed to update the last ping day preferences!";
if (!HasOutputPipe()) {
// Just set success to whether or not the http transfer succeeded,
diff --git a/omaha_request_action.h b/omaha_request_action.h
index 6e4384a..0b93109 100644
--- a/omaha_request_action.h
+++ b/omaha_request_action.h
@@ -196,6 +196,10 @@
// number of days since the last ping sent for |key|.
int CalculatePingDays(const std::string& key);
+ // Returns whether we have "active_days" or "roll_call_days" ping values to
+ // send to Omaha and thus we should include them in the response.
+ bool ShouldPing() const;
+
// Returns true if the download of a new update should be deferred.
// False if the update can be downloaded.
bool ShouldDeferDownload(OmahaResponse* output_object);
diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc
index a4d964d..872b83b 100644
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -1458,8 +1458,12 @@
.WillOnce(DoAll(SetArgumentPointee<1>(one_hour_ago), Return(true)));
EXPECT_CALL(prefs, GetInt64(kPrefsLastRollCallPingDay, _))
.WillOnce(DoAll(SetArgumentPointee<1>(one_hour_ago), Return(true)));
- EXPECT_CALL(prefs, SetInt64(kPrefsLastActivePingDay, _)).Times(0);
- EXPECT_CALL(prefs, SetInt64(kPrefsLastRollCallPingDay, _)).Times(0);
+ // LastActivePingDay and PrefsLastRollCallPingDay are set even if we didn't
+ // send a ping.
+ EXPECT_CALL(prefs, SetInt64(kPrefsLastActivePingDay, _))
+ .WillOnce(Return(true));
+ EXPECT_CALL(prefs, SetInt64(kPrefsLastRollCallPingDay, _))
+ .WillOnce(Return(true));
vector<char> post_data;
ASSERT_TRUE(
TestUpdateCheck(nullptr, // request_params
@@ -1882,6 +1886,31 @@
EXPECT_EQ(string::npos, post_str.find("from_version"));
}
+// Checks that the initial ping with a=-1 r=-1 is not send when the device
+// was powerwashed.
+TEST_F(OmahaRequestActionTest, PingWhenPowerwashed) {
+ fake_prefs_.SetString(kPrefsPreviousVersion, "");
+
+ // Flag that the device was powerwashed in the past.
+ fake_system_state_.fake_hardware()->SetPowerwashCount(1);
+
+ vector<char> post_data;
+ ASSERT_TRUE(
+ TestUpdateCheck(nullptr, // request_params
+ GetNoUpdateResponse(OmahaRequestParams::kAppId),
+ -1,
+ false, // ping_only
+ ErrorCode::kSuccess,
+ metrics::CheckResult::kNoUpdateAvailable,
+ metrics::CheckReaction::kUnset,
+ metrics::DownloadErrorCode::kUnset,
+ nullptr,
+ &post_data));
+ // We shouldn't send a ping in this case since powerwash > 0.
+ string post_str(&post_data[0], post_data.size());
+ EXPECT_EQ(string::npos, post_str.find("<ping"));
+}
+
void OmahaRequestActionTest::P2PTest(
bool initial_allow_p2p_for_downloading,
bool initial_allow_p2p_for_sharing,