Clean up property set error handling
Currently we only report why a property set call has failed but drop
the context of what was trying to set the property. This change
adds information about why a property was trying to be set when it
fails.
It also unifies property_set() within init to go through the same
HandlePropertySet() function as normal processes do, removing unneeded
special cases.
Test: boot bullhead
Test: attempt to set invalid properties and see better error messages
Change-Id: I5cd3a40086fd3b226e9c8a5e3a84cb3b31399c0d
diff --git a/init/host_init_stubs.cpp b/init/host_init_stubs.cpp
index cde747d..e6cc08a 100644
--- a/init/host_init_stubs.cpp
+++ b/init/host_init_stubs.cpp
@@ -43,8 +43,8 @@
// property_service.h
uint32_t (*property_set)(const std::string& name, const std::string& value) = nullptr;
-uint32_t HandlePropertySet(const std::string&, const std::string&, const std::string&,
- const ucred&) {
+uint32_t HandlePropertySet(const std::string&, const std::string&, const std::string&, const ucred&,
+ std::string*) {
return 0;
}
diff --git a/init/host_init_stubs.h b/init/host_init_stubs.h
index af96aea..f31ece6 100644
--- a/init/host_init_stubs.h
+++ b/init/host_init_stubs.h
@@ -48,7 +48,7 @@
// property_service.h
extern uint32_t (*property_set)(const std::string& name, const std::string& value);
uint32_t HandlePropertySet(const std::string& name, const std::string& value,
- const std::string& source_context, const ucred& cr);
+ const std::string& source_context, const ucred& cr, std::string* error);
// selinux.h
void SelabelInitialize();
diff --git a/init/property_service.cpp b/init/property_service.cpp
index 338c05a..624780f 100644
--- a/init/property_service.cpp
+++ b/init/property_service.cpp
@@ -117,23 +117,21 @@
return has_access;
}
-static uint32_t PropertySetImpl(const std::string& name, const std::string& value) {
+static uint32_t PropertySet(const std::string& name, const std::string& value, std::string* error) {
size_t valuelen = value.size();
if (!IsLegalPropertyName(name)) {
- LOG(ERROR) << "property_set(\"" << name << "\", \"" << value << "\") failed: bad name";
+ *error = "Illegal property name";
return PROP_ERROR_INVALID_NAME;
}
if (valuelen >= PROP_VALUE_MAX && !StartsWith(name, "ro.")) {
- LOG(ERROR) << "property_set(\"" << name << "\", \"" << value << "\") failed: "
- << "value too long";
+ *error = "Property value too long";
return PROP_ERROR_INVALID_VALUE;
}
if (mbstowcs(nullptr, value.data(), 0) == static_cast<std::size_t>(-1)) {
- LOG(ERROR) << "property_set(\"" << name << "\", \"" << value << "\") failed: "
- << "value not a UTF8 encoded string";
+ *error = "Value is not a UTF8 encoded string";
return PROP_ERROR_INVALID_VALUE;
}
@@ -141,8 +139,7 @@
if (pi != nullptr) {
// ro.* properties are actually "write-once".
if (StartsWith(name, "ro.")) {
- LOG(ERROR) << "property_set(\"" << name << "\", \"" << value << "\") failed: "
- << "property already set";
+ *error = "Read-only property was already set";
return PROP_ERROR_READ_ONLY_PROPERTY;
}
@@ -150,8 +147,7 @@
} else {
int rc = __system_property_add(name.c_str(), name.size(), value.c_str(), valuelen);
if (rc < 0) {
- LOG(ERROR) << "property_set(\"" << name << "\", \"" << value << "\") failed: "
- << "__system_property_add failed";
+ *error = "__system_property_add failed";
return PROP_ERROR_SET_FAILED;
}
}
@@ -205,8 +201,10 @@
if (info.pid != pid) {
return false;
}
- if (PropertySetImpl(info.name, info.value) != PROP_SUCCESS) {
- LOG(ERROR) << "Failed to set async property " << info.name;
+ std::string error;
+ if (PropertySet(info.name, info.value, &error) != PROP_SUCCESS) {
+ LOG(ERROR) << "Failed to set async property " << info.name << " to " << info.value << ": "
+ << error;
}
property_children.pop();
if (!property_children.empty()) {
@@ -216,9 +214,9 @@
}
static uint32_t PropertySetAsync(const std::string& name, const std::string& value,
- PropertyAsyncFunc func) {
+ PropertyAsyncFunc func, std::string* error) {
if (value.empty()) {
- return PropertySetImpl(name, value);
+ return PropertySet(name, value, error);
}
PropertyChildInfo info;
@@ -236,30 +234,27 @@
return selinux_android_restorecon(value.c_str(), SELINUX_ANDROID_RESTORECON_RECURSE);
}
-uint32_t PropertySet(const std::string& name, const std::string& value) {
- if (name == "selinux.restorecon_recursive") {
- return PropertySetAsync(name, value, RestoreconRecursiveAsync);
- }
-
- return PropertySetImpl(name, value);
-}
-
uint32_t InitPropertySet(const std::string& name, const std::string& value) {
if (StartsWith(name, "ctl.")) {
- LOG(ERROR) << "Do not set ctl. properties from init; call the Service functions directly";
+ LOG(ERROR) << "InitPropertySet: Do not set ctl. properties from init; call the Service "
+ "functions directly";
+ return PROP_ERROR_INVALID_NAME;
+ }
+ if (name == "selinux.restorecon_recursive") {
+ LOG(ERROR) << "InitPropertySet: Do not set selinux.restorecon_recursive from init; use the "
+ "restorecon builtin directly";
return PROP_ERROR_INVALID_NAME;
}
- const char* type = nullptr;
- property_info_area->GetPropertyInfo(name.c_str(), nullptr, &type);
-
- if (type == nullptr || !CheckType(type, value)) {
- LOG(ERROR) << "property_set: name: '" << name << "' type check failed, type: '"
- << (type ?: "(null)") << "' value: '" << value << "'";
- return PROP_ERROR_INVALID_VALUE;
+ uint32_t result = 0;
+ ucred cr = {.pid = 1, .uid = 0, .gid = 0};
+ std::string error;
+ result = HandlePropertySet(name, value, kInitContext.c_str(), cr, &error);
+ if (result != PROP_SUCCESS) {
+ LOG(ERROR) << "Init cannot set '" << name << "' to '" << value << "': " << error;
}
- return PropertySet(name, value);
+ return result;
}
class SocketConnection {
@@ -390,9 +385,9 @@
// This returns one of the enum of PROP_SUCCESS or PROP_ERROR*.
uint32_t HandlePropertySet(const std::string& name, const std::string& value,
- const std::string& source_context, const ucred& cr) {
+ const std::string& source_context, const ucred& cr, std::string* error) {
if (!IsLegalPropertyName(name)) {
- LOG(ERROR) << "PropertySet: illegal property name \"" << name << "\"";
+ *error = "Illegal property name";
return PROP_ERROR_INVALID_NAME;
}
@@ -405,9 +400,7 @@
const char* type = nullptr;
property_info_area->GetPropertyInfo(control_string.c_str(), &target_context, &type);
if (!CheckMacPerms(control_string, target_context, source_context.c_str(), cr)) {
- LOG(ERROR) << "PropertySet: Unable to " << (name.c_str() + 4) << " service ctl ["
- << value << "]"
- << " uid:" << cr.uid << " gid:" << cr.gid << " pid:" << cr.pid;
+ *error = StringPrintf("Unable to '%s' service %s", name.c_str() + 4, value.c_str());
return PROP_ERROR_HANDLE_CONTROL_MESSAGE;
}
@@ -420,13 +413,13 @@
property_info_area->GetPropertyInfo(name.c_str(), &target_context, &type);
if (!CheckMacPerms(name, target_context, source_context.c_str(), cr)) {
- LOG(ERROR) << "PropertySet: permission denied uid:" << cr.uid << " name:" << name;
+ *error = "SELinux permission check failed";
return PROP_ERROR_PERMISSION_DENIED;
}
if (type == nullptr || !CheckType(type, value)) {
- LOG(ERROR) << "PropertySet: name: '" << name << "' type check failed, type: '"
- << (type ?: "(null)") << "' value: '" << value << "'";
+ *error = StringPrintf("Property type check failed, value doesn't match expected type '%s'",
+ (type ?: "(null)"));
return PROP_ERROR_INVALID_VALUE;
}
@@ -445,7 +438,11 @@
<< process_log_string;
}
- return PropertySet(name, value);
+ if (name == "selinux.restorecon_recursive") {
+ return PropertySetAsync(name, value, RestoreconRecursiveAsync, error);
+ }
+
+ return PropertySet(name, value, error);
}
static void handle_property_set_fd() {
@@ -488,7 +485,16 @@
prop_name[PROP_NAME_MAX-1] = 0;
prop_value[PROP_VALUE_MAX-1] = 0;
- HandlePropertySet(prop_value, prop_value, socket.source_context(), socket.cred());
+ const auto& cr = socket.cred();
+ std::string error;
+ uint32_t result =
+ HandlePropertySet(prop_name, prop_value, socket.source_context(), cr, &error);
+ if (result != PROP_SUCCESS) {
+ LOG(ERROR) << "Unable to set property '" << prop_name << "' to '" << prop_value
+ << "' from uid:" << cr.uid << " gid:" << cr.gid << " pid:" << cr.pid << ": "
+ << error;
+ }
+
break;
}
@@ -502,7 +508,14 @@
return;
}
- auto result = HandlePropertySet(name, value, socket.source_context(), socket.cred());
+ const auto& cr = socket.cred();
+ std::string error;
+ uint32_t result = HandlePropertySet(name, value, socket.source_context(), cr, &error);
+ if (result != PROP_SUCCESS) {
+ LOG(ERROR) << "Unable to set property '" << name << "' to '" << value
+ << "' from uid:" << cr.uid << " gid:" << cr.gid << " pid:" << cr.pid << ": "
+ << error;
+ }
socket.SendUint32(result);
break;
}
diff --git a/init/property_service.h b/init/property_service.h
index d4973fc..29eaaa9 100644
--- a/init/property_service.h
+++ b/init/property_service.h
@@ -32,7 +32,7 @@
extern uint32_t (*property_set)(const std::string& name, const std::string& value);
uint32_t HandlePropertySet(const std::string& name, const std::string& value,
- const std::string& source_context, const ucred& cr);
+ const std::string& source_context, const ucred& cr, std::string* error);
extern bool PropertyChildReap(pid_t pid);
diff --git a/init/subcontext.cpp b/init/subcontext.cpp
index faab368..762492c 100644
--- a/init/subcontext.cpp
+++ b/init/subcontext.cpp
@@ -297,7 +297,11 @@
for (const auto& property : subcontext_reply->properties_to_set()) {
ucred cr = {.pid = pid_, .uid = 0, .gid = 0};
- HandlePropertySet(property.name(), property.value(), context_, cr);
+ std::string error;
+ if (HandlePropertySet(property.name(), property.value(), context_, cr, &error) != 0) {
+ LOG(ERROR) << "Subcontext init could not set '" << property.name() << "' to '"
+ << property.value() << "': " << error;
+ }
}
if (subcontext_reply->reply_case() == SubcontextReply::kFailure) {