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

[Piotr Sikora]

Hey Maxim,

> Aproach I would prefer to use to resolve this is already outlined
> at the IfIsEvil page: I would like to keep declarative config and
> imprative rewrite module directives separate, that is, disable all
> non-rewrite-module directives inside if()'s.  That's the way we
> are slowly moving along, in particular, via introducing variables
> support in various places.

I agree with you 100% on that :)

Having said that, removing "if" support from that many directives will
definitely break a lot of setups, so I didn't even bother with working
on a patch that would do that...

> If you can't wait and want to fix this particular SIGSIGV, here
> are some possible aproaches I will be happy enough with:

I can wait, this isn't an issue for any of my existing code or
configurations... I've just noticed the mismatch while working on
ngx_cache_purge-2.0 and I found it terribly wrong, so I wanted to get
it fixed ASAP.

> 1) Just log error and return 500 if configuration incosistency
> detected instead of dereferencing a NULL pointer.

Attached, hopefully this meets your requirements ;)
Feel free to change the error message.

Best regards,
Piotr Sikora


Core: detect content handler and location configuration mismatch.

When using exclusive content handlers (proxy_pass, etc.) and "if"
pseudo-locations, content handler is always set to the one from the
last "if" pseudo-location that evaluates to true or to the one from
the location block if "if" pseudo-locations don't provide any.
At the same time, location configuration is updated for each "if"
pseudo-location that evaluates to true, which results 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 detects this mismatch by keeping track of the location
configuration from which the content handler originated and returns
"500 Internal Server Error" response instead of dereferencing NULL
pointer and crashing worker process.

Signed-off-by: Piotr Sikora <piotr at cloudflare.com>
---
 src/http/ngx_http_core_module.c | 9 +++++++++
 src/http/ngx_http_request.h     | 1 +
 2 files changed, 10 insertions(+)

diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c
index a7db74b..b48175b 100644
--- a/src/http/ngx_http_core_module.c
+++ b/src/http/ngx_http_core_module.c
@@ -1400,6 +1400,14 @@ ngx_http_core_content_phase(ngx_http_request_t *r,
     ngx_str_t  path;

     if (r->content_handler) {
+        if (r->loc_conf != r->content_handler_loc_conf) {
+            ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
+                          "content handler and configuration mismatch");
+
+            ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
+            return NGX_OK;
+        }
+
         r->write_event_handler = ngx_http_request_empty_handler;
         ngx_http_finalize_request(r, r->content_handler(r));
         return NGX_OK;
@@ -1526,6 +1534,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;