From cb215a7e9ecec9feb5aae9d9a5b1c89f392208e7 Mon Sep 17 00:00:00 2001 From: Greg Hackmann Date: Wed, 13 Feb 2013 14:41:48 -0800 Subject: [PATCH 1/6] bionic: make property area expandable The property area is initially one 4K region, automatically expanding as needed up to 64 regions. To avoid duplicating code, __system_property_area_init() now allocates and initializes the first region (previously it was allocated in init's init_property_area() and initialized in bionic). For testing purposes, __system_property_set_filename() may be used to override the file used to map in regions. Signed-off-by: Greg Hackmann (cherry picked from commit d32969701be070c0161c2643ee3c3df16066bbb8) Change-Id: I038d451fe8849b0c4863663eec6f57f6521bf4a7 --- libc/bionic/system_properties.c | 187 ++++++++++++++++++++------ libc/include/sys/_system_properties.h | 19 ++- tests/property_benchmark.cpp | 56 ++++++-- tests/system_properties_test.cpp | 76 ++++++++--- 4 files changed, 264 insertions(+), 74 deletions(-) diff --git a/libc/bionic/system_properties.c b/libc/bionic/system_properties.c index 5197ef3ff..800c8b84f 100644 --- a/libc/bionic/system_properties.c +++ b/libc/bionic/system_properties.c @@ -55,7 +55,6 @@ struct prop_area { unsigned volatile serial; unsigned magic; unsigned version; - unsigned reserved[4]; unsigned toc[1]; }; @@ -70,10 +69,9 @@ struct prop_info { typedef struct prop_info prop_info; static const char property_service_socket[] = "/dev/socket/" PROP_SERVICE_NAME; +static char property_filename[PATH_MAX] = PROP_FILENAME; -static unsigned dummy_props = 0; - -prop_area *__system_property_area__ = (void*) &dummy_props; +prop_area *__system_property_regions__[PA_REGION_COUNT] = { NULL, }; static int get_fd_from_env(void) { @@ -86,27 +84,79 @@ static int get_fd_from_env(void) return atoi(env); } -void __system_property_area_init(void *data) +static int map_prop_region_rw(size_t region) { - prop_area *pa = data; + prop_area *pa; + int fd; + size_t offset = region * PA_SIZE; + + if (__system_property_regions__[region]) { + return 0; + } + + /* dev is a tmpfs that we can use to carve a shared workspace + * out of, so let's do that... + */ + fd = open(property_filename, O_RDWR | O_CREAT | O_NOFOLLOW, 0644); + if (fd < 0) { + if (errno == EACCES) { + /* for consistency with the case where the process has already + * mapped the page in and segfaults when trying to write to it + */ + abort(); + } + return -1; + } + + if (ftruncate(fd, offset + PA_SIZE) < 0) + goto out; + + pa = mmap(NULL, PA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, offset); + if(pa == MAP_FAILED) + goto out; + memset(pa, 0, PA_SIZE); pa->magic = PROP_AREA_MAGIC; pa->version = PROP_AREA_VERSION; /* plug into the lib property services */ - __system_property_area__ = pa; + __system_property_regions__[region] = pa; + + close(fd); + return 0; + +out: + close(fd); + return -1; } -int __system_properties_init(void) +int __system_property_set_filename(const char *filename) +{ + size_t len = strlen(filename); + if (len >= sizeof(property_filename)) + return -1; + + strcpy(property_filename, filename); + return 0; +} + +int __system_property_area_init() +{ + return map_prop_region_rw(0); +} + +static int map_prop_region(size_t region) { bool fromFile = true; + bool swapped; + size_t offset = region * PA_SIZE; int result = -1; - if(__system_property_area__ != ((void*) &dummy_props)) { + if(__system_property_regions__[region]) { return 0; } - int fd = open(PROP_FILENAME, O_RDONLY | O_NOFOLLOW); + int fd = open(property_filename, O_RDONLY | O_NOFOLLOW); if ((fd < 0) && (errno == ENOENT)) { /* @@ -133,23 +183,33 @@ int __system_properties_init(void) if ((fd_stat.st_uid != 0) || (fd_stat.st_gid != 0) - || ((fd_stat.st_mode & (S_IWGRP | S_IWOTH)) != 0)) { + || ((fd_stat.st_mode & (S_IWGRP | S_IWOTH)) != 0) + || (fd_stat.st_size < offset + PA_SIZE) ) { goto cleanup; } - prop_area *pa = mmap(NULL, fd_stat.st_size, PROT_READ, MAP_SHARED, fd, 0); + prop_area *pa = mmap(NULL, PA_SIZE, PROT_READ, MAP_SHARED, fd, offset); if (pa == MAP_FAILED) { goto cleanup; } if((pa->magic != PROP_AREA_MAGIC) || (pa->version != PROP_AREA_VERSION)) { - munmap(pa, fd_stat.st_size); + munmap(pa, PA_SIZE); goto cleanup; } - __system_property_area__ = pa; result = 0; + swapped = __sync_bool_compare_and_swap(&__system_property_regions__[region], + NULL, pa); + if (!swapped) { + /** + * In the event of a race either mapping is equally good, so + * the thread that lost can just throw its mapping away and proceed as + * normal. + */ + munmap(pa, PA_SIZE); + } cleanup: if (fromFile) { @@ -159,17 +219,31 @@ cleanup: return result; } +int __system_properties_init() +{ + return map_prop_region(0); +} + int __system_property_foreach( void (*propfn)(const prop_info *pi, void *cookie), void *cookie) { - prop_area *pa = __system_property_area__; - unsigned i; + size_t region; - for (i = 0; i < pa->count; i++) { - unsigned entry = pa->toc[i]; - prop_info *pi = TOC_TO_INFO(pa, entry); - propfn(pi, cookie); + for (region = 0; region < PA_REGION_COUNT; region++) { + prop_area *pa; + unsigned i; + + int err = map_prop_region(region); + if (err < 0) + break; + pa = __system_property_regions__[region]; + + for (i = 0; i < pa->count; i++) { + unsigned entry = pa->toc[i]; + prop_info *pi = TOC_TO_INFO(pa, entry); + propfn(pi, cookie); + } } return 0; @@ -177,9 +251,15 @@ int __system_property_foreach( const prop_info *__system_property_find_nth(unsigned n) { - prop_area *pa = __system_property_area__; + size_t region = n / PA_COUNT_MAX; + prop_area *pa; - if(n >= pa->count) { + int err = map_prop_region(region); + if (err < 0) + return NULL; + pa = __system_property_regions__[region]; + + if((n % PA_COUNT_MAX) >= pa->count) { return 0; } else { return TOC_TO_INFO(pa, pa->toc[n]); @@ -188,25 +268,36 @@ const prop_info *__system_property_find_nth(unsigned n) const prop_info *__system_property_find(const char *name) { - prop_area *pa = __system_property_area__; - unsigned count = pa->count; - unsigned *toc = pa->toc; unsigned len = strlen(name); - prop_info *pi; + size_t region; if (len >= PROP_NAME_MAX) return 0; if (len < 1) return 0; - while(count--) { - unsigned entry = *toc++; - if(TOC_NAME_LEN(entry) != len) continue; + for (region = 0; region < PA_REGION_COUNT; region++) { + prop_area *pa; + unsigned count; + unsigned *toc; + prop_info *pi; - pi = TOC_TO_INFO(pa, entry); - if(memcmp(name, pi->name, len)) continue; + int err = map_prop_region(region); + if (err < 0) + return 0; + pa = __system_property_regions__[region]; + count = pa->count; + toc = pa->toc; - return pi; + while(count--) { + unsigned entry = *toc++; + if(TOC_NAME_LEN(entry) != len) continue; + + pi = TOC_TO_INFO(pa, entry); + if(memcmp(name, pi->name, len)) continue; + + return pi; + } } return 0; @@ -333,7 +424,7 @@ int __system_property_wait(const prop_info *pi) { unsigned n; if(pi == 0) { - prop_area *pa = __system_property_area__; + prop_area *pa = __system_property_regions__[0]; n = pa->serial; do { __futex_wait(&pa->serial, n, 0); @@ -349,7 +440,7 @@ int __system_property_wait(const prop_info *pi) int __system_property_update(prop_info *pi, const char *value, unsigned int len) { - prop_area *pa = __system_property_area__; + prop_area *pa = __system_property_regions__[0]; if (len >= PROP_VALUE_MAX) return -1; @@ -368,12 +459,11 @@ int __system_property_update(prop_info *pi, const char *value, unsigned int len) int __system_property_add(const char *name, unsigned int namelen, const char *value, unsigned int valuelen) { - prop_area *pa = __system_property_area__; - prop_info *pa_info_array = (void*) (((char*) pa) + PA_INFO_START); + prop_area *pa; + prop_info *pa_info_array; prop_info *pi; + size_t region; - if (pa->count == PA_COUNT_MAX) - return -1; if (namelen >= PROP_NAME_MAX) return -1; if (valuelen >= PROP_VALUE_MAX) @@ -381,13 +471,28 @@ int __system_property_add(const char *name, unsigned int namelen, if (namelen < 1) return -1; + for (region = 0; region < PA_REGION_COUNT; region++) + { + int err = map_prop_region_rw(region); + if (err < 0) + return -1; + + pa = __system_property_regions__[region]; + + if (pa->count < PA_COUNT_MAX) + break; + } + + if (region == PA_REGION_COUNT) + return -1; + + pa_info_array = (void*) (((char*) pa) + PA_INFO_START); pi = pa_info_array + pa->count; pi->serial = (valuelen << 24); memcpy(pi->name, name, namelen + 1); memcpy(pi->value, value, valuelen + 1); - pa->toc[pa->count] = - (namelen << 24) | (((unsigned) pi) - ((unsigned) pa)); + pa->toc[pa->count] = (namelen << 24) | (((unsigned) pi) - ((unsigned) pa)); pa->count++; pa->serial++; @@ -403,7 +508,7 @@ unsigned int __system_property_serial(const prop_info *pi) unsigned int __system_property_wait_any(unsigned int serial) { - prop_area *pa = __system_property_area__; + prop_area *pa = __system_property_regions__[0]; do { __futex_wait(&pa->serial, serial, 0); diff --git a/libc/include/sys/_system_properties.h b/libc/include/sys/_system_properties.h index c5bc2235b..4971a4c12 100644 --- a/libc/include/sys/_system_properties.h +++ b/libc/include/sys/_system_properties.h @@ -42,12 +42,13 @@ typedef struct prop_msg prop_msg; #define PROP_SERVICE_NAME "property_service" #define PROP_FILENAME "/dev/__properties__" -/* (8 header words + 247 toc words) = 1020 bytes */ -/* 1024 bytes header and toc + 247 prop_infos @ 128 bytes = 32640 bytes */ +/* (4 header words + 28 toc words) = 128 bytes */ +/* 128 bytes header and toc + 28 prop_infos @ 128 bytes = 3712 bytes */ -#define PA_COUNT_MAX 247 -#define PA_INFO_START 1024 -#define PA_SIZE 32768 +#define PA_COUNT_MAX 28 +#define PA_REGION_COUNT 128 +#define PA_INFO_START 128 +#define PA_SIZE 4096 #define TOC_NAME_LEN(toc) ((toc) >> 24) #define TOC_TO_INFO(area, toc) ((prop_info*) (((char*) area) + ((toc) & 0xFFFFFF))) @@ -96,12 +97,18 @@ struct prop_msg #define PROP_PATH_LOCAL_OVERRIDE "/data/local.prop" #define PROP_PATH_FACTORY "/factory/factory.prop" +/* +** Map the property area from the specified filename. This +** method is for testing only. +*/ +int __system_property_set_filename(const char *filename); + /* ** Initialize the area to be used to store properties. Can ** only be done by a single process that has write access to ** the property area. */ -void __system_property_area_init(void *data); +int __system_property_area_init(); /* Add a new system property. Can only be done by a single ** process that has write access to the property area, and diff --git a/tests/property_benchmark.cpp b/tests/property_benchmark.cpp index 2c8e2a115..7266bd08a 100644 --- a/tests/property_benchmark.cpp +++ b/tests/property_benchmark.cpp @@ -15,23 +15,40 @@ */ #include "benchmark.h" +#include #define _REALLY_INCLUDE_SYS__SYSTEM_PROPERTIES_H_ #include #include +#include -extern void *__system_property_area__; +extern void *__system_property_regions__[PA_REGION_COUNT]; #define TEST_NUM_PROPS \ - Arg(1)->Arg(4)->Arg(16)->Arg(64)->Arg(128)->Arg(247) + Arg(1)->Arg(4)->Arg(16)->Arg(64)->Arg(128)->Arg(256)->Arg(512)->Arg(1024) struct LocalPropertyTestState { - LocalPropertyTestState(int nprops) : nprops(nprops) { + LocalPropertyTestState(int nprops) : nprops(nprops), valid(false) { static const char prop_name_chars[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ.-_"; - old_pa = __system_property_area__; - pa = malloc(PA_SIZE); - __system_property_area_init(pa); + + char dir_template[] = "/data/nativetest/prop-XXXXXX"; + char *dirname = mkdtemp(dir_template); + if (!dirname) { + perror("making temp file for test state failed (is /data/nativetest writable?)"); + return; + } + + for (size_t i = 0; i < PA_REGION_COUNT; i++) { + old_pa[i] = __system_property_regions__[i]; + __system_property_regions__[i] = NULL; + } + + pa_dirname = dirname; + pa_filename = pa_dirname + "/__properties__"; + + __system_property_set_filename(pa_filename.c_str()); + __system_property_area_init(); names = new char* [nprops]; name_lens = new int[nprops]; @@ -54,10 +71,22 @@ struct LocalPropertyTestState { } __system_property_add(names[i], name_lens[i], values[i], value_lens[i]); } + + valid = true; } ~LocalPropertyTestState() { - __system_property_area__ = old_pa; + if (!valid) + return; + + for (size_t i = 0; i < PA_REGION_COUNT; i++) { + __system_property_regions__[i] = old_pa[i]; + } + + __system_property_set_filename(PROP_FILENAME); + unlink(pa_filename.c_str()); + rmdir(pa_dirname.c_str()); + for (int i = 0; i < nprops; i++) { delete names[i]; delete values[i]; @@ -66,7 +95,6 @@ struct LocalPropertyTestState { delete[] name_lens; delete[] values; delete[] value_lens; - free(pa); } public: const int nprops; @@ -74,10 +102,12 @@ public: int *name_lens; char **values; int *value_lens; + bool valid; private: - void *pa; - void *old_pa; + std::string pa_dirname; + std::string pa_filename; + void *old_pa[PA_REGION_COUNT]; }; static void BM_property_get(int iters, int nprops) @@ -87,6 +117,9 @@ static void BM_property_get(int iters, int nprops) LocalPropertyTestState pa(nprops); char value[PROP_VALUE_MAX]; + if (!pa.valid) + return; + srandom(iters * nprops); StartBenchmarkTiming(); @@ -104,6 +137,9 @@ static void BM_property_find(int iters, int nprops) LocalPropertyTestState pa(nprops); + if (!pa.valid) + return; + srandom(iters * nprops); StartBenchmarkTiming(); diff --git a/tests/system_properties_test.cpp b/tests/system_properties_test.cpp index 70ff1d662..50bdfdfe4 100644 --- a/tests/system_properties_test.cpp +++ b/tests/system_properties_test.cpp @@ -16,32 +16,61 @@ #include #include +#include +#include #if __BIONIC__ #define _REALLY_INCLUDE_SYS__SYSTEM_PROPERTIES_H_ #include -extern void *__system_property_area__; +extern void *__system_property_regions__[PA_REGION_COUNT]; struct LocalPropertyTestState { - LocalPropertyTestState() { - old_pa = __system_property_area__; - pa = malloc(PA_SIZE); - __system_property_area_init(pa); + LocalPropertyTestState() : valid(false) { + char dir_template[] = "/data/nativetest/prop-XXXXXX"; + char *dirname = mkdtemp(dir_template); + if (!dirname) { + perror("making temp file for test state failed (is /data/nativetest writable?)"); + return; + } + + for (size_t i = 0; i < PA_REGION_COUNT; i++) { + old_pa[i] = __system_property_regions__[i]; + __system_property_regions__[i] = NULL; + } + + pa_dirname = dirname; + pa_filename = pa_dirname + "/__properties__"; + + __system_property_set_filename(pa_filename.c_str()); + __system_property_area_init(); + valid = true; } ~LocalPropertyTestState() { - __system_property_area__ = old_pa; - free(pa); + if (!valid) + return; + + for (size_t i = 0; i < PA_REGION_COUNT; i++) { + __system_property_regions__[i] = old_pa[i]; + } + + __system_property_set_filename(PROP_FILENAME); + unlink(pa_filename.c_str()); + rmdir(pa_dirname.c_str()); } +public: + bool valid; private: - void *pa; - void *old_pa; + std::string pa_dirname; + std::string pa_filename; + void *old_pa[PA_REGION_COUNT]; }; TEST(properties, add) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); char propvalue[PROP_VALUE_MAX]; @@ -61,6 +90,7 @@ TEST(properties, add) { TEST(properties, update) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); char propvalue[PROP_VALUE_MAX]; prop_info *pi; @@ -91,27 +121,34 @@ TEST(properties, update) { ASSERT_STREQ(propvalue, "value6"); } -// 247 = max # of properties supported by current implementation -// (this should never go down) -TEST(properties, fill_247) { +TEST(properties, fill) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); char prop_name[PROP_NAME_MAX]; char prop_value[PROP_VALUE_MAX]; char prop_value_ret[PROP_VALUE_MAX]; + int count = 0; int ret; - for (int i = 0; i < 247; i++) { - ret = snprintf(prop_name, PROP_NAME_MAX - 1, "property_%d", i); + while (true) { + ret = snprintf(prop_name, PROP_NAME_MAX - 1, "property_%d", count); memset(prop_name + ret, 'a', PROP_NAME_MAX - 1 - ret); - ret = snprintf(prop_value, PROP_VALUE_MAX - 1, "value_%d", i); + ret = snprintf(prop_value, PROP_VALUE_MAX - 1, "value_%d", count); memset(prop_value + ret, 'b', PROP_VALUE_MAX - 1 - ret); prop_name[PROP_NAME_MAX - 1] = 0; prop_value[PROP_VALUE_MAX - 1] = 0; - ASSERT_EQ(0, __system_property_add(prop_name, PROP_NAME_MAX - 1, prop_value, PROP_VALUE_MAX - 1)); + ret = __system_property_add(prop_name, PROP_NAME_MAX - 1, prop_value, PROP_VALUE_MAX - 1); + if (ret < 0) + break; + + count++; } - for (int i = 0; i < 247; i++) { + // For historical reasons at least 247 properties must be supported + ASSERT_GE(count, 247); + + for (int i = 0; i < count; i++) { ret = snprintf(prop_name, PROP_NAME_MAX - 1, "property_%d", i); memset(prop_name + ret, 'a', PROP_NAME_MAX - 1 - ret); ret = snprintf(prop_value, PROP_VALUE_MAX - 1, "value_%d", i); @@ -134,6 +171,7 @@ static void foreach_test_callback(const prop_info *pi, void* cookie) { TEST(properties, foreach) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); size_t count = 0; ASSERT_EQ(0, __system_property_add("property", 8, "value1", 6)); @@ -146,6 +184,7 @@ TEST(properties, foreach) { TEST(properties, find_nth) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); ASSERT_EQ(0, __system_property_add("property", 8, "value1", 6)); ASSERT_EQ(0, __system_property_add("other_property", 14, "value2", 6)); @@ -165,6 +204,7 @@ TEST(properties, find_nth) { TEST(properties, errors) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); char prop_value[PROP_NAME_MAX]; ASSERT_EQ(0, __system_property_add("property", 8, "value1", 6)); @@ -181,6 +221,7 @@ TEST(properties, errors) { TEST(properties, serial) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); const prop_info *pi; unsigned int serial; @@ -206,6 +247,7 @@ static void *PropertyWaitHelperFn(void *arg) TEST(properties, wait) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); unsigned int serial; prop_info *pi; pthread_t t; From f7511e3bc932f9f4d025a62871c29bf1e0ac0ea7 Mon Sep 17 00:00:00 2001 From: Greg Hackmann Date: Thu, 20 Jun 2013 10:33:28 -0700 Subject: [PATCH 2/6] bionic: add missing memory barriers to system properties 1) Reading the value must finish before checking whether it's intact 2) Setting the serial's dirty bit must visible before modifying the value 3) The modified value must be visible before clearing the serial's dirty bit 4) New properties and their TOC entries must be visible before updating the property count Signed-off-by: Greg Hackmann (cherry picked from commit 5bfa3ee8b37ef162154559575193018a6235acba) Change-Id: Id3fa45261fc2df2ae493ab5194bc2b6bff04e966 --- libc/bionic/system_properties.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/libc/bionic/system_properties.c b/libc/bionic/system_properties.c index 800c8b84f..b12879e47 100644 --- a/libc/bionic/system_properties.c +++ b/libc/bionic/system_properties.c @@ -49,6 +49,7 @@ #include #include +#include struct prop_area { unsigned volatile count; @@ -315,6 +316,7 @@ int __system_property_read(const prop_info *pi, char *name, char *value) } len = SERIAL_VALUE_LEN(serial); memcpy(value, pi->value, len + 1); + ANDROID_MEMBAR_FULL(); if(serial == pi->serial) { if(name != 0) { strcpy(name, pi->name); @@ -446,7 +448,9 @@ int __system_property_update(prop_info *pi, const char *value, unsigned int len) return -1; pi->serial = pi->serial | 1; + ANDROID_MEMBAR_FULL(); memcpy(pi->value, value, len + 1); + ANDROID_MEMBAR_FULL(); pi->serial = (len << 24) | ((pi->serial + 1) & 0xffffff); __futex_wake(&pi->serial, INT32_MAX); @@ -493,6 +497,7 @@ int __system_property_add(const char *name, unsigned int namelen, memcpy(pi->value, value, valuelen + 1); pa->toc[pa->count] = (namelen << 24) | (((unsigned) pi) - ((unsigned) pa)); + ANDROID_MEMBAR_FULL(); pa->count++; pa->serial++; From 996cdc4b1a7fcae89650bee0a44b6cb7900a4a3c Mon Sep 17 00:00:00 2001 From: Greg Hackmann Date: Thu, 20 Jun 2013 11:27:56 -0700 Subject: [PATCH 3/6] bionic: reimplement property area as hybrid trie/binary tree See the comments for an explanation of how properties are stored. The trie structure is designed to scale better than the previous array-based implementation. Searching an array with n properties required average O(n) string compares of the entire key; searching the trie requires average O(log n) string compares of each token (substrings between '.' characters). Signed-off-by: Greg Hackmann (cherry picked from commit 6ac8e6a46d71a51bec16938efa89f275fa89cf7d) Change-Id: Icbe31908572f33b4d9b85d5b62ac837cbd0f85e0 --- libc/bionic/system_properties.c | 369 +++++++++++++++++++------- libc/include/sys/_system_properties.h | 12 +- 2 files changed, 280 insertions(+), 101 deletions(-) diff --git a/libc/bionic/system_properties.c b/libc/bionic/system_properties.c index b12879e47..126fea56a 100644 --- a/libc/bionic/system_properties.c +++ b/libc/bionic/system_properties.c @@ -34,6 +34,7 @@ #include #include #include +#include #include @@ -51,12 +52,15 @@ #include #include +#define ALIGN(x, a) (((x) + (a - 1)) & ~(a - 1)) + struct prop_area { - unsigned volatile count; + unsigned bytes_used; unsigned volatile serial; unsigned magic; unsigned version; - unsigned toc[1]; + unsigned reserved[28]; + char data[0]; }; typedef struct prop_area prop_area; @@ -69,11 +73,46 @@ struct prop_info { typedef struct prop_info prop_info; +/* + * Properties are stored in a hybrid trie/binary tree structure. + * Each property's name is delimited at '.' characters, and the tokens are put + * into a trie structure. Siblings at each level of the trie are stored in a + * binary tree. For instance, "ro.secure"="1" could be stored as follows: + * + * +-----+ children +----+ children +--------+ + * | |-------------->| ro |-------------->| secure | + * +-----+ +----+ +--------+ + * / \ / | + * left / \ right left / | prop +===========+ + * v v v +-------->| ro.secure | + * +-----+ +-----+ +-----+ +-----------+ + * | net | | sys | | com | | 1 | + * +-----+ +-----+ +-----+ +===========+ + */ + +typedef volatile uint32_t prop_off_t; +struct prop_bt { + char name[PROP_NAME_MAX]; + uint8_t namelen; + uint8_t reserved[3]; + + prop_off_t prop; + + prop_off_t left; + prop_off_t right; + + prop_off_t children; +}; + +typedef struct prop_bt prop_bt; + static const char property_service_socket[] = "/dev/socket/" PROP_SERVICE_NAME; static char property_filename[PATH_MAX] = PROP_FILENAME; prop_area *__system_property_regions__[PA_REGION_COUNT] = { NULL, }; +const size_t PA_DATA_SIZE = PA_SIZE - sizeof(prop_area); + static int get_fd_from_env(void) { char *env = getenv("ANDROID_PROPERTY_WORKSPACE"); @@ -120,6 +159,11 @@ static int map_prop_region_rw(size_t region) pa->magic = PROP_AREA_MAGIC; pa->version = PROP_AREA_VERSION; + if (!region) { + /* reserve root node */ + pa->bytes_used += sizeof(prop_bt); + } + /* plug into the lib property services */ __system_property_regions__[region] = pa; @@ -225,83 +269,183 @@ int __system_properties_init() return map_prop_region(0); } -int __system_property_foreach( - void (*propfn)(const prop_info *pi, void *cookie), - void *cookie) +static void *new_prop_obj(size_t size, prop_off_t *off) { - size_t region; + prop_area *pa; + size_t i, idx; + size = ALIGN(size, sizeof(uint32_t)); - for (region = 0; region < PA_REGION_COUNT; region++) { - prop_area *pa; - unsigned i; - - int err = map_prop_region(region); - if (err < 0) - break; - pa = __system_property_regions__[region]; - - for (i = 0; i < pa->count; i++) { - unsigned entry = pa->toc[i]; - prop_info *pi = TOC_TO_INFO(pa, entry); - propfn(pi, cookie); + for (i = 0; i < PA_REGION_COUNT; i++) { + int err = map_prop_region_rw(i); + if (err < 0) { + return NULL; } + + pa = __system_property_regions__[i]; + if (pa->bytes_used + size <= PA_DATA_SIZE) + break; } - return 0; + if (i == PA_REGION_COUNT) + return NULL; + + idx = pa->bytes_used; + *off = idx + i * PA_DATA_SIZE; + pa->bytes_used += size; + return pa->data + idx; } -const prop_info *__system_property_find_nth(unsigned n) +static prop_bt *new_prop_bt(const char *name, uint8_t namelen, prop_off_t *off) { - size_t region = n / PA_COUNT_MAX; - prop_area *pa; + prop_off_t off_tmp; + prop_bt *bt = new_prop_obj(sizeof(prop_bt), &off_tmp); + if (bt) { + memcpy(bt->name, name, namelen); + bt->name[namelen] = '\0'; + bt->namelen = namelen; + ANDROID_MEMBAR_FULL(); + *off = off_tmp; + } - int err = map_prop_region(region); - if (err < 0) + return bt; +} + +static prop_info *new_prop_info(const char *name, uint8_t namelen, + const char *value, uint8_t valuelen, prop_off_t *off) +{ + prop_off_t off_tmp; + prop_info *info = new_prop_obj(sizeof(prop_info), &off_tmp); + if (info) { + memcpy(info->name, name, namelen); + info->name[namelen] = '\0'; + info->serial = (valuelen << 24); + memcpy(info->value, value, valuelen); + info->value[valuelen] = '\0'; + ANDROID_MEMBAR_FULL(); + *off = off_tmp; + } + + return info; +} + +static void *to_prop_obj(prop_off_t off) +{ + size_t region = off / PA_DATA_SIZE; + size_t idx = off % PA_DATA_SIZE; + + if (region > PA_REGION_COUNT) return NULL; - pa = __system_property_regions__[region]; - if((n % PA_COUNT_MAX) >= pa->count) { - return 0; + if (map_prop_region(region) < 0) + return NULL; + + return __system_property_regions__[region]->data + idx; +} + +static prop_bt *root_node() +{ + return to_prop_obj(0); +} + +static int cmp_prop_name(const char *one, uint8_t one_len, const char *two, + uint8_t two_len) +{ + if (one_len < two_len) + return -1; + else if (one_len > two_len) + return 1; + else + return strncmp(one, two, one_len); +} + +static prop_bt *find_prop_bt(prop_bt *bt, const char *name, uint8_t namelen, + bool alloc_if_needed) +{ + while (true) { + int ret; + if (!bt) + return bt; + ret = cmp_prop_name(name, namelen, bt->name, bt->namelen); + + if (ret == 0) { + return bt; + } else if (ret < 0) { + if (bt->left) { + bt = to_prop_obj(bt->left); + } else { + if (!alloc_if_needed) + return NULL; + + bt = new_prop_bt(name, namelen, &bt->left); + } + } else { + if (bt->right) { + bt = to_prop_obj(bt->right); + } else { + if (!alloc_if_needed) + return NULL; + + bt = new_prop_bt(name, namelen, &bt->right); + } + } + } +} + +static const prop_info *find_property(prop_bt *trie, const char *name, + uint8_t namelen, const char *value, uint8_t valuelen, + bool alloc_if_needed) +{ + const char *remaining_name = name; + + while (true) { + char *sep = strchr(remaining_name, '.'); + bool want_subtree = (sep != NULL); + uint8_t substr_size; + + prop_bt *root; + + if (want_subtree) { + substr_size = sep - remaining_name; + } else { + substr_size = strlen(remaining_name); + } + + if (!substr_size) + return NULL; + + if (trie->children) { + root = to_prop_obj(trie->children); + } else if (alloc_if_needed) { + root = new_prop_bt(remaining_name, substr_size, &trie->children); + } else { + root = NULL; + } + + if (!root) + return NULL; + + trie = find_prop_bt(root, remaining_name, substr_size, alloc_if_needed); + if (!trie) + return NULL; + + if (!want_subtree) + break; + + remaining_name = sep + 1; + } + + if (trie->prop) { + return to_prop_obj(trie->prop); + } else if (alloc_if_needed) { + return new_prop_info(name, namelen, value, valuelen, &trie->prop); } else { - return TOC_TO_INFO(pa, pa->toc[n]); + return NULL; } } const prop_info *__system_property_find(const char *name) { - unsigned len = strlen(name); - size_t region; - - if (len >= PROP_NAME_MAX) - return 0; - if (len < 1) - return 0; - - for (region = 0; region < PA_REGION_COUNT; region++) { - prop_area *pa; - unsigned count; - unsigned *toc; - prop_info *pi; - - int err = map_prop_region(region); - if (err < 0) - return 0; - pa = __system_property_regions__[region]; - count = pa->count; - toc = pa->toc; - - while(count--) { - unsigned entry = *toc++; - if(TOC_NAME_LEN(entry) != len) continue; - - pi = TOC_TO_INFO(pa, entry); - if(memcmp(name, pi->name, len)) continue; - - return pi; - } - } - - return 0; + return find_property(root_node(), name, strlen(name), NULL, 0, false); } int __system_property_read(const prop_info *pi, char *name, char *value) @@ -463,10 +607,8 @@ int __system_property_update(prop_info *pi, const char *value, unsigned int len) int __system_property_add(const char *name, unsigned int namelen, const char *value, unsigned int valuelen) { - prop_area *pa; - prop_info *pa_info_array; - prop_info *pi; - size_t region; + prop_area *pa = __system_property_regions__[0]; + const prop_info *pi; if (namelen >= PROP_NAME_MAX) return -1; @@ -475,34 +617,12 @@ int __system_property_add(const char *name, unsigned int namelen, if (namelen < 1) return -1; - for (region = 0; region < PA_REGION_COUNT; region++) - { - int err = map_prop_region_rw(region); - if (err < 0) - return -1; - - pa = __system_property_regions__[region]; - - if (pa->count < PA_COUNT_MAX) - break; - } - - if (region == PA_REGION_COUNT) + pi = find_property(root_node(), name, namelen, value, valuelen, true); + if (!pi) return -1; - pa_info_array = (void*) (((char*) pa) + PA_INFO_START); - pi = pa_info_array + pa->count; - pi->serial = (valuelen << 24); - memcpy(pi->name, name, namelen + 1); - memcpy(pi->value, value, valuelen + 1); - - pa->toc[pa->count] = (namelen << 24) | (((unsigned) pi) - ((unsigned) pa)); - ANDROID_MEMBAR_FULL(); - - pa->count++; pa->serial++; __futex_wake(&pa->serial, INT32_MAX); - return 0; } @@ -521,3 +641,72 @@ unsigned int __system_property_wait_any(unsigned int serial) return pa->serial; } + +struct find_nth_cookie { + unsigned count; + unsigned n; + const prop_info *pi; +}; + +static void find_nth_fn(const prop_info *pi, void *ptr) +{ + struct find_nth_cookie *cookie = ptr; + + if (cookie->n == cookie->count) + cookie->pi = pi; + + cookie->count++; +} + +const prop_info *__system_property_find_nth(unsigned n) +{ + struct find_nth_cookie cookie; + int err; + + memset(&cookie, 0, sizeof(cookie)); + cookie.n = n; + + err = __system_property_foreach(find_nth_fn, &cookie); + if (err < 0) + return NULL; + + return cookie.pi; +} + +static int foreach_property(prop_off_t off, + void (*propfn)(const prop_info *pi, void *cookie), void *cookie) +{ + prop_bt *trie = to_prop_obj(off); + if (!trie) + return -1; + + if (trie->left) { + int err = foreach_property(trie->left, propfn, cookie); + if (err < 0) + return -1; + } + if (trie->prop) { + prop_info *info = to_prop_obj(trie->prop); + if (!info) + return -1; + propfn(info, cookie); + } + if (trie->children) { + int err = foreach_property(trie->children, propfn, cookie); + if (err < 0) + return -1; + } + if (trie->right) { + int err = foreach_property(trie->right, propfn, cookie); + if (err < 0) + return -1; + } + + return 0; +} + +int __system_property_foreach(void (*propfn)(const prop_info *pi, void *cookie), + void *cookie) +{ + return foreach_property(0, propfn, cookie); +} diff --git a/libc/include/sys/_system_properties.h b/libc/include/sys/_system_properties.h index 4971a4c12..9c48e1b3e 100644 --- a/libc/include/sys/_system_properties.h +++ b/libc/include/sys/_system_properties.h @@ -37,7 +37,7 @@ typedef struct prop_msg prop_msg; #define PROP_AREA_MAGIC 0x504f5250 -#define PROP_AREA_VERSION 0x45434f76 +#define PROP_AREA_VERSION 0xfc6ed0ab #define PROP_SERVICE_NAME "property_service" #define PROP_FILENAME "/dev/__properties__" @@ -45,14 +45,9 @@ typedef struct prop_msg prop_msg; /* (4 header words + 28 toc words) = 128 bytes */ /* 128 bytes header and toc + 28 prop_infos @ 128 bytes = 3712 bytes */ -#define PA_COUNT_MAX 28 #define PA_REGION_COUNT 128 -#define PA_INFO_START 128 #define PA_SIZE 4096 -#define TOC_NAME_LEN(toc) ((toc) >> 24) -#define TOC_TO_INFO(area, toc) ((prop_info*) (((char*) area) + ((toc) & 0xFFFFFF))) - #define SERIAL_VALUE_LEN(serial) ((serial) >> 24) #define SERIAL_DIRTY(serial) ((serial) & 1) @@ -84,11 +79,6 @@ struct prop_msg ** 1. pi->serial = pi->serial | 1 ** 2. memcpy(pi->value, local_value, value_len) ** 3. pi->serial = (value_len << 24) | ((pi->serial + 1) & 0xffffff) -** -** Improvements: -** - maintain the toc sorted by pi->name to allow lookup -** by binary search -** */ #define PROP_PATH_RAMDISK_DEFAULT "/default.prop" From 1540f601be32bdd4af8e8c13bdf2bc06bdaa76f1 Mon Sep 17 00:00:00 2001 From: Greg Hackmann Date: Wed, 19 Jun 2013 13:31:21 -0700 Subject: [PATCH 4/6] bionic: revert to a single (larger) property area d329697 is too complicated. Change the multiple property pages back to a single 128K property area that's mapped in entirely at initialization (the memory will not get allocated until the pages are touched). d329697 has other changes useful for testing (moving property area initialization inside bionic and adding __system_property_set_filename) so undo the change manually rather than with git revert. Signed-off-by: Greg Hackmann (cherry picked from commit 5f05348c18286a2cea46eae8acf94ed5b7932fac) Change-Id: I690704552afc07a4dd410277893ca9c40bc13e5f --- libc/bionic/system_properties.c | 91 ++++++++------------------- libc/include/sys/_system_properties.h | 6 +- tests/property_benchmark.cpp | 14 ++--- tests/system_properties_test.cpp | 14 ++--- 4 files changed, 36 insertions(+), 89 deletions(-) diff --git a/libc/bionic/system_properties.c b/libc/bionic/system_properties.c index 126fea56a..d4054d245 100644 --- a/libc/bionic/system_properties.c +++ b/libc/bionic/system_properties.c @@ -109,7 +109,7 @@ typedef struct prop_bt prop_bt; static const char property_service_socket[] = "/dev/socket/" PROP_SERVICE_NAME; static char property_filename[PATH_MAX] = PROP_FILENAME; -prop_area *__system_property_regions__[PA_REGION_COUNT] = { NULL, }; +prop_area *__system_property_area__ = NULL; const size_t PA_DATA_SIZE = PA_SIZE - sizeof(prop_area); @@ -124,15 +124,10 @@ static int get_fd_from_env(void) return atoi(env); } -static int map_prop_region_rw(size_t region) +static int map_prop_area_rw() { prop_area *pa; int fd; - size_t offset = region * PA_SIZE; - - if (__system_property_regions__[region]) { - return 0; - } /* dev is a tmpfs that we can use to carve a shared workspace * out of, so let's do that... @@ -148,24 +143,21 @@ static int map_prop_region_rw(size_t region) return -1; } - if (ftruncate(fd, offset + PA_SIZE) < 0) + if (ftruncate(fd, PA_SIZE) < 0) goto out; - pa = mmap(NULL, PA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, offset); + pa = mmap(NULL, PA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); if(pa == MAP_FAILED) goto out; memset(pa, 0, PA_SIZE); pa->magic = PROP_AREA_MAGIC; pa->version = PROP_AREA_VERSION; - - if (!region) { - /* reserve root node */ - pa->bytes_used += sizeof(prop_bt); - } + /* reserve root node */ + pa->bytes_used = sizeof(prop_bt); /* plug into the lib property services */ - __system_property_regions__[region] = pa; + __system_property_area__ = pa; close(fd); return 0; @@ -187,20 +179,14 @@ int __system_property_set_filename(const char *filename) int __system_property_area_init() { - return map_prop_region_rw(0); + return map_prop_area_rw(); } -static int map_prop_region(size_t region) +static int map_prop_area() { bool fromFile = true; - bool swapped; - size_t offset = region * PA_SIZE; int result = -1; - if(__system_property_regions__[region]) { - return 0; - } - int fd = open(property_filename, O_RDONLY | O_NOFOLLOW); if ((fd < 0) && (errno == ENOENT)) { @@ -229,11 +215,11 @@ static int map_prop_region(size_t region) if ((fd_stat.st_uid != 0) || (fd_stat.st_gid != 0) || ((fd_stat.st_mode & (S_IWGRP | S_IWOTH)) != 0) - || (fd_stat.st_size < offset + PA_SIZE) ) { + || (fd_stat.st_size < PA_SIZE) ) { goto cleanup; } - prop_area *pa = mmap(NULL, PA_SIZE, PROT_READ, MAP_SHARED, fd, offset); + prop_area *pa = mmap(NULL, PA_SIZE, PROT_READ, MAP_SHARED, fd, 0); if (pa == MAP_FAILED) { goto cleanup; @@ -245,16 +231,8 @@ static int map_prop_region(size_t region) } result = 0; - swapped = __sync_bool_compare_and_swap(&__system_property_regions__[region], - NULL, pa); - if (!swapped) { - /** - * In the event of a race either mapping is equally good, so - * the thread that lost can just throw its mapping away and proceed as - * normal. - */ - munmap(pa, PA_SIZE); - } + + __system_property_area__ = pa; cleanup: if (fromFile) { @@ -266,33 +244,20 @@ cleanup: int __system_properties_init() { - return map_prop_region(0); + return map_prop_area(); } static void *new_prop_obj(size_t size, prop_off_t *off) { - prop_area *pa; - size_t i, idx; + prop_area *pa = __system_property_area__; size = ALIGN(size, sizeof(uint32_t)); - for (i = 0; i < PA_REGION_COUNT; i++) { - int err = map_prop_region_rw(i); - if (err < 0) { - return NULL; - } - - pa = __system_property_regions__[i]; - if (pa->bytes_used + size <= PA_DATA_SIZE) - break; - } - - if (i == PA_REGION_COUNT) + if (pa->bytes_used + size > PA_DATA_SIZE) return NULL; - idx = pa->bytes_used; - *off = idx + i * PA_DATA_SIZE; - pa->bytes_used += size; - return pa->data + idx; + *off = pa->bytes_used; + __system_property_area__->bytes_used += size; + return __system_property_area__->data + *off; } static prop_bt *new_prop_bt(const char *name, uint8_t namelen, prop_off_t *off) @@ -330,16 +295,10 @@ static prop_info *new_prop_info(const char *name, uint8_t namelen, static void *to_prop_obj(prop_off_t off) { - size_t region = off / PA_DATA_SIZE; - size_t idx = off % PA_DATA_SIZE; - - if (region > PA_REGION_COUNT) + if (off > PA_DATA_SIZE) return NULL; - if (map_prop_region(region) < 0) - return NULL; - - return __system_property_regions__[region]->data + idx; + return __system_property_area__->data + off; } static prop_bt *root_node() @@ -570,7 +529,7 @@ int __system_property_wait(const prop_info *pi) { unsigned n; if(pi == 0) { - prop_area *pa = __system_property_regions__[0]; + prop_area *pa = __system_property_area__; n = pa->serial; do { __futex_wait(&pa->serial, n, 0); @@ -586,7 +545,7 @@ int __system_property_wait(const prop_info *pi) int __system_property_update(prop_info *pi, const char *value, unsigned int len) { - prop_area *pa = __system_property_regions__[0]; + prop_area *pa = __system_property_area__; if (len >= PROP_VALUE_MAX) return -1; @@ -607,7 +566,7 @@ int __system_property_update(prop_info *pi, const char *value, unsigned int len) int __system_property_add(const char *name, unsigned int namelen, const char *value, unsigned int valuelen) { - prop_area *pa = __system_property_regions__[0]; + prop_area *pa = __system_property_area__; const prop_info *pi; if (namelen >= PROP_NAME_MAX) @@ -633,7 +592,7 @@ unsigned int __system_property_serial(const prop_info *pi) unsigned int __system_property_wait_any(unsigned int serial) { - prop_area *pa = __system_property_regions__[0]; + prop_area *pa = __system_property_area__; do { __futex_wait(&pa->serial, serial, 0); diff --git a/libc/include/sys/_system_properties.h b/libc/include/sys/_system_properties.h index 9c48e1b3e..92e35e124 100644 --- a/libc/include/sys/_system_properties.h +++ b/libc/include/sys/_system_properties.h @@ -42,11 +42,7 @@ typedef struct prop_msg prop_msg; #define PROP_SERVICE_NAME "property_service" #define PROP_FILENAME "/dev/__properties__" -/* (4 header words + 28 toc words) = 128 bytes */ -/* 128 bytes header and toc + 28 prop_infos @ 128 bytes = 3712 bytes */ - -#define PA_REGION_COUNT 128 -#define PA_SIZE 4096 +#define PA_SIZE (128 * 1024) #define SERIAL_VALUE_LEN(serial) ((serial) >> 24) #define SERIAL_DIRTY(serial) ((serial) & 1) diff --git a/tests/property_benchmark.cpp b/tests/property_benchmark.cpp index 7266bd08a..d10be915e 100644 --- a/tests/property_benchmark.cpp +++ b/tests/property_benchmark.cpp @@ -23,7 +23,7 @@ #include #include -extern void *__system_property_regions__[PA_REGION_COUNT]; +extern void *__system_property_area__; #define TEST_NUM_PROPS \ Arg(1)->Arg(4)->Arg(16)->Arg(64)->Arg(128)->Arg(256)->Arg(512)->Arg(1024) @@ -39,10 +39,8 @@ struct LocalPropertyTestState { return; } - for (size_t i = 0; i < PA_REGION_COUNT; i++) { - old_pa[i] = __system_property_regions__[i]; - __system_property_regions__[i] = NULL; - } + old_pa = __system_property_area__; + __system_property_area__ = NULL; pa_dirname = dirname; pa_filename = pa_dirname + "/__properties__"; @@ -79,9 +77,7 @@ struct LocalPropertyTestState { if (!valid) return; - for (size_t i = 0; i < PA_REGION_COUNT; i++) { - __system_property_regions__[i] = old_pa[i]; - } + __system_property_area__ = old_pa; __system_property_set_filename(PROP_FILENAME); unlink(pa_filename.c_str()); @@ -107,7 +103,7 @@ public: private: std::string pa_dirname; std::string pa_filename; - void *old_pa[PA_REGION_COUNT]; + void *old_pa; }; static void BM_property_get(int iters, int nprops) diff --git a/tests/system_properties_test.cpp b/tests/system_properties_test.cpp index 50bdfdfe4..96026074c 100644 --- a/tests/system_properties_test.cpp +++ b/tests/system_properties_test.cpp @@ -24,7 +24,7 @@ #define _REALLY_INCLUDE_SYS__SYSTEM_PROPERTIES_H_ #include -extern void *__system_property_regions__[PA_REGION_COUNT]; +extern void *__system_property_area__; struct LocalPropertyTestState { LocalPropertyTestState() : valid(false) { @@ -35,10 +35,8 @@ struct LocalPropertyTestState { return; } - for (size_t i = 0; i < PA_REGION_COUNT; i++) { - old_pa[i] = __system_property_regions__[i]; - __system_property_regions__[i] = NULL; - } + old_pa = __system_property_area__; + __system_property_area__ = NULL; pa_dirname = dirname; pa_filename = pa_dirname + "/__properties__"; @@ -52,9 +50,7 @@ struct LocalPropertyTestState { if (!valid) return; - for (size_t i = 0; i < PA_REGION_COUNT; i++) { - __system_property_regions__[i] = old_pa[i]; - } + __system_property_area__ = old_pa; __system_property_set_filename(PROP_FILENAME); unlink(pa_filename.c_str()); @@ -65,7 +61,7 @@ public: private: std::string pa_dirname; std::string pa_filename; - void *old_pa[PA_REGION_COUNT]; + void *old_pa; }; TEST(properties, add) { From 1d36ee1a6e69ec529a7c43a4fe6268f85bc5134a Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Sun, 16 Jun 2013 10:19:16 -0700 Subject: [PATCH 5/6] bionic: prevent root processes from calling __system_property_add If a root process other than init calls __system_property_add, which it should never do, it will break the design assumption that there is only one mutator. Pass O_EXCL to open() in map_prop_region_rw to ensure that only one process ever has the property pages open for write. (cherry picked from commit fb9b7b436f3ef94385f1b0c55ab81f246f0d96b8) Change-Id: I6df3afedbfb5d07891b095aa24b78278381a5aaf --- libc/bionic/system_properties.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/libc/bionic/system_properties.c b/libc/bionic/system_properties.c index d4054d245..f9671c636 100644 --- a/libc/bionic/system_properties.c +++ b/libc/bionic/system_properties.c @@ -128,11 +128,13 @@ static int map_prop_area_rw() { prop_area *pa; int fd; + int ret; /* dev is a tmpfs that we can use to carve a shared workspace * out of, so let's do that... */ - fd = open(property_filename, O_RDWR | O_CREAT | O_NOFOLLOW, 0644); + fd = open(property_filename, O_RDWR | O_CREAT | O_NOFOLLOW | O_CLOEXEC | + O_EXCL, 0444); if (fd < 0) { if (errno == EACCES) { /* for consistency with the case where the process has already @@ -143,6 +145,10 @@ static int map_prop_area_rw() return -1; } + ret = fcntl(fd, F_SETFD, FD_CLOEXEC); + if (ret < 0) + goto out; + if (ftruncate(fd, PA_SIZE) < 0) goto out; @@ -186,8 +192,16 @@ static int map_prop_area() { bool fromFile = true; int result = -1; + int fd; + int ret; - int fd = open(property_filename, O_RDONLY | O_NOFOLLOW); + fd = open(property_filename, O_RDONLY | O_NOFOLLOW | O_CLOEXEC); + if (fd >= 0) { + /* For old kernels that don't support O_CLOEXEC */ + ret = fcntl(fd, F_SETFD, FD_CLOEXEC); + if (ret < 0) + goto cleanup; + } if ((fd < 0) && (errno == ENOENT)) { /* From 836dbf65e4370df38cddc170229a7b0bdf882c8c Mon Sep 17 00:00:00 2001 From: Greg Hackmann Date: Fri, 21 Jun 2013 13:02:38 -0700 Subject: [PATCH 6/6] bionic: store property names as variable-length strings Names are immutable, so the fixed-sized arrays can be replaced with variable-length ones to save memory (especially on internal tree nodes). Signed-off-by: Greg Hackmann (cherry picked from commit 492ce95d9f6149137cb5b63c55cf2b3cdbe51e5e) Change-Id: Ib074192d1b71150233d78c58e9ffcf7ecf688b6b --- libc/bionic/system_properties.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/libc/bionic/system_properties.c b/libc/bionic/system_properties.c index f9671c636..481e6ae45 100644 --- a/libc/bionic/system_properties.c +++ b/libc/bionic/system_properties.c @@ -66,9 +66,9 @@ struct prop_area { typedef struct prop_area prop_area; struct prop_info { - char name[PROP_NAME_MAX]; unsigned volatile serial; char value[PROP_VALUE_MAX]; + char name[0]; }; typedef struct prop_info prop_info; @@ -92,7 +92,6 @@ typedef struct prop_info prop_info; typedef volatile uint32_t prop_off_t; struct prop_bt { - char name[PROP_NAME_MAX]; uint8_t namelen; uint8_t reserved[3]; @@ -102,6 +101,8 @@ struct prop_bt { prop_off_t right; prop_off_t children; + + char name[0]; }; typedef struct prop_bt prop_bt; @@ -277,7 +278,7 @@ static void *new_prop_obj(size_t size, prop_off_t *off) static prop_bt *new_prop_bt(const char *name, uint8_t namelen, prop_off_t *off) { prop_off_t off_tmp; - prop_bt *bt = new_prop_obj(sizeof(prop_bt), &off_tmp); + prop_bt *bt = new_prop_obj(sizeof(prop_bt) + namelen + 1, &off_tmp); if (bt) { memcpy(bt->name, name, namelen); bt->name[namelen] = '\0'; @@ -293,7 +294,7 @@ static prop_info *new_prop_info(const char *name, uint8_t namelen, const char *value, uint8_t valuelen, prop_off_t *off) { prop_off_t off_tmp; - prop_info *info = new_prop_obj(sizeof(prop_info), &off_tmp); + prop_info *info = new_prop_obj(sizeof(prop_info) + namelen + 1, &off_tmp); if (info) { memcpy(info->name, name, namelen); info->name[namelen] = '\0';