From: Willy Tarreau <w@1wt.eu>
Date: Fri, 6 Apr 2018 17:02:25 +0000 (+0200)
Subject: BUG/MAJOR: cache: always initialize newly created objects
X-Git-Tag: v1.9-dev1~310
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1093a4586c58f3d4eb327c768472990e8b95fd95;p=thirdparty%2Fhaproxy.git

BUG/MAJOR: cache: always initialize newly created objects

Recent commit 5bd37fa ("BUG/MAJOR: cache: fix random crashes caused by
incorrect delete() on non-first blocks") addressed an issue where
dangling objects could be deleted in the cache, but even after this fix
some similar segfaults were reported at the same place (cache_free_blocks()).
The tree was always corrupted as well. Placing some traces revealed
that this time it's caused by a missing initialization in
http_action_store_cache() : while object->eb.key is used to note that
the object is not in the tree, the first retrieved block may contain
random data and is not initialized. Further, this entry can be updated
later without the object being inserted into the tree. Thus, if at the
end the object is not stored and the blocks are put back to the avail
list, the next attempt to use them will find eb.key != 0 and will try
to delete the uninitialized block, will see that eb.node.leaf_p is not
NULL (random data), and will dereference it as well as a few other
uninitialized pointers. It was harder to trigger than the previous one,
despite being very closely related. This time the following config
was used :

  listen l1
    mode http
    bind :8888
    http-request cache-use c1
    http-response cache-store c1
    server s1 127.0.0.1:8000

  cache c1
    total-max-size 4
    max-age 10

Httpterm was running on port 8000.

And it was stressed this way :

  $ inject -o 1 -u 500 -P 1 -G '127.0.0.1:8888/?s=4097&p=1&x=%s'
  ... wait 5 seconds then Ctrl-C ...
  # wait 3 seconds doing nothing
  $ inject -o 1 -u 500 -P 1 -G '127.0.0.1:8888/?s=4097&p=1&x=%s'
  => segfault

Other values don't work well. The size and the small pieces in the
responses (p=1) are critical to make it work.

Here the fix consists in pre-zeroing object->eb.key AND object->eb.leaf_p
just after the object is allocated so as to stay consistent with other
locations. Ideally this could be simplified later by only relying on
eb->node.leaf_p everywhere since in the end the key alone is not a
reliable indicator, so that we use only one indicator of being part of
the tree or not.

This fix needs to be backported to 1.8.
---

diff --git a/src/cache.c b/src/cache.c
index 864857c3f3..39e0bad44d 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -446,6 +446,13 @@ enum act_return http_action_store_cache(struct act_rule *rule, struct proxy *px,
 	}
 	shctx_unlock(shctx);
 
+	/* the received memory is not initialized, we need at least to mark
+	 * the object as not indexed yet.
+	 */
+	object = (struct cache_entry *)first->data;
+	object->eb.node.leaf_p = NULL;
+	object->eb.key = 0;
+
 	/* reserve space for the cache_entry structure */
 	first->len = sizeof(struct cache_entry);
 
@@ -470,7 +477,6 @@ enum act_return http_action_store_cache(struct act_rule *rule, struct proxy *px,
 					struct cache_entry *old;
 
 					cache_ctx->first_block = first;
-					object = (struct cache_entry *)first->data;
 
 					object->eb.key = (*(unsigned int *)&txn->cache_hash);
 					memcpy(object->hash, txn->cache_hash, sizeof(object->hash));
@@ -497,8 +503,6 @@ enum act_return http_action_store_cache(struct act_rule *rule, struct proxy *px,
 out:
 	/* if does not cache */
 	if (first) {
-		object = (struct cache_entry *)first->data;
-
 		shctx_lock(shctx);
 		first->len = 0;
 		object->eb.key = 0;