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

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