[PATCH] SSI: ensure context of main request exists for subrequest using SSI

[Maxim Dounin]

Hello!

On Mon, Nov 21, 2022 at 11:33:42AM +0000, Ciel via nginx-devel wrote:

> On Monday, November 21st, 2022 at 17:28, Sergey Kandaurov <pluknet at nginx.com> wrote:
> 
> > The patch doesn't cover regex positional captures in the "if" command.
> > They are also used to be stored in the main request in ctx->captures.
> > 
> > Upgrading main context makes them unavailable, so the fields need to be
> > copied under mctx condition as well if we want to care about that.
> 
> Thanks for pointing out that. I've been scanning this code finding `mctx`s, but
> the one in `ngx_http_ssi_get_variable` used `ctx` instead.
> 
> Now I've changed that variable to `mctx` to align with other references, and
> also made sure that every context of `r->main` gets correct name. If that's not
> appropriate, feel free to trim that part.

That's an unrelated style change, and it should be done in a 
separate patch, if at all (and I would rather refrain from 
changing it).

> I'm not familiar with nginx's roll-out strategy, could anyone kindly tell me when
> shall I expect to see this usable in a stable/mainline release?

Once committed, it will be available in the next mainline release.  
Likely to happen in a couple of months.

> Updated patch below.
> 
> 
> # HG changeset patch
> # User Ciel Zhao <i at ciel.dev>
> # Date 1669029675 -28800
> #      Mon Nov 21 19:21:15 2022 +0800
> # Node ID a42cc9c1b1fd6c07798c7ad6c6a2a3da24e9cc21
> # Parent  17d6a537fb1bb587e4de22961bf5be5f0c648fa8
> SSI: handling of subrequests from other modules (ticket #1263).
> 
> As the SSI parser always uses the context from the main request for storing
> variables and blocks, that context should always exist for subrequests using
> SSI, even though the main request does not necessarily have SSI enabled.
> 
> However, `ngx_http_get_module_ctx(r->main, ...)` is getting NULL in such cases,
> resulting in the worker crashing SIGSEGV when accessing its attributes.
> 
> This patch links the first initialized context to the main request, and
> upgrades it only when main context is initialized.
> 
> diff -r 17d6a537fb1b -r a42cc9c1b1fd src/http/modules/ngx_http_ssi_filter_module.c
> --- a/src/http/modules/ngx_http_ssi_filter_module.c     Wed Nov 02 13:46:16 2022 +0400
> +++ b/src/http/modules/ngx_http_ssi_filter_module.c     Mon Nov 21 19:21:15 2022 +0800
> @@ -329,7 +329,7 @@
>  static ngx_int_t
>  ngx_http_ssi_header_filter(ngx_http_request_t *r)
>  {
> -    ngx_http_ssi_ctx_t       *ctx;
> +    ngx_http_ssi_ctx_t       *ctx, *mctx;
>      ngx_http_ssi_loc_conf_t  *slcf;
>  
>      slcf = ngx_http_get_module_loc_conf(r, ngx_http_ssi_filter_module);
> @@ -341,6 +341,8 @@
>          return ngx_http_next_header_filter(r);
>      }
>  
> +    mctx = ngx_http_get_module_ctx(r->main, ngx_http_ssi_filter_module);
> +
>      ctx = ngx_pcalloc(r->pool, sizeof(ngx_http_ssi_ctx_t));
>      if (ctx == NULL) {
>          return NGX_ERROR;
> @@ -367,6 +369,26 @@
>      r->filter_need_in_memory = 1;
>  
>      if (r == r->main) {
> +
> +        if (mctx) {
> +
> +            /*
> +             * if there was a shared context previously used as main,
> +             * copy variables and blocks
> +             */
> +
> +            ctx->variables = mctx->variables;
> +            ctx->blocks = mctx->blocks;
> +
> +#if (NGX_PCRE)
> +            ctx->ncaptures = mctx->ncaptures;
> +            ctx->captures = mctx->captures;
> +            ctx->captures_data = mctx->captures_data;
> +#endif
> +
> +            mctx->shared = 0;
> +        }
> +
>          ngx_http_clear_content_length(r);
>          ngx_http_clear_accept_ranges(r);
>  

Looks good to me.

Pushed to http://mdounin.ru/hg/nginx without the variable renaming 
part, with just

@@ -380,6 +380,12 @@ ngx_http_ssi_header_filter(ngx_http_requ
             ctx->variables = mctx->variables;
             ctx->blocks = mctx->blocks;
 
+#if (NGX_PCRE)
+            ctx->ncaptures = mctx->ncaptures;
+            ctx->captures = mctx->captures;
+            ctx->captures_data = mctx->captures_data;
+#endif
+
             mctx->shared = 0;
         }
 

compared to the previous version of the patch.

Thanks.

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