From d3c8d5b8d4ade074129b65199a8048c81089ee0e Mon Sep 17 00:00:00 2001
From: Jens Gulin <jens.gulin@sonymobile.com>
Date: Thu, 6 Mar 2014 18:15:43 +0100
Subject: 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
---
 libcutils/Android.mk  | 10 +++++++++
 libcutils/str_parms.c | 57 +++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 54 insertions(+), 13 deletions(-)

(limited to 'libcutils')

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
-- 
cgit v1.1