Possible cache_bypass_bug

[Maxim Dounin]

Hello!

On Mon, Sep 26, 2011 at 11:10:07AM +1000, John Ferlito wrote:

> 
> Howdy,
> 
> I'd like to confirm what I think is a bug before I lodge it in the bug
> tracker.
> 
> I'm seeing pages that shouldn't be cached be cached.
> 
> I have a config that looks something like
> 
> 
>   upstream app_pool {
>     server app.inodes.org;
>     fair;
>   }
> 
>   proxy_cache_path  /srv/www/reverse_proxy/cache levels=1:2 keys_zone=application_cache:8m max_size=1000m inactive=600m;
>   proxy_temp_path   /srv/www/reverse_proxy/cache/tmp;
> 
>   server {
>     listen                  80;
>     server_name             inodes.org;
> 
>     location / {
>       proxy_pass         http://app_pool;
>       proxy_pass_header  Set-Cookie;
> 
>       proxy_cache        application_cache;
>       proxy_cache_key    "$scheme$host$request_uri$query_string";
> 
>       proxy_set_header   Host $host;
>       proxy_set_header   X-Forwarded-For $proxy_add_x_forwarded_for;
>       proxy_set_header   X-Real-IP $remote_addr;
> 
>       proxy_cache_valid  200 302 301 60m;
>       proxy_cache_valid  404         1m;
> 
>       proxy_cache_bypass $http_pragma;
>     }
>   }
> 
> 
> Now the behaviour I'm seeing is specifically for responses with a header like
> 
>   Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0
> 
> That is they shouldn't be cached. They only get cached after a request with
> 
>   Pragma: no-cache
> 
> set.
> 
> If I create a cache log I see something like this.
> 
> 
> MISS   "GET /mospace/team/ HTTP/1.1" (200) # First load
> BYPASS "GET /mospace/team/ HTTP/1.1" (200) # Shift Reload
> HIT    "GET /mospace/team/ HTTP/1.1" (200) # Standard load
> 
> I've done some following of the code path and I think I understand
> where the bug lies.
> 
> In ngx_http_upstream_cache.c we essentially have the following code (snipped
> for clarity)
> 
>   static void
>   ngx_http_upstream_send_response(ngx_http_request_t *r, ngx_http_upstream_t *u)
>   {
>     switch (ngx_http_test_predicates(r, u->conf->no_cache)) {
> 
>     case NGX_DECLINED:
>         u->cacheable = 0;
>         break;
> 
>     default: /* NGX_OK */
> 
>         if (u->cache_status == NGX_HTTP_CACHE_BYPASS) {
> 
>             r->cache->min_uses = u->conf->cache_min_uses;
>             r->cache->body_start = u->conf->buffer_size;
>             r->cache->file_cache = u->conf->cache->data;
> 
>             if (ngx_http_file_cache_create(r) != NGX_OK) {
>                 ngx_http_upstream_finalize_request(r, u, 0);
>                 return;
>             }
> 
>             u->cacheable = 1;
>         }
> 
>         break;
>     }
>   }
> 
> 
> So from my understanding if cache_bypass has been set we mark the page as being
> able to be cached.
> 
> Now this function is called in ngx_http_upstream_process_header, which does the
> following
> 
>     if (ngx_http_upstream_process_headers(r, u) != NGX_OK) {
>         return;
>     }
> 
>     if (!r->subrequest_in_memory) {
>         ngx_http_upstream_send_response(r, u);
>         return;
>     }
> 
> 
> From what I can tell ngx_http_upstream_process_headers will go through
> the headers and when it sees the appropriate Cache-Control header it
> will set cacheable to 0.
> 
> But later on in ngx_http_upstream_send_response we are setting it to
> 1. Hence we start caching private pages but only when a bypass header
> has been set.

Ack, this is clearly a bug.

> Perhaps we should set cacheable to 1 much earlier in the process when
> BYPASS is set? That is in ngx_http_upstream_cache
> 
>   switch (ngx_http_test_predicates(r, u->conf->cache_bypass)) {
>     // snip
>     case NGX_DECLINED:
>       u->cache_status = NGX_HTTP_CACHE_BYPASS;
>       u-> cachable = 1; // Add this here maybe?
>       return NGX_DECLINED;
>   }
> 
> This is the first time I've looked at the nginx source so perhaps the
> above would have some other unintended effect. If not let me know and
> I'll submit a patch.

As far as I see moving 

        u->cacheable = 1;

above the ngx_http_test_predicates(cache_bypass) check should be 
ok.

Obviously "u->cacheable = 1" should be removed from 
ngx_http_test_predicates(no_cache) switch.

Maxim Dounin