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

[Maxim Dounin]

Hello!

On Sat, Dec 08, 2012 at 11:45:17PM -0800, Piotr Sikora wrote:

> 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.

Ignoring non-rewrite-directives in all-but-last matched if() 
is known (and event somewhat documented) behaviour.  While it's 
indeed counter-intuitive (and that's why I specifically outlined it 
as of one the examples of the "if" directive wierdness on the 
http://wiki.nginx.org/IfIsEvil page), it is well defined.

Changing the behaviour to silently ignore arbitrary matched if() 
block is certainly a regression, even compared to a SIGSEGV - 
which is bad, but logged and clearly indicates something is wrong 
(where "something" is unsupported configuration using if() in this 
case).

> 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?

No, as it breaks number of configs which rely on content handlers 
not being inherited, e.g.

   location / {
       proxy_pass http://backend;

       location ~ \.jpg$ {
           # static
       }
   }

(Your patch also makes things bad by introducing inheritance for 
some content handlers but not others, but that's another story 
anyway.)

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.

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

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

2) Postpone ngx_http_update_location_config() till the end of the 
script code execution, thus making configs like

    if ($true) { proxy_pass ... }
    if ($true) { add_header 2 ... }

consistent with

    if ($true) { add_header 1 ... }
    if ($true) { add_header 2 ... }

via ignoring anything but the last matched if() block.  This 
aproach needs careful investigation though, as I'm not really sure 
the won't be any undesired side effects (in particular, some 
limit_rate uses will no longer work, but probably we can live with 
it).

3) Introduce inheritance for content handlers into if blocks only 
(similar to limit_except blocks? likely needs cleanup too though), 
and always overwrite r->content_handler in 
ngx_http_update_location_config().  Net effect should be similar 
to (2), and much like (2) it also neeeds careful investigation.

-- 
Maxim Dounin
http://nginx.com/support.html