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.
This commit is contained in:
Willy Tarreau 2019-01-11 19:38:25 +01:00
parent 762475e1f9
commit c9036c0004
2 changed files with 8 additions and 1 deletions

View File

@ -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 */

View File

@ -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;