[core] improve array API to prevent theoretical memory leaks

- refactor insert into array_find_or_insert; if the element already
  exists the caller must resolve the conflict manually:
  - array_replace frees the old element
  - array_insert_unique calls "insert_dup"
  both have no return value anymore
- fix usages of array_replace; they now don't need to delete the old
  entry anymore; usage in configparser was probably broken, as it
  possibly deleted the old element before calling array_replace

This should fix a lot of the issues reported in "Fortify Open Review
Project - lighttpd 1.4.39" (usually hitting the array limit):
when the array size limit was hit "new" entries leaked instead of
getting added.

On 32-bit INT_MAX entries cannot actually be reached (each entry
requires at least 48 bytes, leading to a total of 96GB memory).

On 64-bit INT_MAX entries would require 224GB memory, so it would be
theoretically possible. But it would need 2^27 reallocations of two
C-arrays of up to 16GB size.

From: Stefan Bühler <stbuehler@web.de>

git-svn-id: svn://svn.lighttpd.net/lighttpd/branches/lighttpd-1.4.x@3098 152afb58-edef-0310-8abb-c4023f1b3aa9

NEWS

@@ -27,6 +27,7 @@ NEWS
   * [mod_dirlisting] dir-listing.hide-dotfiles = "enabled" by default (fixes #1081)
   * [mod_secdownload] fix buffer overflow in secdl_verify_mac (reported by Fortify Open Review Project)
   * [mod_fastcgi,mod_scgi] fix leaking file-descriptor when backend spawning failed (reported by Fortify Open Review Project)
+  * [core] improve array API to prevent memory leaks
 
 - 1.4.39 - 2016-01-02
   * [core] fix memset_s call (fixes #2698)

src/array.c

@@ -71,6 +71,7 @@ void array_reset(array *a) {
 	}
 
 	a->used = 0;
+	a->unique_ndx = 0;
 }
 
 data_unset *array_pop(array *a) {
@@ -80,6 +81,7 @@ data_unset *array_pop(array *a) {
 
 	a->used --;
 	du = a->data[a->used];
+	force_assert(a->sorted[a->used] == a->used); /* only works on "simple" lists */
 	a->data[a->used] = NULL;
 
 	return du;
@@ -168,22 +170,8 @@ void array_set_key_value(array *hdrs, const char *key, size_t key_len, const cha
 	array_insert_unique(hdrs, (data_unset *)ds_dst);
 }
 
-/* replace or insert data, return the old one with the same key */
-data_unset *array_replace(array *a, data_unset *du) {
-	int ndx;
-
-	force_assert(NULL != du);
-	if (-1 == (ndx = array_get_index(a, CONST_BUF_LEN(du->key), NULL))) {
-		array_insert_unique(a, du);
-		return NULL;
-	} else {
-		data_unset *old = a->data[ndx];
-		a->data[ndx] = du;
-		return old;
-	}
-}
-
-int array_insert_unique(array *a, data_unset *str) {
+/* if entry already exists return pointer to existing entry, otherwise insert entry and return NULL */
+static data_unset **array_find_or_insert(array *a, data_unset *str) {
 	int ndx = -1;
 	int pos = 0;
 	size_t j;
@@ -192,25 +180,21 @@ int array_insert_unique(array *a, data_unset *str) {
 	if (buffer_is_empty(str->key) || str->is_index_key) {
 		buffer_copy_int(str->key, a->unique_ndx++);
 		str->is_index_key = 1;
+		force_assert(0 != a->unique_ndx); /* must not wrap or we'll get problems */
 	}
 
 	/* try to find the string */
 	if (-1 != (ndx = array_get_index(a, CONST_BUF_LEN(str->key), &pos))) {
-		/* found, leave here */
-		if (a->data[ndx]->type == str->type) {
-			str->insert_dup(a->data[ndx], str);
-		} else {
-			SEGFAULT();
-		}
-		return 0;
+		/* found collision, return it */
+		return &a->data[ndx];
 	}
 
 	/* insert */
 
-	if (a->used+1 > INT_MAX) {
-		/* we can't handle more then INT_MAX entries: see array_get_index() */
-		return -1;
-	}
+	/* there is no good error handling for hitting this limit
+	 * (we can't handle more then INT_MAX entries: see array_get_index())
+	 */
+	force_assert(a->used + 1 <= INT_MAX);
 
 	if (a->size == 0) {
 		a->size   = 16;
@@ -241,7 +225,7 @@ int array_insert_unique(array *a, data_unset *str) {
 		pos++;
 	}
 
-	/* move everything on step to the right */
+	/* move everything one step to the right */
 	if (pos != ndx) {
 		memmove(a->sorted + (pos + 1), a->sorted + (pos), (ndx - pos) * sizeof(*a->sorted));
 	}
@@ -251,7 +235,29 @@ int array_insert_unique(array *a, data_unset *str) {
 
 	if (a->next_power_of_2 == (size_t)ndx) a->next_power_of_2 <<= 1;
 
-	return 0;
+	return NULL;
+}
+
+/* replace or insert data (free existing entry) */
+void array_replace(array *a, data_unset *entry) {
+	data_unset **old;
+
+	force_assert(NULL != entry);
+	if (NULL != (old = array_find_or_insert(a, entry))) {
+		force_assert(*old != entry);
+		(*old)->free(*old);
+		*old = entry;
+	}
+}
+
+void array_insert_unique(array *a, data_unset *entry) {
+	data_unset **old;
+
+	force_assert(NULL != entry);
+	if (NULL != (old = array_find_or_insert(a, entry))) {
+		force_assert((*old)->type == entry->type);
+		entry->insert_dup(*old, entry);
+	}
 }
 
 void array_print_indent(int depth) {

src/array.h

@@ -162,13 +162,13 @@ array *array_init(void);
 array *array_init_array(array *a);
 void array_free(array *a);
 void array_reset(array *a);
-int array_insert_unique(array *a, data_unset *str);
-data_unset *array_pop(array *a);
+void array_insert_unique(array *a, data_unset *entry);
+data_unset *array_pop(array *a); /* only works on "simple" lists with autogenerated keys */
 int array_print(array *a, int depth);
 data_unset *array_get_unused_element(array *a, data_type_t t);
 data_unset *array_get_element(array *a, const char *key);
 void array_set_key_value(array *hdrs, const char *key, size_t key_len, const char *value, size_t val_len);
-data_unset *array_replace(array *a, data_unset *du);
+void array_replace(array *a, data_unset *entry);
 int array_strcasecmp(const char *a, size_t a_len, const char *b, size_t b_len);
 void array_print_indent(int depth);
 size_t array_get_max_key_length(array *a);

src/configfile.c

@@ -1195,7 +1195,6 @@ int config_read(server *srv, const char *fn) {
 		force_assert(NULL != prepends);
 		buffer_copy_buffer(prepends->key, modules->key);
 		array_replace(srv->config, (data_unset *)prepends);
-		modules->free((data_unset *)modules);
 		modules = prepends;
 
 		/* append default modules */

src/configparser.y

@@ -178,14 +178,23 @@ varline ::= key(A) APPEND expression(B). {
       ctx->ok = 0;
     } else if (NULL != (du = array_get_element(vars, A->ptr))) {
       /* exists in current block */
-      du = configparser_merge_data(du, B);
+      if (du->type != B->type) {
+        /* might create new data when merging different types; need to replace old array entry */
+        /* also merging will kill the old data */
+        du = du->copy(du);
+        du = configparser_merge_data(du, B);
+        if (NULL != du) {
+          buffer_copy_buffer(du->key, A);
+          array_replace(vars, du);
+        }
+      } else {
+        data_unset *old_du = du;
+        du = configparser_merge_data(old_du, B);
+        force_assert((NULL == du) || (du == old_du)); /* must not create new data when types match */
+      }
       if (NULL == du) {
         ctx->ok = 0;
       }
-      else {
-        buffer_copy_buffer(du->key, A);
-        array_replace(vars, du);
-      }
       B->free(B);
     } else if (NULL != (du = configparser_get_variable(ctx, A))) {
       du = configparser_merge_data(du, B);

src/mod_fastcgi.c

@@ -2256,8 +2256,7 @@ range_success: ;
 
 		buffer_copy_string_len(dcls->key, "Content-Length", sizeof("Content-Length")-1);
 		buffer_copy_int(dcls->value, sendfile2_content_length);
-		dcls = (data_string*) array_replace(con->response.headers, (data_unset *)dcls);
-		if (dcls) dcls->free((data_unset*)dcls);
+		array_replace(con->response.headers, (data_unset *)dcls);
 
 		con->parsed_response |= HTTP_CONTENT_LENGTH;
 		con->response.content_length = sendfile2_content_length;
@@ -2486,8 +2485,7 @@ static int fcgi_demux_response(server *srv, handler_ctx *hctx) {
 
 						buffer_copy_string_len(dcls->key, "Content-Length", sizeof("Content-Length")-1);
 						buffer_copy_int(dcls->value, sce->st.st_size);
-						dcls = (data_string*) array_replace(con->response.headers, (data_unset *)dcls);
-						if (dcls) dcls->free((data_unset*)dcls);
+						array_replace(con->response.headers, (data_unset *)dcls);
 
 						con->parsed_response |= HTTP_CONTENT_LENGTH;
 						con->response.content_length = sce->st.st_size;