Skip to content

Commit

Permalink
fix(proxy-wasm) do not accidentally reset filter list from FFI call
Browse files Browse the repository at this point in the history
When calling `proxy_wasm.set_property` from Lua, we want a `pwctx`
to be able to store a property. But we need to do that before
`proxy_wasm.start`, so `pwctx->ready` didn't have a chance to get
initialized yet.

Passing `NULL, 0` to `filter_ids` and `nfilters` when obtaining the `pwctx`
from the `set_property` FFI call caused the `pwctx` to get initialized with an
empty filter list, defeating the configuration previously set up by
`proxy_wasm.attach`.

In this fix, we make the `NULL` value of `filter_ids` to mean
"please don't do the `pwctx->ready` initialization". The other
use of `ngx_proxy_wasm_ctx` in `ngx_wasm_ops.c` always passes a
non-NULL value (even when the filter list is empty), because the
`elts` pointer is allocated on `ngx_array_init`.

This issue caused an integration failure in Kong Gateway. This PR
includes a regression test for our test suite, which reproduces the
sequence observed in the Gateway; the test fails without the fix
added to `ngx_proxy_wasm.c` and passes with the fix.
  • Loading branch information
hishamhm authored and thibaultcha committed Oct 3, 2023
1 parent e0f5a9f commit 89170ed
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 2 deletions.
6 changes: 4 additions & 2 deletions src/common/proxy_wasm/ngx_proxy_wasm.c
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ ngx_proxy_wasm_ctx(ngx_uint_t *filter_ids, size_t nfilters,
return NULL;
}

if (!pwctx->ready) {
if (!pwctx->ready && filter_ids) {
pwctx->nfilters = nfilters;
pwctx->isolation = isolation;

Expand Down Expand Up @@ -399,7 +399,9 @@ ngx_proxy_wasm_ctx_destroy(ngx_proxy_wasm_ctx_t *pwctx)
ngx_proxy_wasm_release_instance(ictx, 0);
}

ngx_proxy_wasm_store_destroy(&pwctx->store);
if (pwctx->ready) {
ngx_proxy_wasm_store_destroy(&pwctx->store);
}

#if 0
if (pwctx->authority.data) {
Expand Down
43 changes: 43 additions & 0 deletions t/04-openresty/ffi/302-proxy_wasm_set_property_host.t
Original file line number Diff line number Diff line change
Expand Up @@ -262,3 +262,46 @@ ok
]
--- no_error_log
[error]
=== TEST 7: set_property() - regression test: setting properties at startup doesn't reset filter list
--- wasm_modules: hostcalls
--- http_config
init_worker_by_lua_block {
local proxy_wasm = require "resty.wasmx.proxy_wasm"
local filters = {
{ name = "hostcalls", config = "on=log " ..
"test=/t/log/property " ..
"name=wasmx.startup_property" },
}
_G.c_plan = assert(proxy_wasm.new(filters))
assert(proxy_wasm.load(_G.c_plan))
}
--- config
location /t {
proxy_wasm_request_headers_in_access on;
access_by_lua_block {
local proxy_wasm = require "resty.wasmx.proxy_wasm"
assert(proxy_wasm.attach(_G.c_plan))
assert(proxy_wasm.set_property("wasmx.startup_property", "foo"))
assert(proxy_wasm.start())
ngx.say("ok")
}
}
--- response_body
ok
--- error_log eval
[
qr/\[info\] .*? \[hostcalls\] on_log/,
qr/\[info\] .*?hostcalls.*? wasmx.startup_property: foo/,
]
--- no_error_log
[error]

1 comment on commit 89170ed

@thibaultcha
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just dropping a note here as I write the changelog: I think this was a hotfix, fixing the initial fix of fake requests contexts freeing in e0f5a9f

Please sign in to comment.