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

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel