On Sun, Sep 15, 2024 at 12:50 AM Maxim Dounin <mdou...@mdounin.ru> wrote:
>
> Hello!
>
> On Sat, Sep 14, 2024 at 11:46:09PM -0400, Will Hawkins wrote:
>
> > On Sat, Sep 14, 2024 at 11:13 PM Maxim Dounin <mdou...@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 <mdou...@mdounin.ru>
> > > > wrote:
> > > > >
> > > > > Hello!
> > > > >
> > > > > On Fri, Sep 13, 2024 at 01:20:34AM -0400, Will Hawkins wrote:
> > > > >
> > > > > > # HG changeset patch
> > > > > > # User Will Hawkins <hawki...@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!
>
> The specific ngx_http_finalize_request(NGX_DECLINED) handling is
> for location content handlers. While it probably can be extended
> to support content phase (since r->content_handler is expected to
> be NULL at the content phase), it won't work for other phases
> anyway. (And the required code would be more complex, see
> ngx_http_core_content_phase() - it needs to check if there are
> additional content phase handlers, and return 403/404 if not.)
>
> As such, I would rather recommend generic approach to resume phase
> processing, much like it is used in other phase handlers: restore
> ngx_http_core_run_phases() as a write handler, run it, and then
> return NGX_DECLINED from the handler.
>
> (Note that just calling ngx_http_finalize_request(NGX_DECLINED)
> and then returning NGX_DECLINED from the handler would be mostly
> equivalent for a content phase handler, but I would rather
> recommend restoring ngx_http_core_run_phases explicitly.)
>
> Examples can be seen in the limit_req and mirror modules. The
> mirror module specifically reads the request body and can be used
> as a guidance on how to properly restore phase processing after
> reading the request body.
>
> (Note though that the mirror module runs in the precontent phase,
> and therefore needs additional steps to properly pause phase
> processing; for a content phase handler, just
>
> rc = ngx_http_read_client_request_body(r, ngx_http_foo_handler);
>
> if (rc >= NGX_HTTP_SPECIAL_RESPONSE) {
> return rc;
> }
>
> return NGX_DONE;
>
> as normally seen in content handlers would be the way to go.)
>
Thank you (again!) for the thorough response. I really appreciate it.
It might take me a day or two, but I would like to experiment with
preparing another version of the patch that incorporates your advice
by creating a function that would take the necessary steps to "fixup"
the request so that returning an NGX_DECLINED triggers processing in
the next phase.
Thank you again for the response!
Will
> --
> Maxim Dounin
> http://mdounin.ru/
