[PATCH] Core: fix content handler and location configuration mismatch.

Piotr Sikora piotr at cloudflare.com
Sun Dec 9 07:45:17 UTC 2012


Hey Maxim,

> And what will happen with the configuration specified in the "if
> ($slow) { ... }" as in the config above?

It will get ignored.

> For limit_rate your config will work as it's saved in request by
> ngx_http_update_location_config()

Correct.

> but e.g.
>
>
>     if ($slow) {
>         add_header "X-Accel-Limit-Rate: 10k";
>     }
>
> will silently ignore if() match, which is wrong.

So you're saying that ignoring if() match (by design and only location
configuration, rewrites and setting variables will work just fine) is
worse than crashing worker process? Sorry, but I'm not buying that.

Anyway, even without this patch, location configurations (other than
the last match) are always silently ignored when there is more than a
single if() match, e.g.:

    location / {
        set $pass  1;
        set $slow  1;

        proxy_pass  http://127.0.0.1:8000;

        if ($pass) {
            add_header "X-Passed" "True";
        }

        if ($slow) {
            add_header "X-Accel-Limit-Rate" "10k";
        }
    }

will silently ignore first if() match, which - quoting you - "is
wrong", so this isn't regression, just different behavior.

Alternatively, the current behavior can be retained by inheriting
content handler defined in the location block (if there is any).
Attached patch makes sure that the current behavior doesn't change,
but also takes care of the mentioned SIGSEGV case, in which case it
will use the location configuration from the last matched if()
pseudo-location that had content handler defined instead of
mismatching content handler and location configuration from different
if() pseudo-locations and crashing... Does this make more sense to
you?

Best regards,
Piotr Sikora


Core: fix content handler and location configuration mismatch.

When using exclusive content handlers (proxy_pass, etc.) and "if"
pseudo-locations, content handler was always set to the one from the
last "if" pseudo-location that evaluated to true or to the one from
the location block if "if" pseudo-locations didn't provide any.
At the same time, location configuration was updated for each "if"
pseudo-location that evaluated to true, which resulted in content
handler and location configuration mismatch.

For example, following configuration would result in SIGSEGV, because
"proxy_pass" content handler (set in the "if ($pass)" pseudo-location)
would be called with location configuration from the "if ($slow)"
pseudo-location which doesn't contain any upstream configuration.

    location / {
        set $pass  1;
        set $slow  1;

        if ($pass) {
            proxy_pass  http://127.0.0.1:8000;
        }

        if ($slow) {
            limit_rate  10k;
        }
    }

This patch fixes this issue by keeping track of and restoring location
configuration from which the content handler originated.

Signed-off-by: Piotr Sikora <piotr at cloudflare.com>
---
 src/http/modules/ngx_http_fastcgi_module.c | 2 +-
 src/http/modules/ngx_http_proxy_module.c   | 2 +-
 src/http/modules/ngx_http_scgi_module.c    | 2 +-
 src/http/modules/ngx_http_uwsgi_module.c   | 2 +-
 src/http/ngx_http_core_module.c            | 2 ++
 src/http/ngx_http_request.h                | 1 +
 6 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/http/modules/ngx_http_fastcgi_module.c
b/src/http/modules/ngx_http_fastcgi_module.c
index f1917e2..039e9b4 100644
--- a/src/http/modules/ngx_http_fastcgi_module.c
+++ b/src/http/modules/ngx_http_fastcgi_module.c
@@ -2414,7 +2414,7 @@ ngx_http_fastcgi_merge_loc_conf(ngx_conf_t *cf,
void *parent, void *child)

     if (conf->upstream.upstream || conf->fastcgi_lengths) {
         clcf = ngx_http_conf_get_module_loc_conf(cf, ngx_http_core_module);
-        if (clcf->handler == NULL && clcf->lmt_excpt) {
+        if (clcf->handler == NULL) {
             clcf->handler = ngx_http_fastcgi_handler;
         }
     }
diff --git a/src/http/modules/ngx_http_proxy_module.c
b/src/http/modules/ngx_http_proxy_module.c
index 541c59f..6c65970 100644
--- a/src/http/modules/ngx_http_proxy_module.c
+++ b/src/http/modules/ngx_http_proxy_module.c
@@ -2768,7 +2768,7 @@ ngx_http_proxy_merge_loc_conf(ngx_conf_t *cf,
void *parent, void *child)

     if (conf->upstream.upstream || conf->proxy_lengths) {
         clcf = ngx_http_conf_get_module_loc_conf(cf, ngx_http_core_module);
-        if (clcf->handler == NULL && clcf->lmt_excpt) {
+        if (clcf->handler == NULL) {
             clcf->handler = ngx_http_proxy_handler;
             conf->location = prev->location;
         }
diff --git a/src/http/modules/ngx_http_scgi_module.c
b/src/http/modules/ngx_http_scgi_module.c
index 5a3af92..5fdefc0 100644
--- a/src/http/modules/ngx_http_scgi_module.c
+++ b/src/http/modules/ngx_http_scgi_module.c
@@ -1358,7 +1358,7 @@ ngx_http_scgi_merge_loc_conf(ngx_conf_t *cf,
void *parent, void *child)

     if (conf->upstream.upstream || conf->scgi_lengths) {
         clcf = ngx_http_conf_get_module_loc_conf(cf, ngx_http_core_module);
-        if (clcf->handler == NULL && clcf->lmt_excpt) {
+        if (clcf->handler == NULL) {
             clcf->handler = ngx_http_scgi_handler;
         }
     }
diff --git a/src/http/modules/ngx_http_uwsgi_module.c
b/src/http/modules/ngx_http_uwsgi_module.c
index 0c6414c..f2ce278 100644
--- a/src/http/modules/ngx_http_uwsgi_module.c
+++ b/src/http/modules/ngx_http_uwsgi_module.c
@@ -1397,7 +1397,7 @@ ngx_http_uwsgi_merge_loc_conf(ngx_conf_t *cf,
void *parent, void *child)

     if (conf->upstream.upstream || conf->uwsgi_lengths) {
         clcf = ngx_http_conf_get_module_loc_conf(cf, ngx_http_core_module);
-        if (clcf->handler == NULL && clcf->lmt_excpt) {
+        if (clcf->handler == NULL) {
             clcf->handler = ngx_http_uwsgi_handler;
         }
     }
diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c
index a7db74b..a9213d2 100644
--- a/src/http/ngx_http_core_module.c
+++ b/src/http/ngx_http_core_module.c
@@ -1400,6 +1400,7 @@ ngx_http_core_content_phase(ngx_http_request_t *r,
     ngx_str_t  path;

     if (r->content_handler) {
+        r->loc_conf = r->content_handler_loc_conf;
         r->write_event_handler = ngx_http_request_empty_handler;
         ngx_http_finalize_request(r, r->content_handler(r));
         return NGX_OK;
@@ -1526,6 +1527,7 @@ ngx_http_update_location_config(ngx_http_request_t *r)

     if (clcf->handler) {
         r->content_handler = clcf->handler;
+        r->content_handler_loc_conf = r->loc_conf;
     }
 }

diff --git a/src/http/ngx_http_request.h b/src/http/ngx_http_request.h
index f234840..97c9607 100644
--- a/src/http/ngx_http_request.h
+++ b/src/http/ngx_http_request.h
@@ -358,6 +358,7 @@ struct ngx_http_request_s {
     void                            **main_conf;
     void                            **srv_conf;
     void                            **loc_conf;
+    void                            **content_handler_loc_conf;

     ngx_http_event_handler_pt         read_event_handler;
     ngx_http_event_handler_pt         write_event_handler;
-- 
1.8.0.1



More information about the nginx-devel mailing list