Commit 098357b0 authored by Jim Jagielski's avatar Jim Jagielski
Browse files

Merge r1721899 from trunk: 

mod_cache_socache: Fix a possible cached entity body corruption when it
is received from an origin server in multiple batches and forwarded by
mod_proxy.

Upstream buckets should be setaside when saving response body (store_body),
but since those will finally be flatten in the cache buffer (commit_entity),
let's save them directly into the buffer to avoid heap allocation(s) and
the final copy.

Reported by: Mike Pastore <mike oobak.org> 

Submitted by: ylavic
Reviewed/backported by: jim


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1731082 13f79535-47bb-0310-9956-ffa450edef68
parent 7fe649be

CHANGES

+5 −0
Original line number Diff line number Diff line 
                                                         -*- coding: utf-8 -*-



Changes with Apache 2.4.19



  *) mod_cache_socache: Fix a possible cached entity body corruption when it

     is received from an origin server in multiple batches and forwarded by

     mod_proxy.  [Yann Ylavic]



  *) core: Add expression support to SetHandler.

     [Eric Covener]

STATUS

+0 −7
Original line number Diff line number Diff line
@@ -112,13 +112,6 @@ RELEASE SHOWSTOPPERS: 
PATCHES ACCEPTED TO BACKPORT FROM TRUNK:

  [ start all new proposals below, under PATCHES PROPOSED. ]



  *) mod_cache_socache: Fix a possible cached entity body corruption when it

     is received from an origin server in multiple batches and forwarded by

     mod_proxy.

     trunk patch: http://svn.apache.org/r1721899

     2.4.x patch: trunk works, module CHANGES

     +1: ylavic, jim, rpluem



  *) mod_watchdog: Error logging

     trunk patch:  http://svn.apache.org/r1722154

                   http://svn.apache.org/r1722701

modules/cache/mod_cache_socache.c

+22 −50
Original line number Diff line number Diff line
@@ -76,12 +76,12 @@ typedef struct cache_socache_object_t 
    apr_table_t *headers_out; /* Output headers to save */

    cache_socache_info_t socache_info; /* Header information. */

    apr_size_t body_offset; /* offset to the start of the body */

    apr_off_t body_length; /* length of the cached entity body */

    unsigned int newbody :1; /* whether a new body is present */

    apr_time_t expire; /* when to expire the entry */



    const char *name; /* Requested URI without vary bits - suitable for mortals. */

    const char *key; /* On-disk prefix; URI with Vary bits (if present) */

    apr_off_t file_size; /*  File size of the cached data file  */

    apr_off_t offset; /* Max size to set aside */

    apr_time_t timeout; /* Max time to set aside */

    unsigned int done :1; /* Is the attempt to cache complete? */
@@ -462,8 +462,8 @@ static int open_entity(cache_handle_t *h, request_rec *r, const char *key) 
     */

    apr_pool_create(&sobj->pool, r->pool);



    sobj->buffer = apr_palloc(sobj->pool, dconf->max + 1);

    sobj->buffer_len = dconf->max + 1;

    sobj->buffer = apr_palloc(sobj->pool, dconf->max);

    sobj->buffer_len = dconf->max;



    /* attempt to retrieve the cached entry */

    if (socache_mutex) {
@@ -953,13 +953,7 @@ static apr_status_t store_body(cache_handle_t *h, request_rec *r, 
    }



    if (!sobj->newbody) {

        if (sobj->body) {

            apr_brigade_cleanup(sobj->body);

        }

        else {

            sobj->body = apr_brigade_create(r->pool,

                    r->connection->bucket_alloc);

        }

        sobj->body_length = 0;

        sobj->newbody = 1;

    }

    if (sobj->offset) {
@@ -1021,27 +1015,19 @@ static apr_status_t store_body(cache_handle_t *h, request_rec *r, 
            continue;

        }



        sobj->file_size += length;

        if (sobj->file_size >= sobj->buffer_len - sobj->body_offset) {

        sobj->body_length += length;

        if (sobj->body_length >= sobj->buffer_len - sobj->body_offset) {

            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02378)

                    "URL %s failed the buffer size check "

                    "(%" APR_OFF_T_FMT ">=%" APR_SIZE_T_FMT ")",

                    h->cache_obj->key, sobj->file_size, sobj->buffer_len - sobj->body_offset);

                    h->cache_obj->key, sobj->body_length,

                    sobj->buffer_len - sobj->body_offset);

            apr_pool_destroy(sobj->pool);

            sobj->pool = NULL;

            return APR_EGENERAL;

        }



        rv = apr_bucket_copy(e, &e);

        if (rv != APR_SUCCESS) {

            ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(02379)

                    "Error when copying bucket for URL %s",

                    h->cache_obj->key);

            apr_pool_destroy(sobj->pool);

            sobj->pool = NULL;

            return rv;

        }

        APR_BRIGADE_INSERT_TAIL(sobj->body, e);

        memcpy(sobj->buffer + sobj->body_offset + sobj->body_length - length,

               str, length);



        /* have we reached the limit of how much we're prepared to write in one

         * go? If so, leave, we'll get called again. This prevents us from trying
@@ -1075,8 +1061,10 @@ static apr_status_t store_body(cache_handle_t *h, request_rec *r, 
            return APR_EGENERAL;

        }

        if (cl_header) {

            apr_int64_t cl = apr_atoi64(cl_header);

            if ((errno == 0) && (sobj->file_size != cl)) {

            apr_off_t cl;

            char *cl_endp;

            if (apr_strtoff(&cl, cl_header, &cl_endp, 10) != APR_SUCCESS

                    || *cl_endp != '\0' || cl != sobj->body_length) {

                ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02381)

                        "URL %s didn't receive complete response, not caching",

                        h->cache_obj->key);
@@ -1100,24 +1088,6 @@ static apr_status_t commit_entity(cache_handle_t *h, request_rec *r) 
    cache_object_t *obj = h->cache_obj;

    cache_socache_object_t *sobj = (cache_socache_object_t *) obj->vobj;

    apr_status_t rv;

    apr_size_t len;



    /* flatten the body into the buffer */

    len = sobj->buffer_len - sobj->body_offset;

    rv = apr_brigade_flatten(sobj->body, (char *) sobj->buffer

            + sobj->body_offset, &len);

    if (APR_SUCCESS != rv) {

        ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(02382)

                "could not flatten brigade, not caching: %s",

                sobj->key);

        goto fail;

    }

    if (len >= sobj->buffer_len - sobj->body_offset) {

        ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(02383)

                "body too big for the cache buffer, not caching: %s",

                h->cache_obj->key);

        goto fail;

    }



    if (socache_mutex) {

        apr_status_t status = apr_global_mutex_lock(socache_mutex);
@@ -1126,13 +1096,13 @@ static apr_status_t commit_entity(cache_handle_t *h, request_rec *r) 
                    "could not acquire lock, ignoring: %s", obj->key);

            apr_pool_destroy(sobj->pool);

            sobj->pool = NULL;

            return rv;

            return status;

        }

    }

    rv = conf->provider->socache_provider->store(

            conf->provider->socache_instance, r->server,

            (unsigned char *) sobj->key, strlen(sobj->key), sobj->expire,

            sobj->buffer, (unsigned int) sobj->body_offset + len, sobj->pool);

            sobj->buffer, sobj->body_offset + sobj->body_length, sobj->pool);

    if (socache_mutex) {

        apr_status_t status = apr_global_mutex_unlock(socache_mutex);

        if (status != APR_SUCCESS) {
@@ -1140,7 +1110,7 @@ static apr_status_t commit_entity(cache_handle_t *h, request_rec *r) 
                    "could not release lock, ignoring: %s", obj->key);

            apr_pool_destroy(sobj->pool);

            sobj->pool = NULL;

            return DECLINED;

            return status;

        }

    }

    if (rv != APR_SUCCESS) {
@@ -1290,9 +1260,11 @@ static const char *set_cache_max(cmd_parms *parms, void *in_struct_ptr, 
{

    cache_socache_dir_conf *dconf = (cache_socache_dir_conf *) in_struct_ptr;



    if (apr_strtoff(&dconf->max, arg, NULL, 10) != APR_SUCCESS || dconf->max

            < 1024) {

        return "CacheSocacheMaxSize argument must be a integer representing the max size of a cached entry (headers and body), at least 1024";

    if (apr_strtoff(&dconf->max, arg, NULL, 10) != APR_SUCCESS

            || dconf->max < 1024 || dconf->max > APR_UINT32_MAX) {

        return "CacheSocacheMaxSize argument must be a integer representing "

               "the max size of a cached entry (headers and body), at least 1024 "

               "and at most " APR_STRINGIFY(APR_UINT32_MAX);

    }

    dconf->max_set = 1;

    return NULL;