Bug 44579 - mod_cache doesn't handle If-Range correctly
Summary: mod_cache doesn't handle If-Range correctly
Status: RESOLVED FIXED
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_cache (show other bugs)
Version: 2.2.8
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: FixedInTrunk, PatchAvailable
Depends on:
Blocks:
 
Reported: 2008-03-11 08:16 UTC by moog
Modified: 2009-10-03 07:59 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description moog 2008-03-11 08:16:13 UTC
Use mod_cache and mod_disk_cache.
Suppose an object is in the cache, but expired, and the backend has a newer version of the object.
Request the object partially, using Range and If-Range, with an If-Range time equal to the timestamp of the object in the cache.

Since the object has expired, mod_cache revalidates with the backend and retrieves the newer version, deleting the old version from the cache.

According to the specification of If-Range in RFC2616, the server should deliver the whole of the new version of the object, but instead it delivers only the part of it specified in Range.

If the cached object is made current or deleted, If-Range requests are handled correctly, until the backend version is updated again.

Looking towards the end of cache_save_filter in mod_cache.c:  

        /* Restore the original request headers and see if we need to
         * return anything else than the cached response (ie. the original
         * request was conditional).
         */
        r->headers_in = cache->stale_headers;
        status = ap_meets_conditions(r);
        if (status != OK) {
            r->status = status;

            bkt = apr_bucket_flush_create(bb->bucket_alloc);
            APR_BRIGADE_INSERT_TAIL(bb, bkt);
        }
        else {
            cache->provider->recall_body(cache->handle, r->pool, bb);
        }

ap_meets_conditions always returns OK for an If-Range request, so in the situation described above, the If-Range request gets treated like an unconditional request.
Comment 1 Ruediger Pluem 2008-03-11 13:54:36 UTC
Can you please provide a request and the response headers of the corresponding response  to this request where you get only the specified range of the entity instead of the whole one?
Comment 2 moog 2008-03-11 17:16:47 UTC
After ensuring doc.txt was in the cache and expired, I updated it with
"date >doc.txt" then requested a range of it:

GET /doc.txt HTTP/1.1
Host: 127.0.0.1
Range: bytes=10-20
If-Range: Tue, 11 Mar 2008 23:57:41 GMT

HTTP/1.1 206 Partial Content
Date: Wed, 12 Mar 2008 00:04:26 GMT
Server: Apache/2.2.8 (Unix)
Last-Modified: Wed, 12 Mar 2008 00:03:22 GMT
ETag: "310314-1d-448322a4b8680"
Accept-Ranges: bytes
Content-Length: 11
Content-Range: bytes 10-20/29
Content-Type: text/plain

 00:03:22 G
Comment 3 Ruediger Pluem 2008-03-12 01:54:04 UTC
Does the following patch fix your problem?

Index: modules/cache/mod_cache.c
===================================================================
--- modules/cache/mod_cache.c   (revision 634179)
+++ modules/cache/mod_cache.c   (working copy)
@@ -613,6 +613,12 @@
             cache->provider->remove_entity(cache->stale_handle);
             /* Treat the request as if it wasn't conditional. */
             cache->stale_handle = NULL;
+            /*
+             * Restore the original request headers as they may be needed
+             * by further output filters like the byterange filter to make
+             * the correct decisions.
+             */
+            r->headers_in = cache->stale_headers;
         }
     }
Comment 4 moog 2008-03-12 03:45:22 UTC
Yes, your patch fixes the problem, thanks very much!
Comment 5 Ruediger Pluem 2008-03-12 09:08:12 UTC
Committed to trunk as r636386 (http://svn.apache.org/viewvc?view=rev&revision=636386).
Comment 6 moog 2008-03-12 12:11:43 UTC
Sorry, spoke too soon... although your patch fixes the problem when the backend is the local disk, it has no effect when the backend is another server reverse-proxied with mod_proxy.

GET /proxied/date.txt HTTP/1.1
Host: 127.0.0.1
Range: bytes=10-20
If-Range: Wed, 12 Mar 2008 18:48:57 GMT

causes Apache to make the following request to the backend:

GET /store/date.txt HTTP/1.1
Host: [backend]
Range: bytes=10-20
If-None-Match: "3e61762-1d-44841e3b1d840"
If-Modified-Since: Wed, 12 Mar 2008 18:48:57 GMT
X-Forwarded-For: 127.0.0.1
X-Forwarded-Host: 127.0.0.1
X-Forwarded-Server: box
Connection: Keep-Alive

and since the backend has updated content, it delivers the new content, but only part of it, because that's what it was asked for:

HTTP/1.1 206 Partial Content
Date: Wed, 12 Mar 2008 18:51:00 GMT
Server: Apache/2.2.3 (Debian) mod_ssl/2.2.3 OpenSSL/0.9.8c WebAuth/3.5.3
Last-Modified: Wed, 12 Mar 2008 18:49:59 GMT
ETag: "3e61762-1d-44841e763e3c0"
Accept-Ranges: bytes
Content-Length: 11
Content-Range: bytes 10-20/29
Keep-Alive: timeout=15, max=100
Connection: Keep-Alive
Content-Type: text/plain; charset=ISO-8859-1

 18:49:59 G

Then Apache delivers this partial content back to the client, not respecting the If-Range request:

HTTP/1.1 206 Partial Content
Date: Wed, 12 Mar 2008 18:51:00 GMT
Server: Apache/2.2.3 (Debian) mod_ssl/2.2.3 OpenSSL/0.9.8c WebAuth/3.5.3
Last-Modified: Wed, 12 Mar 2008 18:49:59 GMT
ETag: "3e61762-1d-44841e763e3c0"
Accept-Ranges: bytes
Content-Length: 11
Content-Range: bytes 10-20/29
Content-Type: text/plain; charset=ISO-8859-1

 18:49:59 G

Perhaps when a client makes an If-Range request, Apache too needs to make an If-Range request to the backend, rather than an If-Modified-Since, so that it will retrieve the full content when it's needed.
Comment 7 Ruediger Pluem 2008-03-12 15:02:15 UTC
(In reply to comment #6)
> Sorry, spoke too soon... although your patch fixes the problem when the backend
> is the local disk, it has no effect when the backend is another server
> reverse-proxied with mod_proxy.

Can you please check if the following patch fixes your problem:

Index: modules/cache/cache_storage.c
===================================================================
--- modules/cache/cache_storage.c       (Revision 636503)
+++ modules/cache/cache_storage.c       (Arbeitskopie)
@@ -283,9 +283,18 @@
                 apr_table_unset(r->headers_in, "If-Match");
                 apr_table_unset(r->headers_in, "If-Modified-Since");
                 apr_table_unset(r->headers_in, "If-None-Match");
-                apr_table_unset(r->headers_in, "If-Range");
                 apr_table_unset(r->headers_in, "If-Unmodified-Since");

+                /*
+                 * If we have an If-Range header in the original request, we
+                 * also need to remove a possible Range header as otherwise
+                 * we might get back a Range response where we shouldn't.
+                 */
+                if (apr_table_get(r->headers_in, "If-Range")) {
+                    apr_table_unset(r->headers_in, "Range");
+                }
+                apr_table_unset(r->headers_in, "If-Range");
+
                 etag = apr_table_get(h->resp_hdrs, "ETag");
                 lastmod = apr_table_get(h->resp_hdrs, "Last-Modified");



Comment 8 Ruediger Pluem 2008-03-12 15:13:13 UTC
Better check the following patch:

Index: modules/cache/cache_storage.c
===================================================================
--- modules/cache/cache_storage.c       (Revision 636503)
+++ modules/cache/cache_storage.c       (Arbeitskopie)
@@ -286,6 +286,12 @@
                 apr_table_unset(r->headers_in, "If-Range");
                 apr_table_unset(r->headers_in, "If-Unmodified-Since");

+                /* Do not do Range requests with our own conditionals: If
+                 * we get 304 the Range does not matter and otherwise the
+                 * entity changed and we want to have the complete entity
+                 */
+                apr_table_unset(r->headers_in, "Range");
+
                 etag = apr_table_get(h->resp_hdrs, "ETag");
                 lastmod = apr_table_get(h->resp_hdrs, "Last-Modified");


I made up my mind and in case we set our own conditionals for the request to the backend a Range request makes no sense for the reason stated in the comment.
Comment 9 moog 2008-03-12 17:25:13 UTC
> /* Do not do Range requests with our own conditionals: If
>  * we get 304 the Range does not matter and otherwise the
>  * entity changed and we want to have the complete entity

Good idea.  I've applied this third patch, and not the first two, and in all the cases mentioned earlier I now get the correct behaviour.  Thanks again.
Comment 10 Ruediger Pluem 2008-03-13 00:28:03 UTC
(In reply to comment #9)
> > /* Do not do Range requests with our own conditionals: If
> >  * we get 304 the Range does not matter and otherwise the
> >  * entity changed and we want to have the complete entity
> 
> Good idea.  I've applied this third patch, and not the first two, and in all
> the cases mentioned earlier I now get the correct behaviour.  Thanks again.
> 

The first patch is nevertheless correct. I now applied the third one to trunk as well as r636653  (http://svn.apache.org/viewvc?view=rev&revision=636653). Thanks for testing.
Comment 11 moog 2008-03-13 04:58:16 UTC
(In reply to comment #10)
> The first patch is nevertheless correct. I now applied the third one to trunk

I'm now using both the first and third patches (r636386 and r636653) and I'm getting the correct behaviour.  Thanks.
Comment 12 Ruediger Pluem 2008-05-26 13:04:04 UTC
Proposed for backport to 2.2.x as r660284 (http://svn.apache.org/viewvc?rev=660284&view=rev).
Comment 13 Graham Leggett 2009-10-03 07:59:31 UTC
Backported to v2.2 and released in v2.2.9.