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

[Will Hawkins]

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.

Thank you again for the helpful response and sorry for wasting your time!
Will

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