commit 3196be61fcf0e0303cf819630876b9c812b9474e
author Paul Lawrence <paullawrence@google.com> Tue Jul 11 09:05:11 2023 -0700
committer Paul Lawrence <paullawrence@google.com> Tue Jul 11 13:57:19 2023 -0700
tree a256b7f8b68e78db7635abb186572311d0bb87c8
parent c742bfda6b290e849308005e04160feaf99afc7d

Revert "Fix deadlock caused by two-threaded property controls"

This reverts commit 606afc7b7451aba90e3634076d9b59a5ef08186b.

These fixes for b/262208935 introduced a race condition. We believe the
race is fixed by ag/23879563, but at this point in the release feel that
reverting the fixes and refixing in main is the better solution

Test: Builds, boots
Bug: 283202477
Bug: 288991737
Ignore-AOSP-First: Reverting CL only in internal
Change-Id: I9ae6863b0ea5e064c59d9d34c03d33fa1da12fdc

init/property_service.cpp

diff --git a/init/property_service.cpp b/init/property_service.cpp
index 4242912..2d084db 100644
--- a/init/property_service.cpp
+++ b/init/property_service.cpp
@@ -117,6 +117,7 @@
 static std::mutex accept_messages_lock;
 static std::thread property_service_thread;
 static std::thread property_service_for_system_thread;
+static std::mutex set_property_lock;
 
 static std::unique_ptr<PersistWriteThread> persist_write_thread;
 
@@ -394,37 +395,32 @@
         return {PROP_ERROR_INVALID_VALUE};
     }
 
-    if (name == "sys.powerctl") {
-        // No action here - NotifyPropertyChange will trigger the appropriate action, and since this
-        // can come to the second thread, we mustn't call out to the __system_property_* functions
-        // which support multiple readers but only one mutator.
+    auto lock = std::lock_guard{set_property_lock};
+    prop_info* pi = (prop_info*)__system_property_find(name.c_str());
+    if (pi != nullptr) {
+        // ro.* properties are actually "write-once".
+        if (StartsWith(name, "ro.")) {
+            *error = "Read-only property was already set";
+            return {PROP_ERROR_READ_ONLY_PROPERTY};
+        }
+
+        __system_property_update(pi, value.c_str(), valuelen);
     } else {
-        prop_info* pi = (prop_info*)__system_property_find(name.c_str());
-        if (pi != nullptr) {
-            // ro.* properties are actually "write-once".
-            if (StartsWith(name, "ro.")) {
-                *error = "Read-only property was already set";
-                return {PROP_ERROR_READ_ONLY_PROPERTY};
-            }
-
-            __system_property_update(pi, value.c_str(), valuelen);
-        } else {
-            int rc = __system_property_add(name.c_str(), name.size(), value.c_str(), valuelen);
-            if (rc < 0) {
-                *error = "__system_property_add failed";
-                return {PROP_ERROR_SET_FAILED};
-            }
+        int rc = __system_property_add(name.c_str(), name.size(), value.c_str(), valuelen);
+        if (rc < 0) {
+            *error = "__system_property_add failed";
+            return {PROP_ERROR_SET_FAILED};
         }
+    }
 
-        // Don't write properties to disk until after we have read all default
-        // properties to prevent them from being overwritten by default values.
-        if (socket && persistent_properties_loaded && StartsWith(name, "persist.")) {
-            if (persist_write_thread) {
-                persist_write_thread->Write(name, value, std::move(*socket));
-                return {};
-            }
-            WritePersistentProperty(name, value);
+    // Don't write properties to disk until after we have read all default
+    // properties to prevent them from being overwritten by default values.
+    if (socket && persistent_properties_loaded && StartsWith(name, "persist.")) {
+        if (persist_write_thread) {
+            persist_write_thread->Write(name, value, std::move(*socket));
+            return {};
         }
+        WritePersistentProperty(name, value);
     }
 
     NotifyPropertyChange(name, value);