-
Notifications
You must be signed in to change notification settings - Fork 100
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: per call context #711
Changes from 1 commit
4168d2e
68da28b
e412d98
3149606
4794c19
8a6b78c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
(module | ||
(global (export "extism_context") (mut externref) (ref.null extern)) | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -307,6 +307,43 @@ fn test_toml_manifest() { | |
assert_eq!(count.get("count").unwrap().as_i64().unwrap(), 1); | ||
} | ||
|
||
#[test] | ||
fn test_call_with_context() { | ||
// assert!(set_log_file("stdout", Some(log::Level::Trace))); | ||
#[derive(Clone)] | ||
struct Foo { | ||
message: String, | ||
} | ||
|
||
let f = Function::new( | ||
"host_reflect", | ||
[PTR], | ||
[PTR], | ||
UserData::default(), | ||
|current_plugin, _val, ret, _user_data: UserData<()>| { | ||
let foo = current_plugin.context::<Foo>()?; | ||
let hnd = current_plugin.memory_new(foo.message)?; | ||
ret[0] = current_plugin.memory_to_val(hnd); | ||
Ok(()) | ||
}, | ||
); | ||
|
||
let mut plugin = Plugin::new(WASM_REFLECT, [f], true).unwrap(); | ||
|
||
let message = "hello world"; | ||
let output: String = plugin | ||
.call_with_context( | ||
"reflect", | ||
"anything, really", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hah, this was my initial thought - "then what's the input?" but I realized that the there could be some confusion around what to pass as input vs context (especially since we used to have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point – I renamed to |
||
Foo { | ||
message: message.to_string(), | ||
}, | ||
) | ||
.unwrap(); | ||
|
||
assert_eq!(output, message); | ||
} | ||
|
||
#[test] | ||
fn test_fuzz_reflect_plugin() { | ||
// assert!(set_log_file("stdout", Some(log::Level::Trace))); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a layer of protection around this by saying that "we don't accept any Wasm module that imports
extism_context
." (This would protect against a malicious plugin that imports this externref, holds onto it across two calls, replacing the global value of the second call with the value from the first call.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah my first thought is we don't want people to be able to poke this and use some other plugin-runner's context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returning an error if a plugin is trying to import the global directly sounds like a good idea - I also noticed that the context global gets reset before each call regardless of whether
call_with_context
is used, which makes me confident that a plugin couldn't access the context from another call.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks – added that in 68da28b.
This commit also prevents linked modules from importing the extism kernel's memory directly (which I think would lead to similar problems.) I don't know of anyone doing that today – it would certainly seem to void the warranty label – but I can narrow up that check to just the context if we're worried!