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@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@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/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org