From 647644a65013f52457f37af1de2ad4b7345973bf Mon Sep 17 00:00:00 2001 From: Graham Leggett Date: Tue, 28 May 2013 21:12:19 +0000 Subject: [PATCH] mod_cache: Ensure that updated responses to HEAD requests don't get mistakenly paired with a previously cached body. Ensure that any existing body is removed when a HEAD request is cached. trunk patch: http://svn.apache.org/r1479411 Submitted by: minfrin Reviewed by: jim, wrowe git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1487122 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES | 5 + STATUS | 7 -- modules/cache/mod_cache.c | 7 +- modules/cache/mod_cache_disk.c | 148 ++++++++++++++++-------------- modules/cache/mod_cache_socache.c | 8 +- 5 files changed, 96 insertions(+), 79 deletions(-) diff --git a/CHANGES b/CHANGES index f6d5ffd8ac..ffca0d791d 100644 --- a/CHANGES +++ b/CHANGES @@ -2,6 +2,11 @@ Changes with Apache 2.4.5 + *) mod_cache: Ensure that updated responses to HEAD requests don't get + mistakenly paired with a previously cached body. Ensure that any existing + body is removed when a HEAD request is cached. [Graham Leggett, + Co-Advisor ] + *) mod_cache: Honour Cache-Control: no-store in a request. [Graham Leggett] *) mod_cache: Make sure that contradictory entity headers present in a 304 diff --git a/STATUS b/STATUS index cc920ea9cc..999826c716 100644 --- a/STATUS +++ b/STATUS @@ -90,13 +90,6 @@ RELEASE SHOWSTOPPERS: PATCHES ACCEPTED TO BACKPORT FROM TRUNK: [ start all new proposals below, under PATCHES PROPOSED. ] - * mod_cache: Ensure that updated responses to HEAD requests don't get - mistakenly paired with a previously cached body. Ensure that any existing - body is removed when a HEAD request is cached. - trunk patch: http://svn.apache.org/r1479411 - 2.4.x patch: trunk patch works (minus CHANGES) - +1: minfrin, jim, wrowe - * core: Add the ability to do explicit matching on weak and strong ETags as per RFC2616 Section 13.3.3. trunk patch: http://svn.apache.org/r1479528 diff --git a/modules/cache/mod_cache.c b/modules/cache/mod_cache.c index 9258ebd564..f47eeae587 100644 --- a/modules/cache/mod_cache.c +++ b/modules/cache/mod_cache.c @@ -1435,9 +1435,6 @@ static apr_status_t cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in) /* We found a stale entry which wasn't really stale. */ if (cache->stale_handle) { - /* Load in the saved status and clear the status line. */ - r->status = info->status; - r->status_line = NULL; /* RFC 2616 10.3.5 states that entity headers are not supposed * to be in the 304 response. Therefore, we need to combine the @@ -1476,6 +1473,10 @@ static apr_status_t cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in) apr_bucket *bkt; int status; + /* Load in the saved status and clear the status line. */ + r->status = info->status; + r->status_line = NULL; + /* We're just saving response headers, so we are done. Commit * the response at this point, unless there was a previous error. */ diff --git a/modules/cache/mod_cache_disk.c b/modules/cache/mod_cache_disk.c index b0d86fe1ff..a5699482ae 100644 --- a/modules/cache/mod_cache_disk.c +++ b/modules/cache/mod_cache_disk.c @@ -940,6 +940,10 @@ static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info dobj->headers_in = ap_cache_cacheable_headers_in(r); } + if (r->header_only && r->status != HTTP_NOT_MODIFIED) { + dobj->disk_info.header_only = 1; + } + return APR_SUCCESS; } @@ -1188,49 +1192,51 @@ static apr_status_t store_body(cache_handle_t *h, request_rec *r, continue; } - /* Attempt to create the data file at the last possible moment, if - * the body is empty, we don't write a file at all, and save an inode. - */ - if (!dobj->data.tempfd) { - apr_finfo_t finfo; - rv = apr_file_mktemp(&dobj->data.tempfd, dobj->data.tempfile, - APR_CREATE | APR_WRITE | APR_BINARY | - APR_BUFFERED | APR_EXCL, dobj->data.pool); + if (!dobj->disk_info.header_only) { + + /* Attempt to create the data file at the last possible moment, if + * the body is empty, we don't write a file at all, and save an inode. + */ + if (!dobj->data.tempfd) { + apr_finfo_t finfo; + rv = apr_file_mktemp(&dobj->data.tempfd, dobj->data.tempfile, + APR_CREATE | APR_WRITE | APR_BINARY | APR_BUFFERED + | APR_EXCL, dobj->data.pool); + if (rv != APR_SUCCESS) { + apr_pool_destroy(dobj->data.pool); + return rv; + } + dobj->file_size = 0; + rv = apr_file_info_get(&finfo, APR_FINFO_IDENT, + dobj->data.tempfd); + if (rv != APR_SUCCESS) { + apr_pool_destroy(dobj->data.pool); + return rv; + } + dobj->disk_info.device = finfo.device; + dobj->disk_info.inode = finfo.inode; + dobj->disk_info.has_body = 1; + } + + /* write to the cache, leave if we fail */ + rv = apr_file_write_full(dobj->data.tempfd, str, length, &written); if (rv != APR_SUCCESS) { + ap_log_rerror( + APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(00731) "Error when writing cache file for URL %s", h->cache_obj->key); + /* Remove the intermediate cache file and return non-APR_SUCCESS */ apr_pool_destroy(dobj->data.pool); return rv; } - dobj->file_size = 0; - rv = apr_file_info_get(&finfo, APR_FINFO_IDENT, - dobj->data.tempfd); - if (rv != APR_SUCCESS) { + dobj->file_size += written; + if (dobj->file_size > dconf->maxfs) { + ap_log_rerror( + APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00732) "URL %s failed the size check " + "(%" APR_OFF_T_FMT ">%" APR_OFF_T_FMT ")", h->cache_obj->key, dobj->file_size, dconf->maxfs); + /* Remove the intermediate cache file and return non-APR_SUCCESS */ apr_pool_destroy(dobj->data.pool); - return rv; + return APR_EGENERAL; } - dobj->disk_info.device = finfo.device; - dobj->disk_info.inode = finfo.inode; - dobj->disk_info.has_body = 1; - } - /* write to the cache, leave if we fail */ - rv = apr_file_write_full(dobj->data.tempfd, str, length, &written); - if (rv != APR_SUCCESS) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(00731) - "Error when writing cache file for URL %s", - h->cache_obj->key); - /* Remove the intermediate cache file and return non-APR_SUCCESS */ - apr_pool_destroy(dobj->data.pool); - return rv; - } - dobj->file_size += written; - if (dobj->file_size > dconf->maxfs) { - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00732) - "URL %s failed the size check " - "(%" APR_OFF_T_FMT ">%" APR_OFF_T_FMT ")", - h->cache_obj->key, dobj->file_size, dconf->maxfs); - /* Remove the intermediate cache file and return non-APR_SUCCESS */ - apr_pool_destroy(dobj->data.pool); - return APR_EGENERAL; } /* have we reached the limit of how much we're prepared to write in one @@ -1256,43 +1262,44 @@ static apr_status_t store_body(cache_handle_t *h, request_rec *r, if (seen_eos) { const char *cl_header = apr_table_get(r->headers_out, "Content-Length"); - if (dobj->data.tempfd) { - rv = apr_file_close(dobj->data.tempfd); - if (rv != APR_SUCCESS) { - /* Buffered write failed, abandon attempt to write */ - apr_pool_destroy(dobj->data.pool); - return rv; + if (!dobj->disk_info.header_only) { + + if (dobj->data.tempfd) { + rv = apr_file_close(dobj->data.tempfd); + if (rv != APR_SUCCESS) { + /* Buffered write failed, abandon attempt to write */ + apr_pool_destroy(dobj->data.pool); + return rv; + } } - } - if (r->connection->aborted || r->no_cache) { - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00733) - "Discarding body for URL %s " - "because connection has been aborted.", - h->cache_obj->key); - /* Remove the intermediate cache file and return non-APR_SUCCESS */ - apr_pool_destroy(dobj->data.pool); - return APR_EGENERAL; - } - if (dobj->file_size < dconf->minfs) { - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00734) - "URL %s failed the size check " - "(%" APR_OFF_T_FMT "<%" APR_OFF_T_FMT ")", - h->cache_obj->key, dobj->file_size, dconf->minfs); - /* Remove the intermediate cache file and return non-APR_SUCCESS */ - apr_pool_destroy(dobj->data.pool); - return APR_EGENERAL; - } - if (cl_header) { - apr_int64_t cl = apr_atoi64(cl_header); - if ((errno == 0) && (dobj->file_size != cl)) { - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00735) - "URL %s didn't receive complete response, not caching", - h->cache_obj->key); + if (r->connection->aborted || r->no_cache) { + ap_log_rerror( + APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00733) "Discarding body for URL %s " + "because connection has been aborted.", h->cache_obj->key); /* Remove the intermediate cache file and return non-APR_SUCCESS */ apr_pool_destroy(dobj->data.pool); return APR_EGENERAL; } + if (dobj->file_size < dconf->minfs) { + ap_log_rerror( + APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00734) "URL %s failed the size check " + "(%" APR_OFF_T_FMT "<%" APR_OFF_T_FMT ")", h->cache_obj->key, dobj->file_size, dconf->minfs); + /* Remove the intermediate cache file and return non-APR_SUCCESS */ + apr_pool_destroy(dobj->data.pool); + return APR_EGENERAL; + } + if (cl_header) { + apr_int64_t cl = apr_atoi64(cl_header); + if ((errno == 0) && (dobj->file_size != cl)) { + ap_log_rerror( + APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00735) "URL %s didn't receive complete response, not caching", h->cache_obj->key); + /* Remove the intermediate cache file and return non-APR_SUCCESS */ + apr_pool_destroy(dobj->data.pool); + return APR_EGENERAL; + } + } + } /* All checks were fine, we're good to go when the commit comes */ @@ -1319,7 +1326,12 @@ static apr_status_t commit_entity(cache_handle_t *h, request_rec *r) rv = file_cache_el_final(conf, &dobj->vary, r); } if (APR_SUCCESS == rv) { - rv = file_cache_el_final(conf, &dobj->data, r); + if (!dobj->disk_info.header_only) { + rv = file_cache_el_final(conf, &dobj->data, r); + } + else if (dobj->data.file){ + rv = apr_file_remove(dobj->data.file, dobj->data.pool); + } } /* remove the cached items completely on any failure */ diff --git a/modules/cache/mod_cache_socache.c b/modules/cache/mod_cache_socache.c index fb3fea948a..e6a4c4d992 100644 --- a/modules/cache/mod_cache_socache.c +++ b/modules/cache/mod_cache_socache.c @@ -879,7 +879,13 @@ static apr_status_t store_headers(cache_handle_t *h, request_rec *r, socache_info->request_time = obj->info.request_time; socache_info->response_time = obj->info.response_time; socache_info->status = obj->info.status; - socache_info->header_only = r->header_only; + + if (r->header_only && r->status != HTTP_NOT_MODIFIED) { + socache_info->header_only = 1; + } + else { + socache_info->header_only = sobj->socache_info.header_only; + } socache_info->name_len = strlen(sobj->name); -- 2.40.0