[PATCH 1 of 1] HTTP: Fix infinite loop on NGX_DECLINED in ngx_http_finalize_request

[Maxim Dounin]

Hello!

On Fri, Sep 13, 2024 at 06:11:31PM -0400, Will Hawkins wrote:

> On Fri, Sep 13, 2024 at 11:32 AM Maxim Dounin <mdounin at mdounin.ru> wrote:
> >
> > Hello!
> >
> > On Fri, Sep 13, 2024 at 01:20:34AM -0400, Will Hawkins wrote:
> >
> > > # HG changeset patch
> > > # User Will Hawkins <hawkinsw at obs.cr>
> > > # Date 1726202944 14400
> > > #      Fri Sep 13 00:49:04 2024 -0400
> > > # Node ID 5bfd931f3b9641b51344d437207134f094012de5
> > > # Parent  dbf76fdd109fbbba40a7c5299cc277d180f4bbad
> > > HTTP: Fix infinite loop on NGX_DECLINED in ngx_http_finalize_request
> > >
> > > A handler that invokes ngx_http_finalize_request with NGX_DECLINED
> > > causes an infinite loop because the phase handler index is not
> > > incremented before restarting the processing of phases.
> > >
> > > In (almost) all the other instances where a handler can return
> > > NGX_DECLINED, the phase handler index is incremented before restarting
> > > the processing of phases.
> > >
> > > This change adds that index increment where it was missing.
> > >
> > > diff -r dbf76fdd109f -r 5bfd931f3b96 src/http/ngx_http_request.c
> > > --- a/src/http/ngx_http_request.c     Tue Sep 03 13:11:25 2024 +0300
> > > +++ b/src/http/ngx_http_request.c     Fri Sep 13 00:49:04 2024 -0400
> > > @@ -2519,6 +2519,7 @@
> > >      if (rc == NGX_DECLINED) {
> > >          r->content_handler = NULL;
> > >          r->write_event_handler = ngx_http_core_run_phases;
> > > +        r->phase_handler++;
> > >          ngx_http_core_run_phases(r);
> > >          return;
> > >      }
> > >
> >
> > The ngx_http_finalize_request(NGX_DECLINED) call (or return
> > NGX_DECLINED) is expected to be used by location content handlers,
> > as set by r->content_handler (clcf->handler), to switch back to
> > phase handlers.  Note that it clears r->content_handler, so
> > ngx_http_core_content_phase() will resume normal content phase
> > processing.
> >
> > For example, perl module does this when you return NGX_DECLINED
> > from the perl code.
> >
> > And incrementing r->phase_handler will certainly screw up things for
> > this use case.
> >
> > Could you please clarify why you think that calling
> > ngx_http_finalize_request(NGX_DECLINED) in other cases might be
> > needed, and not a bug in the module which does this?
> >
> 
> Thank you for the explanation!
> 
> After further reflection, I realized that my module was attempting to
> incorrectly participate in the "ecosystem". I was writing my module to
> be a (core) content handler rather than a location content handler. I
> was doing this in an attempt to make it possible for my content
> handler to give an NGX_DECLINED with the effect of giving "other"
> content handlers a chance to respond. As I understand it, with
> location content handlers, only one is allowed. I tried a
> configuration with my module active and 'empty_gif;' and their
> relative order in the location block affected which module was deemed
> the content handler for that block.
> 
> It would be cool if there were a way for the module author to chain
> location handlers.
> 
> I suppose that is not out of the realm of possibility but each module
> would have to implement it themselves, it seems. A module could store
> the existing value of the location content handler during execution of
> the function for handling configuration information. Then, when their
> handler is invoked, they could immediately dispatch to the stored
> content handler in the case that they wanted to decline
> responsibility.

Not sure what exactly you are trying to do, but if you consider 
chaining your module with other modules, content phase handler 
might be more appropriate.  Alternatively, a location content 
handler which generates an error for requests it cannot handle 
might be an option, so it would be possible to configure 
error_page processing if needed.

In general there are two types of content handlers:

1. Content phase handlers, such as in the dav, index, autoindex, 
and static modules.  Such modules are configured globally, and 
settings are inherited into more specific contexts, such as 
locations or nested locations.  These modules can decline the 
request by returning NGX_DECLINED from the handler, so the request 
processing is passed to the next content phase handler.

2. Location content handlers, as set by clcf->handler, such as 
empty_gif, proxy_pass, mp4, and perl.   This can be seen as 
"simple" content handlers.  Such modules are activated exclusively 
for a particular location and not inherited into nested contexts.  
And they are not expected to be chained.  Still, such modules can 
decline the request by returning NGX_DECLINED from the handler or 
by calling ngx_http_finalize_request(NGX_DECLINED) at some point 
later (such as after reading the request body), so phase 
processing is resumed and the request is further handled by 
content phase handlers.

Depending on what you are trying to implement and how do you 
expect it to be configured, both content phase handlers and 
location content handlers might be appropriate.

Chaining location content handlers is not supported, and trying to 
configure more than one location content handler in a location is 
basically a configuration error: for example, you cannot have both 
"empty_gif" and "proxy_pass" in a location at the same time, as 
both modules are expected to handle all requests in the location.

Further, trying to support such chaining for location content 
handlers which handle only some of the requests might not be a 
good idea, since the resulting behaviour will depend on the order 
of the relevant directives, which is not generally expected from a 
declarative configuration (there are few exceptions, such as 
regular expressions and rewrite module instructions, but in general 
order of the directives is not important).  Rather, if chaining is 
needed, using content phase handlers might be a better option - so 
the order of processing by different modules is well known.

Hope this helps.

-- 
Maxim Dounin
http://mdounin.ru/