diff options
author | Jens Gulin <jens.gulin@sonymobile.com> | 2014-03-06 18:15:43 +0100 |
---|---|---|
committer | Elliott Hughes <enh@google.com> | 2014-04-03 11:19:54 -0700 |
commit | d3c8d5b8d4ade074129b65199a8048c81089ee0e (patch) | |
tree | ed11e4123581fdf4b625235a6fc9d5c2729791ff /libcutils | |
parent | 223fc42b5e289e882f67c893374ffbef595a6901 (diff) | |
download | system_core-d3c8d5b8d4ade074129b65199a8048c81089ee0e.zip system_core-d3c8d5b8d4ade074129b65199a8048c81089ee0e.tar.gz system_core-d3c8d5b8d4ade074129b65199a8048c81089ee0e.tar.bz2 |
Handle errno properly to avoid corrupt str_parms
A normal sequence of calls is as follows:
str_parms_create_str, str_parms_add_str, str_parms_destroy.
In some cases the destroy caused double free.
str_parms_add_str will clone the input and send it to hashmapPut
for storage. If hashmapPut did not store the strings it will raise
errno = ENOMEM and leave caller with ownership of the strings.
In any of these cases it will be safe to destroy the str_parms.
But what if it wasn't hashmapPut that said NOMEM? What if there
was a stale NOMEM already before a successful hashmapPut?
In that case the strings will be successfully added to the list
(if new), but when str_parms_add_str sees the NOMEM it will free
them anyway, leaving dangling pointers in the str_parms!!
It is the responsibility of the caller to clear errno before any
interesting call. This patch makes sure that str_parms_add_str
reacts only on errno emmitted from hashmapPut.
Change-Id: If87e4bcc482f09e1c66133d33517b152ebdac65f
Diffstat (limited to 'libcutils')
-rw-r--r-- | libcutils/Android.mk | 10 | ||||
-rw-r--r-- | libcutils/str_parms.c | 57 |
2 files changed, 54 insertions, 13 deletions
diff --git a/libcutils/Android.mk b/libcutils/Android.mk index 93bccb0..635695a 100644 --- a/libcutils/Android.mk +++ b/libcutils/Android.mk @@ -91,6 +91,16 @@ LOCAL_STATIC_LIBRARIES := lib64log LOCAL_CFLAGS += $(hostSmpFlag) -m64 include $(BUILD_HOST_STATIC_LIBRARY) +# Tests for host +# ======================================================== +include $(CLEAR_VARS) +LOCAL_MODULE := tst_str_parms +LOCAL_CFLAGS += -DTEST_STR_PARMS +LOCAL_SRC_FILES := str_parms.c hashmap.c memory.c +LOCAL_STATIC_LIBRARIES := liblog +LOCAL_MODULE_TAGS := optional +include $(BUILD_HOST_EXECUTABLE) + # Shared and static library for target # ======================================================== diff --git a/libcutils/str_parms.c b/libcutils/str_parms.c index 7cfbcb3..953b91d 100644 --- a/libcutils/str_parms.c +++ b/libcutils/str_parms.c @@ -194,23 +194,46 @@ err_create_str_parms: int str_parms_add_str(struct str_parms *str_parms, const char *key, const char *value) { - void *old_val; - void *tmp_key; - void *tmp_val; + void *tmp_key = NULL; + void *tmp_val = NULL; + void *old_val = NULL; + + // strdup and hashmapPut both set errno on failure. + // Set errno to 0 so we can recognize whether anything went wrong. + int saved_errno = errno; + errno = 0; tmp_key = strdup(key); + if (tmp_key == NULL) { + goto clean_up; + } + tmp_val = strdup(value); - old_val = hashmapPut(str_parms->map, tmp_key, tmp_val); + if (tmp_val == NULL) { + goto clean_up; + } - if (old_val) { - free(old_val); - free(tmp_key); - } else if (errno == ENOMEM) { - free(tmp_key); - free(tmp_val); - return -ENOMEM; + old_val = hashmapPut(str_parms->map, tmp_key, tmp_val); + if (old_val == NULL) { + // Did hashmapPut fail? + if (errno == ENOMEM) { + goto clean_up; + } + // For new keys, hashmap takes ownership of tmp_key and tmp_val. + tmp_key = tmp_val = NULL; + } else { + // For existing keys, hashmap takes ownership of tmp_val. + // (It also gives up ownership of old_val.) + tmp_val = NULL; } - return 0; + +clean_up: + free(tmp_key); + free(tmp_val); + free(old_val); + int result = -errno; + errno = saved_errno; + return result; } int str_parms_add_int(struct str_parms *str_parms, const char *key, int value) @@ -337,7 +360,6 @@ static void test_str_parms_str(const char *str) { struct str_parms *str_parms; char *out_str; - int ret; str_parms = str_parms_create_str(str); str_parms_add_str(str_parms, "dude", "woah"); @@ -370,6 +392,15 @@ int main(void) test_str_parms_str("foo=bar;baz=bat;"); test_str_parms_str("foo=bar;baz=bat;foo=bar"); + // hashmapPut reports errors by setting errno to ENOMEM. + // Test that we're not confused by running in an environment where this is already true. + errno = ENOMEM; + test_str_parms_str("foo=bar;baz="); + if (errno != ENOMEM) { + abort(); + } + test_str_parms_str("foo=bar;baz="); + return 0; } #endif |