Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit d3c8d5b8 authored by Jens Gulin's avatar Jens Gulin Committed by Elliott Hughes
Browse files

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
parent 223fc42b

libcutils/Android.mk

+10 −0
Original line number Diff line number Diff line
@@ -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

# ========================================================

libcutils/str_parms.c

+44 −13
Original line number Diff line number Diff line
@@ -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);

    if (tmp_val == NULL) {

        goto clean_up;

    }



    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;

    }



    if (old_val) {

        free(old_val);

        free(tmp_key);

    } else if (errno == ENOMEM) {

clean_up:

    free(tmp_key);

    free(tmp_val);

        return -ENOMEM;

    }

    return 0;

    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