Hello!

On Tue, Nov 08, 2022 at 05:31:25PM +0000, Ciel via nginx-devel wrote:

> Hello! Thanks for your detailed explanation, really helps a lot!
>
> > Another option might be to link the first subrequest's context as
> > the main one - till the main request context is created (if at
> > all). Especially given that we anyway have to upgrade the main
> > request context if the main request is seen after the first
> > subrequest. This will imply an additional check in the body
> > filter along with the flag, but a trivial one.
>
> Yes, you're right, this is a valid *option C* and I have implemented that.
> This can truly save bytes, if someone have SSI only on main request disabled,
> but use templates without variables or blocks.
>
> However, due to the context stealing, we should mark whether the context is for
> main/subrequests, and distinguish that in body filter. Honestly, it's kind of
> counter-intuitive for me to share one context and use (type, flag) tuple to
> check for validity.

While sharing contexts certainly requires some checking, I don't
think there is major difference, especially with slightly more
intuitive flag name (see below).

On the other hand, I tend to think it's somewhat wrong to keep a
created but half-initialized context, which happens with your
original approach.

> > This makes it possible to add "sequence points" to SSI, resolving
> > undefined behaviour due to parallel execution of requests.
>
> But what if the parallel subrequests do not share a common SSI parent, i.e.
> introduced concurrently by other modules? The `wait` parameter seems have dealt
> with intra-module concurrency, but not inter-module ones. If that truly is a
> problem, I'm not going to cover this case in this patch. So answer at your
> interest or leave it alone.

If there are dependencies between subrequests created by some
other module, these are to be addressed with appropriate
synchronization within the module which created these subrequests.
Certainly does not look like something to be addressed in this
work.

[...]

> ********************************************************************************
> Patch C:
>
> # HG changeset patch
> # User Ciel Zhao <i@ciel.dev>
> # Date 1667928380 -28800
> # Wed Nov 09 01:26:20 2022 +0800
> # Node ID 4b6f88048a6104478709b5bd9a6cc6c0c343b36c
> # Parent 17d6a537fb1bb587e4de22961bf5be5f0c648fa8
> SSI: ensure context of main request exists for subrequest using SSI
>
> 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 4b6f88048a61 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 Wed Nov 09 01:26:20 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, *sctx;
> ngx_http_ssi_loc_conf_t *slcf;
>
> slcf = ngx_http_get_module_loc_conf(r, ngx_http_ssi_filter_module);
> @@ -346,9 +346,21 @@
> return NGX_ERROR;
> }
>
> + sctx = ngx_http_get_module_ctx(r, ngx_http_ssi_filter_module);
> + if (sctx != NULL) {
> + // migrate to main context if needed, see below
> + ctx->blocks = sctx->blocks;
> + ctx->variables = sctx->variables;
> + }
> +
> ngx_http_set_ctx(r, ctx, ngx_http_ssi_filter_module);
> -
> -
> + if (ngx_http_get_module_ctx(r->main, ngx_http_ssi_filter_module) == NULL) {
> + // set main context to current context if not exists
> + ngx_http_set_ctx(r->main, ctx, ngx_http_ssi_filter_module);
> + }
> +
> +
> + ctx->is_main = (r == r->main);
> ctx->value_len = slcf->value_len;
> ctx->last_out = &ctx->out;
>
> @@ -405,6 +417,7 @@
> ctx = ngx_http_get_module_ctx(r, ngx_http_ssi_filter_module);
>
> if (ctx == NULL
> + || (!ctx->is_main && r == r->main)
> || (in == NULL
> && ctx->buf == NULL
> && ctx->in == NULL
> diff -r 17d6a537fb1b -r 4b6f88048a61 src/http/modules/ngx_http_ssi_filter_module.h
> --- a/src/http/modules/ngx_http_ssi_filter_module.h Wed Nov 02 13:46:16 2022 +0400
> +++ b/src/http/modules/ngx_http_ssi_filter_module.h Wed Nov 09 01:26:20 2022 +0800
> @@ -71,6 +71,7 @@
> u_char *captures_data;
> #endif
>
> + unsigned is_main:1;
> unsigned conditional:2;
> unsigned encoding:2;
> unsigned block:1;
>

Below is slightly cleaned up version of this patch, please take a
look:

# HG changeset patch
# User Ciel Zhao <i@ciel.dev>
# Date 1667928380 -28800
# Wed Nov 09 01:26:20 2022 +0800
# Node ID 41c67de61c7a916c01f0a1f5054d11821991ffce
# Parent 42bc158a47ecb3c2bd0396c723c307c757f2770e
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 --git a/src/http/modules/ngx_http_ssi_filter_module.c b/src/http/modules/ngx_http_ssi_filter_module.c
--- a/src/http/modules/ngx_http_ssi_filter_module.c
+++ b/src/http/modules/ngx_http_ssi_filter_module.c
@@ -329,7 +329,7 @@ static ngx_http_variable_t ngx_http_ssi
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 @@ ngx_http_ssi_header_filter(ngx_http_requ
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,20 @@ ngx_http_ssi_header_filter(ngx_http_requ
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;
+
+ mctx->shared = 0;
+ }
+
ngx_http_clear_content_length(r);
ngx_http_clear_accept_ranges(r);

@@ -379,6 +395,10 @@ ngx_http_ssi_header_filter(ngx_http_requ
} else {
ngx_http_weak_etag(r);
}
+
+ } else if (mctx == NULL) {
+ ngx_http_set_ctx(r->main, ctx, ngx_http_ssi_filter_module);
+ ctx->shared = 1;
}

return ngx_http_next_header_filter(r);
@@ -405,6 +425,7 @@ ngx_http_ssi_body_filter(ngx_http_reques
ctx = ngx_http_get_module_ctx(r, ngx_http_ssi_filter_module);

if (ctx == NULL
+ || (ctx->shared && r == r->main)
|| (in == NULL
&& ctx->buf == NULL
&& ctx->in == NULL
diff --git a/src/http/modules/ngx_http_ssi_filter_module.h b/src/http/modules/ngx_http_ssi_filter_module.h
--- a/src/http/modules/ngx_http_ssi_filter_module.h
+++ b/src/http/modules/ngx_http_ssi_filter_module.h
@@ -71,6 +71,7 @@ typedef struct {
u_char *captures_data;
#endif

+ unsigned shared:1;
unsigned conditional:2;
unsigned encoding:2;
unsigned block:1;


--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org