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@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/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org