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
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 @@
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);
- tmp_val = strdup(value);
- old_val = hashmapPut(str_parms->map, tmp_key, tmp_val);
-
- if (old_val) {
- free(old_val);
- free(tmp_key);
- } else if (errno == ENOMEM) {
- free(tmp_key);
- free(tmp_val);
- return -ENOMEM;
+ if (tmp_key == NULL) {
+ goto clean_up;
}
- return 0;
+
+ 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;
+ }
+
+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 @@
{
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 @@
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