[PATCH 17 of 20] Upstream: handling of multiple Vary headers (ticket #1423)

[Maxim Dounin]

Hello!

On Thu, May 12, 2022 at 12:44:52AM +0400, Sergey Kandaurov wrote:

> On Thu, Apr 21, 2022 at 01:18:57AM +0300, Maxim Dounin wrote:
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1650492340 -10800
> > #      Thu Apr 21 01:05:40 2022 +0300
> > # Node ID 2027b85971d4b8a7e33c018548468057cb57eaf7
> > # Parent  f460a2f9f88d264ef6c8588eb37bcb85c48010db
> > Upstream: handling of multiple Vary headers (ticket #1423).
> > 
> > Previously, only the last header value was used when caching.
> > 
> > diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
> > --- a/src/http/ngx_http_upstream.c
> > +++ b/src/http/ngx_http_upstream.c
> > @@ -5175,6 +5175,9 @@ static ngx_int_t
> >  ngx_http_upstream_process_vary(ngx_http_request_t *r,
> >      ngx_table_elt_t *h, ngx_uint_t offset)
> >  {
> > +    u_char                *p;
> > +    size_t                 len;
> > +    ngx_str_t              vary;
> >      ngx_table_elt_t      **ph;
> >      ngx_http_upstream_t   *u;
> >  
> > @@ -5192,17 +5195,47 @@ ngx_http_upstream_process_vary(ngx_http_
> >          return NGX_OK;
> >      }
> >  
> > -    if (r->cache == NULL) {
> > +    if (r->cache == NULL || !u->cacheable) {
> > +        return NGX_OK;
> > +    }
> > +
> > +    if (h->value.len == 1 && h->value.data[0] == '*') {
> > +        u->cacheable = 0;
> >          return NGX_OK;
> >      }
> >  
> > -    if (h->value.len > NGX_HTTP_CACHE_VARY_LEN
> > -        || (h->value.len == 1 && h->value.data[0] == '*'))
> > -    {
> > +    if (u->headers_in.vary->next) {
> > +
> > +        len = 0;
> > +
> > +        for (h = u->headers_in.vary; h; h = h->next) {
> > +            len += h->value.len + 2;
> > +        }
> > +
> > +        len -= 2;
> > +
> > +        p = ngx_pnalloc(r->pool, len);
> > +        if (p == NULL) {
> > +            return NGX_ERROR;
> > +        }
> > +
> > +        vary.len = len;
> > +        vary.data = p;
> > +
> > +        for (h = u->headers_in.vary; h; h = h->next) {
> > +            p = ngx_copy(p, h->value.data, h->value.len);
> 
> (h->next == NULL) check missing

Thanks, fixed.

diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -5224,6 +5224,11 @@ ngx_http_upstream_process_vary(ngx_http_
 
         for (h = u->headers_in.vary; h; h = h->next) {
             p = ngx_copy(p, h->value.data, h->value.len);
+
+            if (h->next == NULL) {
+                break;
+            }
+
             *p++ = ','; *p++ = ' ';
         }
 


> 
> > +            *p++ = ','; *p++ = ' ';
> > +        }
> > +
> > +    } else {
> > +        vary = h->value;
> > +    }
> > +
> > +    if (vary.len > NGX_HTTP_CACHE_VARY_LEN) {
> >          u->cacheable = 0;
> >      }
> >  
> > -    r->cache->vary = h->value;
> > +    r->cache->vary = vary;
> >  
> >  #endif
> >  
> 
> Memory from the pool could be allocated just once among multiple values
> if postpone it to somewhere else (but probably it's not worth it).

Yes, I've considered it, yet it looks much more readable to 
preserve handling as is.

This shouldn't be an issue in valid cases, as there shouldn't 
more than a couple of Vary header lines.  And assuming an attempt 
to consume as much memory as it can, this will be something like 
(2 + 4 + 6 + ... + 128) = 32 * 130 = ~4k, which isn't significant.

[...]

-- 
Maxim Dounin
http://mdounin.ru/