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

[Maxim Dounin]

Hello!

On Fri, Nov 04, 2022 at 09:00:33AM +0000, Ciel via nginx-devel wrote:

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

Certainly there are SSI templates which only use include commands, 
but you are probably right and creating the main request context 
would be easier.  With on-demand creation, even if we'll introduce 
a function to obtain/create main request context, we'll have to 
check for memory allocation errors in all places we use it.

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.

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

The ngx_flag_t type is configuration-specific, and shouldn't be 
used in runtime structures such as SSI contexts.

The general rule for runtime flags is that they are created as 
ngx_uint_t with appropriate comment if there are no other 
bit fields in the structure, for example
(src/http/modules/ngx_http_headers_filter_module.c):

    ngx_uint_t                 always;  /* unsigned  always:1 */

and upgraded to proper bit fields as long as there is more than 
one.

Given that ngx_http_ssi_ctx_t already contains bit fields, the new 
flag should be added as a bit field.

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

The idea of the postpone filter is to glue all subrequests 
together in the correct order.  Basically, it makes it possible to 
execute SSI subrequests in parallel.

For sure this can be a problem if subrequests are used without 
taking this into account.  For example, if you'll set a variable 
in an SSI include virtual, trying to use it in the main SSI 
document or other included files might easily fail because it is 
not yet set.

To fix this, there is a special parameter of the include SSI 
command, "wait".  Quoting docs 
(https://nginx.org/en/docs/http/ngx_http_ssi_module.html#commands):

: wait
:
: a non-standard parameter that instructs to wait for a request to 
: fully complete before continuing with SSI processing, for example:
:
: <!--# include virtual="/remote/body.php?argument=value" wait="yes" -->

This makes it possible to add "sequence points" to SSI, resolving 
undefined behaviour due to parallel execution of requests.

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