[PATCH 1 of 1] HTTP: Fix infinite loop on NGX_DECLINED in ngx_http_finalize_request
Maxim Dounin
mdounin at mdounin.ru
Sun Sep 15 04:50:23 UTC 2024
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 <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!
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.)
--
Maxim Dounin
http://mdounin.ru/
More information about the nginx-devel
mailing list