[PATCH] SSI: ensure context of main request exists for subrequest using SSI
# HG changeset patch
# User Ciel Zhao
# Date 1667411069 -28800
# Thu Nov 03 01:44:29 2022 +0800
# Node ID 85141e004b5af89a9d443bc0084a34291193567a
# Parent 1ae25660c0c76edef14121ca64362f28b9d57a70
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 checks the context of the main request after initializing the context
for subrequests, and creates one if not exists.
diff -r 1ae25660c0c7 -r 85141e004b5a
src/http/modules/ngx_http_ssi_filter_module.c
--- a/src/http/modules/ngx_http_ssi_filter_module.c Wed Oct 19 10:56:21 2022
+0300
+++ b/src/http/modules/ngx_http_ssi_filter_module.c Thu Nov 03 01:44:29 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);
@@ -348,6 +348,16 @@
ngx_http_set_ctx(r, ctx, ngx_http_ssi_filter_module);
+mctx = ngx_http_get_module_ctx(r->main, ngx_http_ssi_filter_module);
+if (mctx == NULL && r != r->main) {
+mctx = ngx_pcalloc(r->main->pool, sizeof(ngx_http_ssi_ctx_t));
+if (mctx == NULL) {
+return NGX_ERROR;
+}
+
+ngx_http_set_ctx(r->main, mctx, ngx_http_ssi_filter_module);
+}
+
ctx->value_len = slcf->value_len;
ctx->last_out = &ctx->out;
@@ -403,8 +413,12 @@
ngx_str_t *params[NGX_HTTP_SSI_MAX_PARAMS + 1];
ctx = ngx_http_get_module_ctx(r, ngx_http_ssi_filter_module);
+slcf = ngx_http_get_module_loc_conf(r, ngx_http_ssi_filter_module);
if (ctx == NULL
+|| !slcf->enable
+|| r->headers_out.content_length_n == 0
+|| ngx_http_test_content_type(r, &slcf->types) == NULL
|| (in == NULL
&& ctx->buf == NULL
&& ctx->in == NULL
@@ -450,8 +464,6 @@
}
}
-slcf = ngx_http_get_module_loc_conf(r, ngx_http_ssi_filter_module);
-
while (ctx->in || ctx->buf) {
if (ctx->buf == NULL) {
___
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org
Re: [PATCH] SSI: ensure context of main request exists for subrequest using SSI
Hi Maxim, Thanks for the quick reply. > It looks like an attempt to fix ticket #1263 > (https://trac.nginx.org/nginx/ticket/1263). I've linked this > thread to the ticket. It might be a good idea to add a reference > into commit log. I encountered this problem building my own website, and spent some hours to figure out the problem and patch. I didn't guess this bug have such history. Reference will be in the updated patch. > In many cases the main request context is not needed. It might be > a good idea to create it only when needed, and avoid doing so if > it isn't. > > Further, in theory, SSI processing of a (in-memory/background) > subrequest might happen even before main request is actually seen > by the SSI module, so the actual context will be created later. > This probably needs to be taken into account. As we only use the main context for variables and stub blocks, it seems appreciable to have the context created on demand. However, we can't say if the SSI templates use these commands until we processed them. So I think this left us merely two options: a. create main context for every requests having SSI enabled (previous patch) b. check main context existence everywhere we use it >From my purview, most SSI templates have some variables involved, so just go >ahead and create context can save many checks, while not introducing much waste of memory. > This does not look like a good solution to filter out processing > of a main request if SSI is not enabled for it. Rather, there > should be an explicit flag, so it would be possible to avoid > evaluation of complex constructs on each body filter call - there > can be a lot of such calls. Totally agree, and this can be checked by `ngx_http_ssi_header_filter` in main request, to initialize the dummy context if needed. But I wonder shall I add a bit field into `ngx_http_ssi_ctx_t` (as there're already some so no memory cost) or add an `ngx_flag_t` (8B on amd64) instead. In the issue tracker 6 years ago, you mentioned, > It was written when there were no subrequests except subrequests created by > the SSI > module itself, and assumes in many places that its context always exists in > the main > request. Though I'm not 100% familiar with the process of subrequests, this arise some more questions in my mind: If subrequests are processed by the SSI filter (before postpone) asynchronously and concurrently, could this introduce some out-of-order execution of SSI commands among subrequests, then lead to non-determined result? If this really happens, should we move the SSI filter past the postpone filter? Looking forward to some enlightenment. I'll post my updated patch after these discussions settle. And thanks again for attention. Ciel Zhao ___ nginx-devel mailing list -- nginx-devel@nginx.org To unsubscribe send an email to nginx-devel-le...@nginx.org
Re: [PATCH] SSI: ensure context of main request exists for subrequest using SSI
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.
> 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.
I've implemented two patch here, the first is the just-create way fixed, and the
second is the context-stealing way. Up to you to choose, or let me know for
other problems.
Patch A:
# HG changeset patch
# User Ciel Zhao
# Date 1667927876 -28800
# Wed Nov 09 01:17:56 2022 +0800
# Node ID f7046e9deabef8c1d3caa4809a4ed5f93c17cf99
# 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 checks the context of the main request after initializing the context
for subrequests, and creates one if not exists.
diff -r 17d6a537fb1b -r f7046e9deabe
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:17:56
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,14 +341,26 @@
return ngx_http_next_header_filter(r);
}
-ctx = ngx_pcalloc(r->pool, sizeof(ngx_http_ssi_ctx_t));
+ctx = ngx_http_get_module_ctx(r, ngx_http_ssi_filter_module);
if (ctx == NULL) {
-return NGX_ERROR;
+ctx = ngx_pcalloc(r->pool, sizeof(ngx_http_ssi_ctx_t));
+if (ctx == NULL) {
+return NGX_ERROR;
+}
+ngx_http_set_ctx(r, ctx, ngx_http_ssi_filter_module);
}
-ngx_http_set_ctx(r, ctx, ngx_http_ssi_filter_module);
-
-
+mctx = ngx_http_get_module_ctx(r->main, ngx_http_ssi_filter_module);
+if (mctx == NULL) {
+mctx = ngx_pcalloc(r->main->pool, sizeof(ngx_http_ssi_ctx_t));
+if (mctx == NULL) {
+return NGX_ERROR;
+}
+ngx_http_set_ctx(r->main, mctx, ngx_http_ssi_filter_module);
+}
+
+
+ctx->enabled = 1;
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->enabled
|| (in == NULL
&& ctx->buf == NULL
&& ctx->in == NULL
diff -r 17d6a537fb1b -r f7046e9deabe
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:17:56
2022 +0800
@@ -71,6 +71,7 @@
u_char *captures_data;
#endif
+unsigned enabled:1;
unsigned conditional:2;
unsigned encoding:2;
unsigned block:1;
Patch C:
# HG changeset patch
# User Ciel Zhao
# 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 us
Re: [PATCH] SSI: ensure context of main request exists for subrequest using SSI
Hello! Great insights. This version seems way more clear for me, and works perfectly on my server. So this pronounced like the final fix for this ticket. (Sorry about forgetting adding the reference again) Since it may take significant time for the patch to rollout, I'd reachout to the Debian package maintainers for the patching of Debian 11 bullseye, if this patch is now accepted by NGINX. Thanks again for your attention and patience. Ciel ___ nginx-devel mailing list -- nginx-devel@nginx.org To unsubscribe send an email to nginx-devel-le...@nginx.org
Re: [PATCH] SSI: ensure context of main request exists for subrequest using SSI
Hello!
On Monday, November 21st, 2022 at 17:28, Sergey Kandaurov
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.
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?
Updated patch below.
# HG changeset patch
# User Ciel Zhao
# 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);
@@ -379,6 +401,10 @@
} 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 +431,7 @@
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
@@ -1559,9 +1586,9 @@
ngx_uint_t i;
ngx_list_part_t *part;
ngx_http_ssi_var_t *var;
-ngx_http_ssi_ctx_t *ctx;
-
-ctx = ngx_http_get_module_ctx(r->main, ngx_http_ssi_filter_module);
+ngx_http_ssi_ctx_t *mctx;
+
+mctx = ngx_http_get_module_ctx(r->main, ngx_http_ssi_filter_module);
#if (NGX_PCRE)
{
@@ -1570,7 +1597,7 @@
if (key >= '0' && key <= '9') {
i = key - '0';
-if (i < ctx->ncaptures) {
+if (i < mctx->ncaptures) {
value = ngx_palloc(r->pool, sizeof(ngx_str_t));
if (value == NULL) {
return NULL;
@@ -1578,8 +1605,8 @@
i *= 2;
-value->data = ctx->captures_data + ctx->captures[i];
-value->len = ctx->captures[i + 1] - ctx->captures[i];
+value->data = mctx->captures_data + mctx->captures[i];
+value->len = mctx->captures[i + 1] - mctx->captures[i];
return value;
}
@@ -1587,11 +1614,11 @@
}
#endif
-if (ctx->variables == NULL) {
+if (mctx->variables == NULL) {
return NULL;
}
-part = &ctx->variables->part;
+part = &mctx->variables->part;
var = part->elts;
for (i = 0; /* void */ ; i++) {
diff -r 17d6a537fb1b -r a42cc9c1b1fd
src/http/modules/ngx_http_ssi_filter_module.h
--- a/src/http/modules/ngx_ht
