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

[Will Hawkins]

On Sat, Sep 14, 2024 at 11:13 PM Maxim Dounin <mdounin at mdounin.ru> wrote:
>
> 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.

Thank you for the response! I really appreciate it. This was (is?)
exactly what I was trying to do. As I said, however, if the handler
cannot make the decision to handle the response until after reading
the body, it cannot return NGX_DECLINED from the handler. It would
have to return something else (e.g., NGX_OK) while waiting for the
callback given to ngx_http_read_client_request_body to execute to
examine the body. Then, inside that callback, the only option would be
to call ngx_http_finalize_request(NGX_DECLINED). At that point, we
would have the infinite loop that I experienced. That was one of the
reasons that I thought the fix I submitted would be helpful. In other
words, the kind of handler I am writing would not be working in a
situation where r->content_handler has a value.

So, would it be possible to have a second version of my proposed patch
that would change the relevant part of ngx_http_finalize_request to
look like:

if (rc == NGX_DECLINED) {
  if (r->content_handler == NULL) {
      r->phase_handler++;
  }
  r->content_handler = NULL;
  r->write_event_handler = ngx_http_core_run_phases;
  ngx_http_core_run_phases(r);
  return;
}

That might make possible both situations:
1. Preserve the existing behavior for location content handlers (like
the perl module the way that you mentioned)
2. Add support for returning NGX_DECLINED in content phase handlers.

If that seems reasonable, I would be more than happy to modify the
patch and submit!


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

You are (of course!) absolutely correct! That would make for a
complicated situation that might result in surprising behavior.

Thank you, again, for all your feedback!
Will

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