On 8/7/24 4:54 AM, Thomas P. wrote:
Hello,
I'm experimenting with non-cacheable variables to compute some values used in
http_rewrite. I've noticed that, for directives like "set $var $computed_var",
I'm getting 2 calls to $computed_var's get_handler. After reading the code behind this
directive, I've noticed it goes through ngx_http_get_flushed_variable twice for every
external variable: once to determine its length, then another time to copy contents
(after allocating the total length of the result). While I expect the get_handler
function to be called every time the variable is referred to, I think calling it once
shall suffice. Switching to ngx_http_get_indexed_variable fixes the issue, and the cache
is still cleared after evaluating the set directive it seems, as calling the non
cacheable variable in another expression calls get_handler another time. All tests still
pass with this change.
Here's the only behaviour change I could observe:
Consider a non-cached variable $weird_var keeping track of its call numbers, and
returning "0" then N times the number of calls so far: 01, 022, 0333...
Use it twice in the same directive: "set $stuff $weird_var$weird_var"
The variable's get_handler will be called twice (which sort of makes sense
since the variable is referred to twice) but the resulting value is the result
of the last call, truncated since it doesn't fit in the allocated buffer: stuff
is 02202 (intuitively, it would be 01022)
Without this patch, the result would be 03330 which is also not the expected
result anyway, so I doubt this would break anyone's workflow. On the other
side, this speeds up complex value processing as we don't make unnecessary
calls.
The changeset is pretty basic, happy reviewing!
Thomas P.
# HG changeset patch
# User Thomas P. <t...@live.fr>
# Date 1722959175 -7200
# Tue Aug 06 17:46:15 2024 +0200
# Node ID d6283a655f35a03f49427a2d88ebbc27f6946e3d
# Parent d1b8568f3042f6019a2302dda4afbadd051fe54b
[http_rewrite] Avoid duplicate non-cacheable variable calls for complex
expressions
The http_rewrite module's rewrite handler called twice variables'
get_handler for non-cacheable variables referenced in expressions like
"set $a $upstream_code;". It goes through ngx_http_get_flushed_variable
once to determine variable length, and one more time to get the variable
contents.
Setting the flushed attribute on the script engine fixes the
issue. This flag is already set in other parts that rely on http_script and
it did not break any test, so it can be safely set. Besides, this only impacts
non-cacheable variables processing.
diff -r d1b8568f3042 -r d6283a655f35 src/http/modules/ngx_http_rewrite_module.c
--- a/src/http/modules/ngx_http_rewrite_module.c Thu Jul 18 17:43:25
2024 +0400
+++ b/src/http/modules/ngx_http_rewrite_module.c Tue Aug 06 17:46:15
2024 +0200
@@ -174,6 +174,7 @@
e->quote = 1;
e->log = rlcf->log;
e->status = NGX_DECLINED;
+ e->flushed = 1;
while (*(uintptr_t *) e->ip) {
code = *(ngx_http_script_code_pt *) e->ip;
Additional context:
NJS patch https://github.com/nginx/njs/pull/771 which introduced
no_cacheable variables triggers a heap-buffer-overflow when the length
of a returned no_cacheable variable varies. Setting e->flushed to 1
fixes the problem.
Some observation of nginx code:
We do set flush to 1 in every place in http and stream except rewrite
handler. This looks like the only place like this.
If e.flush is always 1 we can simplify script code, by removing
ngx_http_get_flushed_variable() calls which cause buffer-overflow when a
variable is no_cacheable and its length varies.
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
