httpd-41551

Version:  

httpd 2.2.4

Bug link:  

https://issues.apache.org/bugzilla/show_bug.cgi?id=41551

How it is diagnosed (reproduced or source analysis)?

We reproduced the failure. We simply used Apache as a mem cache (enabling worker+mod_mem_cache), and use it to cache & serve some small files. Periodically some files’ contents are messed up.

Symptom:

With mod_mem_cache, after run several minutes, they found the file content sent by mod_mem_cache are incorrect!!

The http headers are wrong, including wrong ETag value, and seems like memory overflow.

Root cause:

Concurrency bug.

During the caching process, apache needs to copy the response header received from the backend server into the cache object. Also it needs to recall those headers from cache later.  During this process, apache only copied the ‘pointer’ instead of the actual content, so later when the actual content (stored cache(=mobj) or  going out header(=req/response’s data structure)) got freed by another thread, the pointers would be dangling!

From change log : mod_mem_cache copy headers into longer lived storage;

header names and values could previously point to cleaned up storage.

This function is to retrieve headers from cache.

Another function, store_headers()  (which is also using apr_table_copy) is to store

headers (request and response) into cache (mobj->req_hdrs, mobj->header_out).

mod_mem_cache.c: 542

static apr_status_t recall_headers(cache_handle_t *h, request_rec *r)

{

    mem_cache_object_t *mobj = (mem_cache_object_t*) h->cache_obj->vobj;

    /* In the buggy version, apr_table_copy only copies the pointer, not the actual

         content pointed by the pointer. */

-    h->req_hdrs = apr_table_copy(r->pool, mobj->req_hdrs);

-    h->resp_hdrs = apr_table_copy(r->pool, mobj->header_out);

+    h->req_hdrs = deep_table_copy(r->pool, mobj->req_hdrs);

+    h->resp_hdrs = deep_table_copy(r->pool, mobj->header_out);

    return OK;

}

/* In the buggy version, only ‘nelts’ of ‘t->elts’ is copied. So in

* other words, the content of (char*) key and value are not copied! */

APR_DECLARE(apr_table_t *) apr_table_copy(apr_pool_t *p, const apr_table_t *t) {

   apr_table_t *new = apr_palloc(p, sizeof(apr_table_t));<---last write to new

  /* t->a.pool should have a life span at least as long as p*/

   make_array_core(&new->a, p, t->a.nalloc, sizeof(apr_table_entry_t), 0);

   memcpy(new->a.elts, t->a.elts, t->a.nelts * sizeof(apr_table_entry_t));

   new->a.nelts = t->a.nelts;

   memcpy(new->index_first, t->index_first, sizeof(int) * TABLE_HASH_SIZE);

   memcpy(new->index_last, t->index_last, sizeof(int) * TABLE_HASH_SIZE);

   new->index_initialized = t->index_initialized;

   return new;

}

APR_DECLARE(void) apr_table_add(apr_table_t *t, const char *key,

            const char *val)

{

   …  

   elts = (apr_table_entry_t *) table_push(t);  

   elts->key = apr_pstrdup(t->a.pool, key);

   elts->val = apr_pstrdup(t->a.pool, val);

}

APR_DECLARE(char *) apr_pstrndup(apr_pool_t *a, const char *s, apr_size_t n)

{

   res = apr_palloc(a, n + 1);

   memcpy(res, s, n);

/* table->elts[0].key, table->elts[0].value, table->elts[1].key, …

* all got copied into the copy and returned (eventually

* stored in h->resp_hdrs). Note ‘key’, ‘value’ here are all pointers!*/

+static apr_table_t *deep_table_copy(apr_pool_t *p, const apr_table_t *table)

+{

+    const apr_array_header_t *array = apr_table_elts(table);

+    apr_table_entry_t *elts = (apr_table_entry_t *) array->elts;

+    apr_table_t *copy = apr_table_make(p, array->nelts);

+    int i;

+

+    for (i = 0; i < array->nelts; i++) {

+        apr_table_add(copy, elts[i].key, elts[i].val);

         // key and value’s contend copied!

+    }

+

+    return copy;

+}

+

--------

cache_select ->

      recall_headers ->

Developers confused the functionality of apr_table_copy and misused that.

apr_table_copy allocates a new table and copies one table to the new table but the table keys and respective values are not copied. (The new->a.elts is array of *pointers* of key-value structure).  Only when calling apr_table_add, it allocates the key-value structure from a pool and insert the values.

Therefore, as long as the pool of t->a.pool are available (as long as the pointer is valid), apr_table_copy works correctly, but if later the original pool becomes unavailable, the new table would point garbage data.

* @param p The pool to allocate the new table out of

 * @param t The table to copy

 * @return A copy of the table passed in

+ * @warning The table keys and respective values are not copied

 */

APR_DECLARE(apr_table_t *) apr_table_copy(apr_pool_t *p,

                                          const apr_table_t *t);

Fix:

Look at above patch.

They implemented a new function, apr_deep_table_copy, which directly calls apr_table_add one by one for key-value pairs.

Is there Error Message?

No

Can Errlog anticipate error and put a magic error message?

No. The control-flow is also normal (no cold paths). The only problem is with the data content!