Hello!

On Thu, Nov 03, 2022 at 04:25:52AM +0000, Ciel via nginx-devel wrote:

> # HG changeset patch
> # User Ciel Zhao <i@ciel.dev>
> # 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.

Thanks for the patch.

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.

See below for some comments about the code.

>
> 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;

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.

> @@ -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

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.

> @@ -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) {
>

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