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

feat(wasm): proxy-wasm property getters/setters #11856

Merged
merged 3 commits into from Dec 5, 2023

Conversation

hishamhm
Copy link
Contributor

@hishamhm hishamhm commented Oct 26, 2023

Summary

Adds support for proxy-wasm properties in the kong.* namespace, making a number of Kong PDK functions available for Wasm filters, using the support for dynamic property getters/setters introduced in Kong/ngx_wasm_module#431.

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - beta feature

@hishamhm hishamhm force-pushed the feat/wasm-dynamic-properties branch 3 times, most recently from df17f42 to aeb256d Compare October 26, 2023 14:15
@hishamhm hishamhm force-pushed the feat/wasm-dynamic-properties branch 3 times, most recently from 2bc0a7e to c45beac Compare October 26, 2023 19:21
@hishamhm hishamhm force-pushed the feat/wasm-dynamic-properties branch 2 times, most recently from fb453b1 to f4d9780 Compare November 17, 2023 20:53
@hishamhm
Copy link
Contributor Author

hishamhm commented Nov 17, 2023

Blocker for this PR: the .requirements change needs to be altered to point to ngx_wasm_module's latest main once Kong/ngx_wasm_module#431 gets merged.

kong/runloop/wasm.lua Outdated Show resolved Hide resolved
@hishamhm hishamhm force-pushed the feat/wasm-dynamic-properties branch 3 times, most recently from 06853b5 to 5cc2f77 Compare November 28, 2023 20:57
@hishamhm hishamhm marked this pull request as ready for review November 28, 2023 20:57
@hishamhm hishamhm requested review from locao and flrgh November 28, 2023 21:01
Copy link
Contributor

@flrgh flrgh left a comment

Choose a reason for hiding this comment

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

Nicely done!

@flrgh
Copy link
Contributor

flrgh commented Dec 4, 2023

note: we should rebase this instead of squashing to preserve the history on commits like 5cc2f77

Copy link
Contributor

@locao locao left a comment

Choose a reason for hiding this comment

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

🚀

hishamhm and others added 3 commits December 5, 2023 18:02
…67ca53aa

* chore(deps): bump ngx_wasm_module to b51a15fc972540e6b8964e2fe1d86ebf67ca53aa

Changes since ddb3fa8f7cacc81557144cf22706484eabd79a84:

* b51a15f - chore(*) add a .gitattributes file
* 9959389 - fix(*) resolve a possible segfault in the FFI
* 8c45ad1 - fix(*) proper filter modules order in dynamic OpenResty builds
* 33157a8 - feat(proxy-wasm) custom host properties getters/setters
* 81c703e - docs(*) minor fix for a title level
* db88b15 - fix(proxy-wasm) free dispatch calls during resume edge-case
* 5553ae0 - feat(proxy-wasm) strengthen host functions context checks
Co-Authored-By: Hisham Muhammad <hisham@gobolinux.org>
This prevents triggering a LuaJIT issue when attempting to call an FFI
callback with an ongoing trace further down the stack; attempting to do so can
trigger a "bad callback" assertion.

Stack trace demonstrating the issue in question:

```
   from /home/zhongweiyao/projects/kong/bazel-bin/build/kong-dev/openresty/nginx/sbin/../modules/ngx_wasm_module.so
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/wasm/wrt/ngx_wrt_wasmtime.c:657
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/wasm/vm/ngx_wavm.c:1107
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/wasm/vm/ngx_wavm.c:1184
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/wasm/vm/ngx_wavm.c:1287
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/http/proxy_wasm/ngx_http_proxy_wasm.c:40
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/http/proxy_wasm/ngx_http_proxy_wasm.c:411
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/common/proxy_wasm/ngx_proxy_wasm.c:658
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/common/proxy_wasm/ngx_proxy_wasm.c:783
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/wasm/ngx_wasm_ops.c:417
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/wasm/ngx_wasm_ops.c:290
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/common/lua/ngx_wasm_lua_ffi.c:164
    at ../ngx_lua-0.10.25/src/ngx_http_lua_util.c:1184
    respawn=-3) at src/os/unix/ngx_process.c:199
```

The problem arises when Wasm code eventually calls the FFI callback which triggers Lua code while having an ongoing
trace in the stack (see frame 12, `TRACE_1054`, in the example above).

Eventually the LuaJIT callback crashes like this:

```
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/common/proxy_wasm/ngx_proxy_wasm_properties.c:1058
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/common/proxy_wasm/ngx_proxy_wasm_host.c:780
    at /home/zhongweiyao/.cache/bazel/_bazel_zhongweiyao/7df4419a5ca351a16fa75df771d28bc8/execroot/kong/external/ngx_wasm_module/src/wasm/vm/ngx_wavm_host.c:265
   from /home/zhongweiyao/projects/kong/bazel-bin/build/kong-dev/openresty/nginx/sbin/../modules/ngx_wasm_module.so
   from /home/zhongweiyao/projects/kong/bazel-bin/build/kong-dev/openresty/nginx/sbin/../modules/ngx_wasm_module.so
   from /home/zhongweiyao/projects/kong/bazel-bin/build/kong-dev/openresty/nginx/sbin/../modules/ngx_wasm_module.so
   from /home/zhongweiyao/projects/kong/bazel-bin/build/kong-dev/openresty/nginx/sbin/../modules/ngx_wasm_module.so
      ...
```

Here's some sample minimal code to reproduce the LuaJIT issue outside of the Gateway:

```lua
-- Lua code
local ffi = require("ffi")
local C = ffi.C

ffi.cdef [[
    typedef int (*my_fn_t)(int, int, int);
    int f2();
    void setup(my_fn_t f, int, int, int);
]]

local lib = ffi.load("test")

function setup(cb, a, b, c)
  lib.setup(cb, a, b, c)
end

function f0()
  return lib.f2() + 1
end

do
  local cb = ffi.cast("my_fn_t", function(a, b, c)
    return a+b+c
  end)
  setup(cb, 10, 99, 13)
  print(f0())

  for i=1,300 do
    if i > 60 then f0() end
  end
end
```

```c
/* C code */
typedef int (*my_fn_t)(int, int, int);

my_fn_t gf = 0;
int ga;
int gb;
int gc;

void setup(my_fn_t f, int a, int b, int c) {
  gf = f;
  ga = a;
  gb = b;
  gc = c;
}

int f2() {
  return gf(ga, gb, gc) + 1;
}
```

The issue in question has been a known for a long time. See:

https://luajit.freelists.narkive.com/sdhSLJSr/how-to-make-bad-callback-more-deterministic

```
The bad callback error happens because some JIT-compiled Lua code calls a C
function which in turn calls an FFI callback.
```

https://lua-l.lua.narkive.com/qXJrNlpP/luajit-ffi-windows-bad-callback-error-in-msgwaitformultipleobjects-proof-of-concept

From Mike Pall:

```
The problem is that a FFI callback cannot safely be called from a
C function which is itself called via the FFI from JIT-compiled
code. In your case this is the call to MsgWaitForMultipleObjects.

I've put in a lot of heuristics to detect this, and it usually
succeeds in disabling compilation for such a function. However in
your case the loop is compiled before the callback is ever called,
so the detection fails.

The straighforward solution is to put the message loop into an
extra Lua function and use jit.off(func)
```

Signed-off-by: Hisham Muhammad <hisham@gobolinux.org>
@locao locao force-pushed the feat/wasm-dynamic-properties branch from 5cc2f77 to 6e4acb6 Compare December 5, 2023 21:02
@locao locao merged commit a796ac6 into master Dec 5, 2023
24 checks passed
@locao locao deleted the feat/wasm-dynamic-properties branch December 5, 2023 21:41
@bungle bungle added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Jan 25, 2024
@team-gateway-bot
Copy link
Collaborator

Cherry-pick failed for master, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git remote add upstream https://github.com/kong/kong-ee
git fetch upstream master
git worktree add -d .worktree/cherry-pick-11856-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-11856-to-master-to-upstream
git checkout -b cherry-pick-11856-to-master-to-upstream
ancref=$(git merge-base 8e5cb497b73512afa972c18eda07232c8a94ead6 6e4acb68a7cdd4bf7ea5a56259271bf66157b953)
git cherry-pick -x $ancref..6e4acb68a7cdd4bf7ea5a56259271bf66157b953

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/proxy size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants