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(hostcallDuringLoad): Allow updating the context with InvokeContext #55

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

ritesh089
Copy link
Contributor

wapc go 's wazero runtime allows web assembly modules to make host calls to request the host runtime to execute some tasks that web assembly is not capable of .
wapc 's host call is limited to the guest module's explicitly exported function that gets called during the invoke.
This PR allows the host to update with InvocationContext prior to wasm instantiation. This allows the wasm to perform hostcalls during the loading phase.

Value Added by this PR:
This PR is important because the wasm can now fetch environment variables during the instantiation phase while utilizing the benefits provided by wapc.

If there is a need to perform hostcalls from main, prior to instantiation of the wasm, a line of code is to be added :
m.ctx = wazero.AddInvokeContext(m.ctx)

@pkedy
Copy link
Member

pkedy commented Sep 22, 2023

Hi @ritesh089 - thanks for this PR and the clear explanation of the added value.

First, I'm considering the security impact of this change. External access during initialization is considered insecure in other Wasm plugin projects I've worked on. The Wasm module could be untrusted code. That said, waPC works a little differently, and no concern immediately comes to mind, but I want to think about it some more.

Second, if we assume this is safe, it should be consistent across all engines (e.g., Wasmtime, Wasmer). That said, the approach would differ. With Wazero, I think it would make sense to change WithStartFunctions(functionStart, functionInit) to WithStartFunctions() and call those functions manually with a context initialized with a context like in Invoke:

	ic := invokeContext{operation: operation, guestReq: payload}
	ctx = newInvokeContext(ctx, &ic)

I wanted to run my thinking by you. Thoughts?

/cc: @codefromthecrypt Do you have thoughts on the security aspect of this feature?

@ritesh089
Copy link
Contributor Author

ritesh089 commented Sep 23, 2023

hi @pkedy ,
Since Wazero already supports calling host functions during main directly, I am assuming it is safe.
Wapc go uses the invoke context which will be nil when called from _start because of which the call fails.
Changing to WithStartFunctions() will still call it from main and you will still need the above method to use the invocation context.
This PR would still be useful in that case as well. So it supports both the options. But I think calling _start explicitly requires additional steps with not much change in behavior.

I am not clear about the security impact of this change. I am trying to understand how we can identify that a wasm module is a trusted source in its _start function.

@pkedy
Copy link
Member

pkedy commented Sep 25, 2023

Changing to WithStartFunctions() will still call it from main and you will still need the above method to use the invocation context.

@ritesh089 I wasn't sure if I followed the "will still call it from main" part of this comment, but you are correct about the invocation context.

Check out https://github.com/wapc/wapc-go/compare/feature/host_calls_during_instantiate?expand=1 and see if we are on the same page. The other engines would likely need to be tweaked as well. I welcome contributions, so feel free to snag my changes for this PR if it was what you were thinking above.

@ritesh089
Copy link
Contributor Author

Thanks @pkedy ,
Yes these changes work . Will update my PR accordingly .

@ritesh089
Copy link
Contributor Author

@pkedy , have updated the PR. Can you review and let me know if I need to make any additional changes?

@pkedy pkedy merged commit 83499bd into wapc:master Sep 26, 2023
2 checks passed
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.

2 participants