Merge "Allow read-only system properties to have arbitrary lengths"
diff --git a/libc/bionic/system_properties.cpp b/libc/bionic/system_properties.cpp
index b781ea3..b87d7e8 100644
--- a/libc/bionic/system_properties.cpp
+++ b/libc/bionic/system_properties.cpp
@@ -71,9 +71,27 @@
#define SERIAL_DIRTY(serial) ((serial)&1)
#define SERIAL_VALUE_LEN(serial) ((serial) >> 24)
+constexpr static const char kLongLegacyError[] = "Must use __system_property_read_callback() to read";
+
+// The error message fits in part of a union with the previous 92 char property value so there must
+// be room left over after the error message for the offset to the new longer property value and
+// future expansion fields if needed.
+// Note that this value cannot ever increase. The offset to the new longer property value appears
+// immediately after it, so an increase of this size will break compatibility.
+constexpr size_t kLongLegacyErrorBufferSize = 56;
+static_assert(sizeof(kLongLegacyError) < kLongLegacyErrorBufferSize,
+ "Error message for long properties read by legacy libc must fit within 56 chars");
+
static const char property_service_socket[] = "/dev/socket/" PROP_SERVICE_NAME;
static const char* kServiceVersionPropertyName = "ro.property_service.version";
+// The C11 standard doesn't allow atomic loads from const fields,
+// though C++11 does. Fudge it until standards get straightened out.
+static inline uint_least32_t load_const_atomic(const atomic_uint_least32_t* s, memory_order mo) {
+ atomic_uint_least32_t* non_const_s = const_cast<atomic_uint_least32_t*>(s);
+ return atomic_load_explicit(non_const_s, mo);
+}
+
/*
* Properties are stored in a hybrid trie/binary tree structure.
* Each property's name is delimited at '.' characters, and the tokens are put
@@ -182,12 +200,34 @@
};
struct prop_info {
+ // Read only properties will not set anything but the bottom most bit of serial and the top byte.
+ // We borrow the 2nd from the top byte for extra flags, and use the bottom most bit of that for
+ // our first user, kLongFlag.
+ constexpr static uint32_t kLongFlag = 1 << 16;
atomic_uint_least32_t serial;
// we need to keep this buffer around because the property
// value can be modified whereas name is constant.
- char value[PROP_VALUE_MAX];
+ union {
+ char value[PROP_VALUE_MAX];
+ struct {
+ char error_message[kLongLegacyErrorBufferSize];
+ uint32_t offset;
+ } long_property;
+ };
char name[0];
+ bool is_long() const {
+ return (load_const_atomic(&serial, memory_order_relaxed) & kLongFlag) != 0;
+ }
+
+ const char* long_value() const {
+ // We can't store pointers here since this is shared memory that will have different absolute
+ // pointers in different processes. We don't have data_ from prop_area, but since we know
+ // `this` is data_ + some offset and long_value is data_ + some other offset, we calculate the
+ // offset from `this` to long_value and store it as long_property.offset.
+ return reinterpret_cast<const char*>(this) + long_property.offset;
+ }
+
prop_info(const char* name, uint32_t namelen, const char* value, uint32_t valuelen) {
memcpy(this->name, name, namelen);
this->name[namelen] = '\0';
@@ -196,10 +236,23 @@
this->value[valuelen] = '\0';
}
+ prop_info(const char* name, uint32_t namelen, uint32_t long_offset) {
+ memcpy(this->name, name, namelen);
+ this->name[namelen] = '\0';
+
+ auto error_value_len = sizeof(kLongLegacyError) - 1;
+ atomic_init(&this->serial, error_value_len << 24 | kLongFlag);
+ memcpy(this->long_property.error_message, kLongLegacyError, sizeof(kLongLegacyError));
+
+ this->long_property.offset = long_offset;
+ }
+
private:
DISALLOW_IMPLICIT_CONSTRUCTORS(prop_info);
};
+static_assert(sizeof(prop_info) == 96, "size of struct prop_info must be 96 bytes");
+
// This is public because it was exposed in the NDK. As of 2017-01, ~60 apps reference this symbol.
prop_area* __system_property_area__ = nullptr;
@@ -330,13 +383,28 @@
uint32_t valuelen, uint_least32_t* const off) {
uint_least32_t new_offset;
void* const p = allocate_obj(sizeof(prop_info) + namelen + 1, &new_offset);
- if (p != nullptr) {
- prop_info* info = new (p) prop_info(name, namelen, value, valuelen);
- *off = new_offset;
- return info;
- }
+ if (p == nullptr) return nullptr;
- return nullptr;
+ prop_info* info;
+ if (valuelen >= PROP_VALUE_MAX) {
+ uint32_t long_value_offset = 0;
+ char* long_location = reinterpret_cast<char*>(allocate_obj(valuelen + 1, &long_value_offset));
+ if (!long_location) return nullptr;
+
+ memcpy(long_location, value, valuelen);
+ long_location[valuelen] = '\0';
+
+ // Both new_offset and long_value_offset are offsets based off of data_, however prop_info
+ // does not know what data_ is, so we change this offset to be an offset from the prop_info
+ // pointer that contains it.
+ long_value_offset -= new_offset;
+
+ info = new (p) prop_info(name, namelen, long_value_offset);
+ } else {
+ info = new (p) prop_info(name, namelen, value, valuelen);
+ }
+ *off = new_offset;
+ return info;
}
void* prop_area::to_prop_obj(uint_least32_t off) {
@@ -1161,11 +1229,8 @@
return pa->find(name);
}
-// The C11 standard doesn't allow atomic loads from const fields,
-// though C++11 does. Fudge it until standards get straightened out.
-static inline uint_least32_t load_const_atomic(const atomic_uint_least32_t* s, memory_order mo) {
- atomic_uint_least32_t* non_const_s = const_cast<atomic_uint_least32_t*>(s);
- return atomic_load_explicit(non_const_s, mo);
+static bool is_read_only(const char* name) {
+ return strncmp(name, "ro.", 3) == 0;
}
int __system_property_read(const prop_info* pi, char* name, char* value) {
@@ -1193,6 +1258,13 @@
pi->name, PROP_NAME_MAX - 1, name);
}
}
+ if (is_read_only(pi->name) && pi->is_long()) {
+ async_safe_format_log(ANDROID_LOG_ERROR, "libc",
+ "The property \"%s\" has a value with length %zu that is too large for"
+ " __system_property_get()/__system_property_read(); use"
+ " __system_property_read_callback() instead.",
+ pi->name, strlen(pi->long_value()));
+ }
return len;
}
}
@@ -1204,6 +1276,18 @@
const char* value,
uint32_t serial),
void* cookie) {
+ // Read only properties don't need to copy the value to a temporary buffer, since it can never
+ // change.
+ if (is_read_only(pi->name)) {
+ uint32_t serial = __system_property_serial(pi);
+ if (pi->is_long()) {
+ callback(cookie, pi->name, pi->long_value(), serial);
+ } else {
+ callback(cookie, pi->name, pi->value, serial);
+ }
+ return;
+ }
+
while (true) {
uint32_t serial = __system_property_serial(pi); // acquire semantics
size_t len = SERIAL_VALUE_LEN(serial);
@@ -1260,15 +1344,15 @@
int __system_property_set(const char* key, const char* value) {
if (key == nullptr) return -1;
if (value == nullptr) value = "";
- if (strlen(value) >= PROP_VALUE_MAX) return -1;
if (g_propservice_protocol_version == 0) {
detect_protocol_version();
}
if (g_propservice_protocol_version == kProtocolVersion1) {
- // Old protocol does not support long names
+ // Old protocol does not support long names or values
if (strlen(key) >= PROP_NAME_MAX) return -1;
+ if (strlen(value) >= PROP_VALUE_MAX) return -1;
prop_msg msg;
memset(&msg, 0, sizeof msg);
@@ -1278,6 +1362,8 @@
return send_prop_msg(&msg);
} else {
+ // New protocol only allows long values for ro. properties only.
+ if (strlen(value) >= PROP_VALUE_MAX && !is_read_only(key)) return -1;
// Use proper protocol
PropertyServiceConnection connection;
if (!connection.IsValid()) {
@@ -1364,7 +1450,7 @@
int __system_property_add(const char* name, unsigned int namelen, const char* value,
unsigned int valuelen) {
- if (valuelen >= PROP_VALUE_MAX) {
+ if (valuelen >= PROP_VALUE_MAX && !is_read_only(name)) {
return -1;
}
diff --git a/tests/system_properties_test.cpp b/tests/system_properties_test.cpp
index 7415b3c..69647bf 100644
--- a/tests/system_properties_test.cpp
+++ b/tests/system_properties_test.cpp
@@ -24,6 +24,8 @@
#include <string>
#include <thread>
+using namespace std::literals;
+
#if defined(__BIONIC__)
#define _REALLY_INCLUDE_SYS__SYSTEM_PROPERTIES_H_
@@ -452,3 +454,89 @@
GTEST_LOG_(INFO) << "This test does nothing.\n";
#endif // __BIONIC__
}
+
+TEST(properties, __system_property_extra_long_read_only) {
+#if defined(__BIONIC__)
+ LocalPropertyTestState pa;
+ ASSERT_TRUE(pa.valid);
+
+ std::vector<std::pair<std::string, std::string>> short_properties = {
+ { "ro.0char", std::string() },
+ { "ro.50char", std::string(50, 'x') },
+ { "ro.91char", std::string(91, 'x') },
+ };
+
+ std::vector<std::pair<std::string, std::string>> long_properties = {
+ { "ro.92char", std::string(92, 'x') },
+ { "ro.93char", std::string(93, 'x') },
+ { "ro.1000char", std::string(1000, 'x') },
+ };
+
+ for (const auto& property : short_properties) {
+ const std::string& name = property.first;
+ const std::string& value = property.second;
+ ASSERT_EQ(0, __system_property_add(name.c_str(), name.size(), value.c_str(), value.size()));
+ }
+
+ for (const auto& property : long_properties) {
+ const std::string& name = property.first;
+ const std::string& value = property.second;
+ ASSERT_EQ(0, __system_property_add(name.c_str(), name.size(), value.c_str(), value.size()));
+ }
+
+ auto check_with_legacy_read = [](const std::string& name, const std::string& expected_value) {
+ char value[PROP_VALUE_MAX];
+ EXPECT_EQ(static_cast<int>(expected_value.size()), __system_property_get(name.c_str(), value))
+ << name;
+ EXPECT_EQ(expected_value, value) << name;
+ };
+
+ auto check_with_read_callback = [](const std::string& name, const std::string& expected_value) {
+ const prop_info* pi = __system_property_find(name.c_str());
+ ASSERT_NE(nullptr, pi);
+ std::string value;
+ __system_property_read_callback(pi,
+ [](void* cookie, const char*, const char* value, uint32_t) {
+ std::string* out_value =
+ reinterpret_cast<std::string*>(cookie);
+ *out_value = value;
+ },
+ &value);
+ EXPECT_EQ(expected_value, value) << name;
+ };
+
+ for (const auto& property : short_properties) {
+ const std::string& name = property.first;
+ const std::string& value = property.second;
+ check_with_legacy_read(name, value);
+ check_with_read_callback(name, value);
+ }
+
+ constexpr static const char* kExtraLongLegacyError =
+ "Must use __system_property_read_callback() to read";
+ for (const auto& property : long_properties) {
+ const std::string& name = property.first;
+ const std::string& value = property.second;
+ check_with_legacy_read(name, kExtraLongLegacyError);
+ check_with_read_callback(name, value);
+ }
+
+#else // __BIONIC__
+ GTEST_LOG_(INFO) << "This test does nothing.\n";
+#endif // __BIONIC__
+}
+
+// pa_size is 128 * 1024 currently, if a property is longer then we expect it to fail gracefully.
+TEST(properties, __system_property_extra_long_read_only_too_long) {
+#if defined(__BIONIC__)
+ LocalPropertyTestState pa;
+ ASSERT_TRUE(pa.valid);
+
+ auto name = "ro.super_long_property"s;
+ auto value = std::string(128 * 1024 + 1, 'x');
+ ASSERT_NE(0, __system_property_add(name.c_str(), name.size(), value.c_str(), value.size()));
+
+#else // __BIONIC__
+ GTEST_LOG_(INFO) << "This test does nothing.\n";
+#endif // __BIONIC__
+}