Fix potential use-after-free bug in reboot
Instead of operating on raw pointers, init now uses name of the
services as it's primary identifier. Only place that still uses
vector<Service*> is StopServices.
In addition, ServiceList::services() function is removed, which should
help avoiding similar bugs in the future.
Bug: 170315126
Bug: 174335499
Test: adb reboot
Test: atest CtsInitTestCases
Change-Id: I73ecd7a8c58c2ec3732934c595b7f7db814b7034
Merged-In: I73ecd7a8c58c2ec3732934c595b7f7db814b7034
Ignore-AOSP-First: fixing security vulnerability
(cherry picked from commit 8d6ae2dd8aebfd98e5b0968062b2f0c92fa0b0da)
diff --git a/init/reboot.cpp b/init/reboot.cpp
index 49baf9e..409f8cb 100644
--- a/init/reboot.cpp
+++ b/init/reboot.cpp
@@ -85,12 +85,11 @@
static const std::set<std::string> kDebuggingServices{"tombstoned", "logd", "adbd", "console"};
-static std::vector<Service*> GetDebuggingServices(bool only_post_data) {
- std::vector<Service*> ret;
- ret.reserve(kDebuggingServices.size());
+static std::set<std::string> GetPostDataDebuggingServices() {
+ std::set<std::string> ret;
for (const auto& s : ServiceList::GetInstance()) {
- if (kDebuggingServices.count(s->name()) && (!only_post_data || s->is_post_data())) {
- ret.push_back(s.get());
+ if (kDebuggingServices.count(s->name()) && s->is_post_data()) {
+ ret.insert(s->name());
}
}
return ret;
@@ -503,13 +502,18 @@
// Stops given services, waits for them to be stopped for |timeout| ms.
// If terminate is true, then SIGTERM is sent to services, otherwise SIGKILL is sent.
-static void StopServices(const std::vector<Service*>& services, std::chrono::milliseconds timeout,
+// Note that services are stopped in order given by |ServiceList::services_in_shutdown_order|
+// function.
+static void StopServices(const std::set<std::string>& services, std::chrono::milliseconds timeout,
bool terminate) {
LOG(INFO) << "Stopping " << services.size() << " services by sending "
<< (terminate ? "SIGTERM" : "SIGKILL");
std::vector<pid_t> pids;
pids.reserve(services.size());
- for (const auto& s : services) {
+ for (const auto& s : ServiceList::GetInstance().services_in_shutdown_order()) {
+ if (services.count(s->name()) == 0) {
+ continue;
+ }
if (s->pid() > 0) {
pids.push_back(s->pid());
}
@@ -529,12 +533,12 @@
// Like StopServices, but also logs all the services that failed to stop after the provided timeout.
// Returns number of violators.
-static int StopServicesAndLogViolations(const std::vector<Service*>& services,
+static int StopServicesAndLogViolations(const std::set<std::string>& services,
std::chrono::milliseconds timeout, bool terminate) {
StopServices(services, timeout, terminate);
int still_running = 0;
- for (const auto& s : services) {
- if (s->IsRunning()) {
+ for (const auto& s : ServiceList::GetInstance()) {
+ if (s->IsRunning() && services.count(s->name())) {
LOG(ERROR) << "[service-misbehaving] : service '" << s->name() << "' is still running "
<< timeout.count() << "ms after receiving "
<< (terminate ? "SIGTERM" : "SIGKILL");
@@ -620,8 +624,7 @@
// watchdogd is a vendor specific component but should be alive to complete shutdown safely.
const std::set<std::string> to_starts{"watchdogd"};
- std::vector<Service*> stop_first;
- stop_first.reserve(ServiceList::GetInstance().services().size());
+ std::set<std::string> stop_first;
for (const auto& s : ServiceList::GetInstance()) {
if (kDebuggingServices.count(s->name())) {
// keep debugging tools until non critical ones are all gone.
@@ -639,7 +642,7 @@
<< "': " << result.error();
}
} else {
- stop_first.push_back(s.get());
+ stop_first.insert(s->name());
}
}
@@ -702,7 +705,7 @@
LOG(INFO) << "vold not running, skipping vold shutdown";
}
// logcat stopped here
- StopServices(GetDebuggingServices(false /* only_post_data */), 0ms, false /* SIGKILL */);
+ StopServices(kDebuggingServices, 0ms, false /* SIGKILL */);
// 4. sync, try umount, and optionally run fsck for user shutdown
{
Timer sync_timer;
@@ -784,17 +787,17 @@
sub_reason = "resetprop";
return Error() << "Failed to reset sys.powerctl property";
}
- std::vector<Service*> stop_first;
+ std::set<std::string> stop_first;
// Remember the services that were enabled. We will need to manually enable them again otherwise
// triggers like class_start won't restart them.
- std::vector<Service*> were_enabled;
- stop_first.reserve(ServiceList::GetInstance().services().size());
+ std::set<std::string> were_enabled;
for (const auto& s : ServiceList::GetInstance().services_in_shutdown_order()) {
if (s->is_post_data() && !kDebuggingServices.count(s->name())) {
- stop_first.push_back(s);
+ stop_first.insert(s->name());
}
+ // TODO(ioffe): we should also filter out temporary services here.
if (s->is_post_data() && s->IsEnabled()) {
- were_enabled.push_back(s);
+ were_enabled.insert(s->name());
}
}
{
@@ -814,8 +817,8 @@
r > 0) {
auto fd = unique_fd(TEMP_FAILURE_RETRY(open(services_file_name.c_str(), flags, 0666)));
android::base::WriteStringToFd("Post-data services still running: \n", fd);
- for (const auto& s : stop_first) {
- if (s->IsRunning()) {
+ for (const auto& s : ServiceList::GetInstance()) {
+ if (s->IsRunning() && stop_first.count(s->name())) {
android::base::WriteStringToFd(s->name() + "\n", fd);
}
}
@@ -830,13 +833,14 @@
sub_reason = "vold_reset";
return result;
}
- if (int r = StopServicesAndLogViolations(GetDebuggingServices(true /* only_post_data */),
- sigkill_timeout, false /* SIGKILL */);
+ const auto& debugging_services = GetPostDataDebuggingServices();
+ if (int r = StopServicesAndLogViolations(debugging_services, sigkill_timeout,
+ false /* SIGKILL */);
r > 0) {
auto fd = unique_fd(TEMP_FAILURE_RETRY(open(services_file_name.c_str(), flags, 0666)));
android::base::WriteStringToFd("Debugging services still running: \n", fd);
- for (const auto& s : GetDebuggingServices(true)) {
- if (s->IsRunning()) {
+ for (const auto& s : ServiceList::GetInstance()) {
+ if (s->IsRunning() && debugging_services.count(s->name())) {
android::base::WriteStringToFd(s->name() + "\n", fd);
}
}
@@ -866,9 +870,11 @@
return false;
});
// Re-enable services
- for (const auto& s : were_enabled) {
- LOG(INFO) << "Re-enabling service '" << s->name() << "'";
- s->Enable();
+ for (const auto& s : ServiceList::GetInstance()) {
+ if (were_enabled.count(s->name())) {
+ LOG(INFO) << "Re-enabling service '" << s->name() << "'";
+ s->Enable();
+ }
}
ServiceList::GetInstance().ResetState();
LeaveShutdown();