From c9036c00044a8d81561113886ecec9a9ce71bd3b Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Fri, 11 Jan 2019 19:38:25 +0100 Subject: [PATCH] BUG/MAJOR: cache: fix confusion between zero and uninitialized cache key The cache uses the first 32 bits of the uri's hash as the key to reference the object in the cache. It makes a special case of the value zero to mean that the object is not in the cache anymore. The problem is that when an object hashes as zero, it's still inserted but the eb32_delete() call is skipped, resulting in the object still being chained in the memory area while the block has been reclaimed and used for something else. Then when objects which were chained below it (techically any object since zero is at the root) are deleted, the walk through the upper object may encounter corrupted values where valid pointers were expected. But while this should only happen statically once on 4 billion, the problem gets worse when the cache-use conditions don't match the cache-store ones, because cache-store runs with an uninitialized key, which can create objects that will never be found by the lookup code, or worse, entries with a zero key preventing eviction of the tree node and resulting in a crash. It's easy to accidently end up on such a config because the request rules generally can't be used to decide on the response : http-request cache-use cache if { path_beg /images } http-response cache-store cache In this test, mixing traffic with /images/$RANDOM and /foo/$RANDOM will result in random keys being inserted, some of them possibly being zero, and crashes will quickly happen. The fix consists in 1) always initializing the transaction's cache_hash to zero, and 2) never storing a response for which the hash has not been calculated, as indicated by the value zero. It is worth noting that objects hashing as value zero will never be cached, but given that there's only one chance among 4 billion that this happens, this is totally harmless. This fix must be backported to 1.9 and 1.8. --- src/cache.c | 8 +++++++- src/proto_http.c | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/cache.c b/src/cache.c index a2d93758f..698395f15 100644 --- a/src/cache.c +++ b/src/cache.c @@ -646,6 +646,7 @@ enum act_return http_action_store_cache(struct act_rule *rule, struct proxy *px, struct shared_context *shctx = shctx_ptr(cconf->c.cache); struct cache_st *cache_ctx = NULL; struct cache_entry *object, *old; + unsigned int key = *(unsigned int *)txn->cache_hash; /* Don't cache if the response came from a cache */ if ((obj_type(s->target) == OBJ_TYPE_APPLET) && @@ -661,6 +662,10 @@ enum act_return http_action_store_cache(struct act_rule *rule, struct proxy *px, if (txn->meth != HTTP_METH_GET) goto out; + /* cache key was not computed */ + if (!key) + goto out; + /* cache only 200 status code */ if (txn->status != 200) goto out; @@ -800,7 +805,8 @@ enum act_return http_action_store_cache(struct act_rule *rule, struct proxy *px, if (cache_ctx) { cache_ctx->first_block = first; - object->eb.key = (*(unsigned int *)&txn->cache_hash); + object->eb.key = key; + memcpy(object->hash, txn->cache_hash, sizeof(object->hash)); /* Insert the node later on caching success */ diff --git a/src/proto_http.c b/src/proto_http.c index bf661ed44..25e0e3efd 100644 --- a/src/proto_http.c +++ b/src/proto_http.c @@ -7222,6 +7222,7 @@ void http_init_txn(struct stream *s) ? (TX_NOT_FIRST|TX_WAIT_NEXT_RQ) : 0); txn->status = -1; + *(unsigned int *)txn->cache_hash = 0; txn->cookie_first_date = 0; txn->cookie_last_date = 0;