Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(proxy-wasm) do not accidentally reset filter list from FFI call #423

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

hishamhm
Copy link
Contributor

@hishamhm hishamhm commented Oct 2, 2023

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.

@hishamhm hishamhm requested a review from thibaultcha October 2, 2023 22:04
@flrgh
Copy link
Contributor

flrgh commented Oct 2, 2023

👍 Just verified locally: this is passing the gateway integration test (currently failing against e0f5a9f).

@thibaultcha
Copy link
Member

Excellent, thanks for taking care of it!

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.
@hishamhm hishamhm force-pushed the fix/ffi-pwctx-ready-too-early branch from 371a615 to 2a8d403 Compare October 2, 2023 22:51
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #423 (2a8d403) into main (e0f5a9f) will increase coverage by 0.00117%.
The diff coverage is 100.00000%.

Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##                main        #423         +/-   ##
===================================================
+ Coverage   90.12876%   90.12993%   +0.00117%     
===================================================
  Files             46          46                 
  Lines           8388        8389          +1     
===================================================
+ Hits            7560        7561          +1     
  Misses           828         828                 
Files Coverage Δ
src/common/proxy_wasm/ngx_proxy_wasm.c 92.46377% <100.00000%> (+0.01093%) ⬆️

@thibaultcha thibaultcha merged commit 89170ed into main Oct 3, 2023
32 checks passed
@thibaultcha thibaultcha deleted the fix/ffi-pwctx-ready-too-early branch October 3, 2023 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants