[PATCH] Ignore response header entries with zero hash in ngx_http_varaible_headers

[Maxim Dounin]

Hello!

On Thu, Sep 29, 2011 at 12:06:25PM -0700, agentzh wrote:

> I've noticed that the ngx_http_variable_headers function in the nginx
> core does not ignore (response) header entries with zero "hash" field,
> which may result in incorrect values of nginx variables like
> $sent_http_cache_control and the attached patch for nginx 1.1.4
> (hopefully) fixes this issue.

It looks like this particular case affects 
$sent_http_cache_control only.

Though it looks like similar issue exists in 
ngx_http_variable_unknown_header(), it should be fixed 
too.  Quick look suggests in vanilla nginx it may affect 
$sent_http_accept_ranges, $sent_http_refresh, 
$sent_http_www_authenticate and probably other headers.

Could you please take a closer look and provide patch which fixes 
this one too?

And see below for review.

> 
> Thanks!
> -agentzh

> --- nginx-1.1.4/src/http/ngx_http_variables.c	2011-05-30 05:36:17.000000000 -0700
> +++ nginx-1.1.4-patched/src/http/ngx_http_variables.c	2011-09-29 11:57:07.000000000 -0700
> @@ -648,7 +648,14 @@
>  
>      a = (ngx_array_t *) ((char *) r + data);
>  
> -    n = a->nelts;
> +    h = a->elts;
> +    n = 0;
> +
> +    for (i = 0; i < a->nelts; i++) {
> +        if (h[i]->hash) {
> +            n++;
> +        }
> +    }

It would be good to follow same style of checking hash as in
ngx_http_header_filter_module.c, i.e.

    for (i = 0; i < a->nelts; i++) {

        if (h[i].hash == 0) {
            continue;
        }

        n++;
    }

>  
>      if (n == 0) {
>          v->not_found = 1;
> @@ -659,19 +666,12 @@
>      v->no_cacheable = 0;
>      v->not_found = 0;
>  
> -    h = a->elts;
> -
> -    if (n == 1) {
> -        v->len = (*h)->value.len;
> -        v->data = (*h)->value.data;
> -
> -        return NGX_OK;
> -    }
> -

While this is unlikely to be a hot path, preserving optimized case 
for n == 1 would be good, at least in case n == 1 && a->nelts == 1.

>      len = - (ssize_t) (sizeof("; ") - 1);
>  
> -    for (i = 0; i < n; i++) {
> -        len += h[i]->value.len + sizeof("; ") - 1;
> +    for (i = 0; i < a->nelts; i++) {
> +        if (h[i]->hash) {
> +            len += h[i]->value.len + sizeof("; ") - 1;
> +        }

Same as above,

    for (...) {

        if (h[i]->hash == 0) { 
            continue;
        }

        ...
    }

>      }
>  
>      p = ngx_pnalloc(r->pool, len);
> @@ -683,6 +683,10 @@
>      v->data = p;
>  
>      for (i = 0; /* void */ ; i++) {
> +        if (!h[i]->hash) {
> +            continue;
> +        }
> +

And here.

>          p = ngx_copy(p, h[i]->value.data, h[i]->value.len);
>  
>          if (i == n - 1) {

Maxim Dounin