On Sun, Feb 01, 2004 at 01:30:56AM +0000, Pier Fumagalli wrote:
> I found a small bug in mod_disk_cache...
>
> CacheMinFileSize and CacheMaxFileSize process the argument with atoi(),
> which simply means that they'll get -1 if the value is quite large (I
> need to cache some big files)...
>
> At the same time, I added a small feature I needed: the ability to
> cache content from sources where content-length is not specified
> (namely, friggin' JSPs).
Strange that you found that at the same time as I did. Your patch is
missing the following:
* it must check CacheMinFileSize and CacheMaxFileSize when the
response is complete. That may mean that we are checking at two places:
-> when the C-L is known, we check before we cache
-> when it is unknown we check "on-the-way" for CacheMaxFileSize, and
after caching completion for CacheMinFileSize
* it *may* compare the actual size of the cached copy against the C-L
header, and discard it if the two don't match
Also, in spite of RFC2616 14.8, we currently *never* cache
authenticated responses, even if the RFC would allow for it.
The auth check can be taken out of mod_cache,
the check in mod_disk_cache replaces it (but I have not checked
whether mod_mem_cache checks auth as well -- it should, or mod_cache
should try and analyze the headers_out as well).
The patch (completely independent from yours) tries to address that.
IMO the two should be merged (I am on my way home, so I'm not doing
the merge right now).
Martin
--
<[EMAIL PROTECTED]> | Fujitsu Siemens
Fon: +49-89-636-46021, FAX: +49-89-636-47655 | 81730 Munich, Germany
Index: modules/experimental/mod_cache.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_cache.c,v
retrieving revision 1.79
diff -u -r1.79 mod_cache.c
--- modules/experimental/mod_cache.c 1 Jan 2004 13:26:18 -0000 1.79
+++ modules/experimental/mod_cache.c 4 Feb 2004 18:24:37 -0000
@@ -159,6 +159,11 @@
* - RFC2616 14.9.2 Cache-Control: no-store
* - Pragma: no-cache
* - Any requests requiring authorization.
+ * (exceptions as per RFC2616 14.8 Authorization:
+ * Cache-Control _response_ header includes "public",
+ * "must-revalidate" or "s-maxage"; we are going to test
+ * that in the actual filter because here we haven't read
+ * the response headers yet).
*/
if (conf->ignorecachecontrol == 1 && auth == NULL) {
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
@@ -167,7 +172,7 @@
}
else {
if (ap_cache_liststr(NULL, cc_in, "no-store", NULL) ||
- ap_cache_liststr(NULL, pragma, "no-cache", NULL) || (auth != NULL)) {
+ ap_cache_liststr(NULL, pragma, "no-cache", NULL)) {
/* delete the previously cached file */
cache_remove_url(r, cache->types, url);
Index: modules/experimental/mod_disk_cache.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_disk_cache.c,v
retrieving revision 1.49
diff -u -r1.49 mod_disk_cache.c
--- modules/experimental/mod_disk_cache.c 1 Jan 2004 13:26:18 -0000 1.49
+++ modules/experimental/mod_disk_cache.c 4 Feb 2004 18:24:39 -0000
@@ -203,10 +203,25 @@
apr_file_close(dobj->fd);
dobj->fd = NULL;
- /* XXX log */
- }
+ /* XXX log */
+ }
+
+ return APR_SUCCESS;
+}
+
+static apr_status_t file_cache_errorcleanup(disk_cache_object_t *dobj, request_rec *r)
+{
+ if (dobj->fd) {
+ apr_file_close(dobj->fd);
+ dobj->fd = NULL;
+ }
+ /* Remove the header file, the temporary body file, and a potential old body file
*/
+ apr_file_remove(dobj->hdrsfile, r->pool);
+ apr_file_remove(dobj->tempfile, r->pool);
+ apr_file_remove(dobj->datafile, r->pool);
- return APR_SUCCESS;
+ /* Return non-APR_SUCCESS in order to have mod_cache remove the disk_cache filter
*/
+ return DECLINED;
}
@@ -337,7 +352,8 @@
return DECLINED;
}
- if (len < conf->minfs || len > conf->maxfs) {
+ /* If the Content-Length is still unknown, cache anyway */
+ if (len != -1 && (len < conf->minfs || len > conf->maxfs)) {
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
"cache_disk: URL %s failed the size check, "
"or is incomplete",
@@ -665,11 +681,14 @@
"disk_cache: Caching headers for URL %s", dobj->name);
return APR_SUCCESS;
}
+
static apr_status_t write_body(cache_handle_t *h, request_rec *r, apr_bucket_brigade
*b)
{
apr_bucket *e;
apr_status_t rv;
disk_cache_object_t *dobj = (disk_cache_object_t *) h->cache_obj->vobj;
+ disk_cache_conf *conf = ap_get_module_config(r->server->module_config,
+ &disk_cache_module);
if (!dobj->fd) {
rv = apr_file_open(&dobj->fd, dobj->tempfile,
@@ -678,6 +697,7 @@
if (rv != APR_SUCCESS) {
return rv;
}
+ dobj->file_size = 0;
}
for (e = APR_BRIGADE_FIRST(b);
e != APR_BRIGADE_SENTINEL(b);
@@ -686,10 +706,49 @@
const char *str;
apr_size_t length;
apr_bucket_read(e, &str, &length, APR_BLOCK_READ);
- apr_file_write(dobj->fd, str, &length);
+ if (apr_file_write(dobj->fd, str, &length) != APR_SUCCESS) {
+ ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
+ "cache_disk: Error when writing cache file for URL %s",
+ h->cache_obj->key);
+ /* Remove the intermediate cache file and return non-APR_SUCCESS */
+ return file_cache_errorcleanup(dobj, r);
+ }
+ dobj->file_size += length;
+ if (dobj->file_size > conf->maxfs) {
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
+ "cache_disk: URL %s failed the size check (%lu>%lu)",
+ h->cache_obj->key, (unsigned long)dobj->file_size, (unsigned
long)conf->maxfs);
+ /* Remove the intermediate cache file and return non-APR_SUCCESS */
+ return file_cache_errorcleanup(dobj, r);
+ }
}
+
+ /* Was this the final bucket? If yes, close the body file and make sanity checks
*/
if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(b))) {
- file_cache_el_final(h, r); /* Link to the perm file, and close the
descriptor */
+ if (h->cache_obj->info.len <= 0) {
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
+ "disk_cache: Cached body has %lu bytes", (unsigned
long)dobj->file_size);
+ h->cache_obj->info.len = dobj->file_size;
+ }
+ else if (h->cache_obj->info.len != dobj->file_size) {
+ /* "Content-Length" and actual content disagree in size. Log that. */
+ ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
+ "disk_cache: URL %s failed the size check (%lu != %lu)",
+ h->cache_obj->key,
+ (unsigned long)h->cache_obj->info.len,
+ (unsigned long)dobj->file_size);
+ /* Remove the intermediate cache file and return non-APR_SUCCESS */
+ return file_cache_errorcleanup(dobj, r);
+ }
+ if (dobj->file_size < conf->minfs) {
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
+ "cache_disk: URL %s failed the size check (%lu<%lu)",
+ h->cache_obj->key, (unsigned long)dobj->file_size, (unsigned
long)conf->minfs);
+ /* Remove the intermediate cache file and return non-APR_SUCCESS */
+ return file_cache_errorcleanup(dobj, r);
+ }
+ /* All checks were fine. Move tempfile to final destination */
+ file_cache_el_final(h, r); /* Link to the perm file, and close the
descriptor */
ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
"disk_cache: Cached body for URL %s", dobj->name);
}
