Sub-requests poisoning r->variables cache

[Maxim Dounin]

Hello!

On Thu, Feb 22, 2018 at 11:53:14AM +1100, Lawrence Stewart via nginx-devel wrote:

> Greetings nginx developers,
> 
> The root cause analysis for a bug I observed during development of some
> custom nginx module code turns out to be an unfortunate interaction between
> sub-requests and indexed variable lookups using
> ngx_http_get_indexed_variable(). We're based on nginx 1.13.6.
> 
> The sequence of events is:
> 
> - Main request arrives and triggers an access-phase sub-request. Main
> request includes request variable "x=blah", but sub-request does not.
> 
> - Sub-request arrives at custom module's header filter
> 
> - Custom module looks up variable "x" with ngx_http_get_indexed_variable(),
> and it is not found in the sub-request. This causes the "not_found" flag to
> be set in the sr->variables var cache for variable "x", but
> because ngx_http_subrequest() shallow copies r->variables from parent
> request to sub-request, the not_found flag change affects the main
> request's r->variables too.
> 
> - Sub-request completes, main request is allowed to progress and arrives at
> custom module's header filter
> 
> - Custom module again looks up variable "x"
> with ngx_http_get_indexed_variable(), and even though "x" is present in
> r->args and I've confirmed can be found by
> ngx_http_arg(), ngx_http_get_indexed_variable() sees the not_found flag,
> does not refresh the variable cache and returns the poisoned
> ngx_http_variable_value_t.
> 
> There are ways to deal with this in our custom code, but it seems like a
> fundamental problem in the current design of the sub-request mechanism and
> I'm thinking that it may be appropriate to address this in the nginx core
> code e.g. by deep copying r->variables in ngx_http_subrequest(), or by
> somehow tracking when to invalidate the ngx_http_variable_value_t flags and
> refresh the r->variables cache.
> 
> Thoughts?

Variables are expected to be shared between the main request and 
subrequests.  If a variable can have different values in the main 
request and subrequests, consider marking it non-cacheable.  For 
example, the $arg_* variables are defined as follows:

    { ngx_string("arg_"), NULL, ngx_http_variable_argument,
      0, NGX_HTTP_VAR_NOCACHEABLE|NGX_HTTP_VAR_PREFIX, 0 },

This is because request arguments can be changed, and, in 
particular, can be different in a subrequest.

While the approach with shared variables might not be convenient 
in some scenarious, it is certainly is in many of them - for 
example, you don't need to recalculate various map{} results, you 
can freely use variables in SSI includes (the actual use case 
subrequests were created for), and so on.

Given the above, I don't really think we need to change things 
here.  Suggested changes are likely to break more things they will 
fix.

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