WASIp2 component support for wasmtime #1877
Conversation
Reviewer's GuideThis PR extends the Wasmtime handler to detect whether a Wasm input is a module or component, refactors symbol loading into a helper and macro, extracts module execution into its own function, adds a new WASIp2 component execution path, and updates includes and documentation accordingly. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
So I wanted to implement the Is this by choice or a bug? |
|
@flouthoc PTAL |
Signed-off-by: Maximilian Hüter <[email protected]>
Signed-off-by: Maximilian Hüter <[email protected]>
Signed-off-by: Maximilian Hüter <[email protected]>
t4chib4ne
force-pushed
the
wasmtime_wasip2
branch
from
0074108 to
ce8441d
Compare
|
Thanks for taking a look, hope I didn't oversee anything! The only thing keeping this PR as a draft is that I would like to give both WASM modules and components the ability to read & write to the working directory. But the previous implementation did not allow this. Are there any security concerns or can this feature simply be added? |
src/libcrun/handlers/wasmtime.c
Outdated
|
|
||
| wasm_byte_vec_t wasm; | ||
| // Load and parse container entrypoint | ||
| FILE *file = fopen (pathname, "rbe"); |
There was a problem hiding this comment.
This part of the code was just copied. The e seems to be a glibc extension which sets the O_CLOEXEC flag on the descriptor. This flag causes the fd to be automatically closed upon calling one of the exec functions.
A few lines later the fd is actually closed by hand and none the exec functions are visibly being called. As e is not standards compliant maybe it should be removed?
Sources: fopen(3) and open(2) man pages
There was a problem hiding this comment.
As e is not standards compliant maybe it should be removed?
Yes if it's not necessary I think we should remove it, wdyt @giuseppe
There was a problem hiding this comment.
Some extra info: Closing the fd manually later does not prevent all issues O_CLOEXEC tries to solve. The glibc man page states that it is a glibc extension so I figured it would not be standards compliant but musl and FreeBSDs libc actually also implement the e in fopen.
Sources: musl and FreeBSDs libc
There was a problem hiding this comment.
I think we can remove it if it is not required, I see similar comment later from review bot #1877 (comment)
There was a problem hiding this comment.
After reading other parts of the code for quite some time, I think it is safe to say e is not required. This is also encouraged by the fact that right before the fopen we are always in a single threaded context thus the mentioned leak in open(2) is not possible.
| FROM scratch | ||
| COPY hello.wasm / | ||
| CMD ["/hello.wasm"] | ||
| ENTRYPOINT ["/hello.wasm"] |
There was a problem hiding this comment.
Could you write it in commit message or here, about why is this change needed ?
There was a problem hiding this comment.
Oh, sorry forgot to mention it ...
After adding the pass through of argv to the component I wanted to try it out. Upon running podman run mywasm-image:latest arg1 arg2 podman would replace CMD (the wasm binary) with arg1 which of course does not work. Changing to ENTRYPOINT resolves this.
There was a problem hiding this comment.
This looks like a breaking change. I think adding this message to commit logs can help us revisit this and fix this if needed.
cc @giuseppe
There was a problem hiding this comment.
Adding this to the commit logs should be possible but I don't really see where this is a breaking change? Without the changes of this PR a wasm module runs into the same problem. IIRC this is also the same behavior a non-wasm container would show.
There was a problem hiding this comment.
Is the message in ce7d656 OK?
…sm-wasi-example In docs/wasm-wasi-example.md the Containerfile had the WebAssembly binary in the `CMD` instruction. But running the container via podman like so `podman run mywasm-image:latest arg1 arg2` does not work as podman instructs crun to run `arg1 arg2` instead of `hello.wasm arg1 arg2` resulting in the error `arg1: command not found`. Using `ENTRYPOINT` in the Containerfile makes the previously mentioned podman command work. Signed-off-by: Maximilian Hüter <[email protected]>
Signed-off-by: Maximilian Hüter <[email protected]>
Signed-off-by: Maximilian Hüter <[email protected]>
Signed-off-by: Maximilian Hüter <[email protected]>
t4chib4ne
force-pushed
the
wasmtime_wasip2
branch
from
31fd57e to
9a073f6
Compare
Signed-off-by: Maximilian Hüter <[email protected]>
Signed-off-by: Maximilian Hüter <[email protected]>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extracting the repeated dlsym() lookups into a helper utility that takes an array of symbol names and returns function pointers, reducing duplicate code.
- Relying on matching the literal error string “component passed to module validation” for component detection is brittle; consider using a dedicated component detection API or inspecting the Wasm magic/header instead.
- When a symbol lookup fails, include the specific missing symbol name in the error output to make debugging dynamic loading issues easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the repeated dlsym() lookups into a helper utility that takes an array of symbol names and returns function pointers, reducing duplicate code.
- Relying on matching the literal error string “component passed to module validation” for component detection is brittle; consider using a dedicated component detection API or inspecting the Wasm magic/header instead.
- When a symbol lookup fails, include the specific missing symbol name in the error output to make debugging dynamic loading issues easier.
## Individual Comments
### Comment 1
<location> `src/libcrun/handlers/wasmtime.c:91` </location>
<code_context>
+
+ wasm_byte_vec_t wasm;
+ // Load and parse container entrypoint
+ FILE *file = fopen (pathname, "rbe");
+ if (! file)
+ error (EXIT_FAILURE, 0, "error loading entrypoint");
</code_context>
<issue_to_address>
**issue:** The use of 'rbe' as the fopen mode is non-standard and may cause portability issues.
Use 'rb' for reading binary files unless 'rbe' is required and confirmed to be supported across all target platforms.
</issue_to_address>
### Comment 2
<location> `src/libcrun/handlers/wasmtime.c:107` </location>
<code_context>
+ // If entrypoint contains a webassembly text format
+ // compile it on the fly and convert to equivalent
+ // binary format.
+ if (has_suffix (pathname, "wat") > 0)
+ {
+ wasmtime_error_t *err = wasmtime_wat2wasm ((char *) wasm.data, file_size, &wasm_bytes);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Suffix check for 'wat' may match unintended filenames.
This check may incorrectly match filenames like 'mywat' or 'format'. Use '.wat' as the suffix to ensure only intended files are processed.
```suggestion
if (has_suffix (pathname, ".wat") > 0)
```
</issue_to_address>
### Comment 3
<location> `src/libcrun/handlers/wasmtime.c:129` </location>
<code_context>
+ wasmtime_error_message (err, &error_message);
+ wasmtime_error_delete (err);
+
+ if (strcmp ((char *) error_message.data, "component passed to module validation") != 0)
+ error (EXIT_FAILURE, 0, "failed to validate module: %.*s", (int) error_message.size, error_message.data);
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** String comparison for error message is brittle and may break with upstream changes.
Consider using an error code or a more stable identifier instead of matching the error message string, to avoid breakage from upstream changes.
Suggested implementation:
```c
wasmtime_error_message (err, &error_message);
// Use a more robust check for the error type.
// If wasmtime_error_as_exit_status is available, use it.
int exit_status = 0;
bool is_component_error = false;
#ifdef WASMTIME_HAS_ERROR_EXIT_STATUS
if (wasmtime_error_as_exit_status(err, &exit_status)) {
// Check for a specific exit status if known for component error.
if (exit_status == WASMTIME_COMPONENT_VALIDATION_ERROR_CODE) {
is_component_error = true;
}
}
#endif
// Fallback: check for substring in error message.
if (!is_component_error) {
if (memmem(error_message.data, error_message.size, "component passed to module validation", strlen("component passed to module validation")) == NULL)
error (EXIT_FAILURE, 0, "failed to validate module: %.*s", (int) error_message.size, error_message.data);
}
wasmtime_error_delete (err);
```
- You may need to define `WASMTIME_HAS_ERROR_EXIT_STATUS` and `WASMTIME_COMPONENT_VALIDATION_ERROR_CODE` if the wasmtime API provides such error codes. If not, you can remove the conditional and rely on the substring check.
- If you have a helper function for error type checking, use that instead of the inline logic.
- Make sure to include the necessary headers for `memmem` if not already present.
</issue_to_address>
### Comment 4
<location> `src/libcrun/handlers/wasmtime.c:278` </location>
<code_context>
wasi_config_inherit_stdout (wasi_config);
wasi_config_inherit_stderr (wasi_config);
- wasi_config_preopen_dir (wasi_config, ".", ".");
+ wasi_config_preopen_dir (
+ wasi_config,
+ ".",
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Hardcoded permissions for preopened directory may be overly permissive.
Evaluate if both read and write permissions are required, or if access can be restricted to enhance security.
Suggested implementation:
```c
WASMTIME_WASI_DIR_PERMS_READ,
WASMTIME_WASI_FILE_PERMS_READ);
```
If your application requires write access, you can revert to the original permissions. Otherwise, this change will restrict access to read-only, improving security.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Maximilian Hüter <[email protected]>
Signed-off-by: Maximilian Hüter <[email protected]>
Signed-off-by: Maximilian Hüter <[email protected]>
Signed-off-by: Maximilian Hüter <[email protected]>
Signed-off-by: Maximilian Hüter <[email protected]>
| was defined by the component spec. | ||
| See: https://github.com/WebAssembly/component-model/blob/main/design/mvp/Binary.md#component-definitions | ||
| */ | ||
| if (header[6] == '\0' && header[7] == '\0') |
There was a problem hiding this comment.
you need to make sure the header length is at least 8 before accessing these bytes.
Since you already need the length for the first comment, I'd suggest to store the header length first before any other check (size_t header_len = strlen (header);)
There was a problem hiding this comment.
I agree but strlen could terminate instantly for a correct header or never if we are unlucky because header just contains "random" bytes.
header is just a pointer to the whole wasm binary which we know the size of. So the function could also take that size as a parameter and check for at least a length of 8.
| wasmtime_error_message (err, &error_message); | ||
| wasmtime_error_delete (err); | ||
| error (EXIT_FAILURE, 0, "failed to compile component: %.*s", (int) error_message.size, error_message.data); |
There was a problem hiding this comment.
this can be done only if err != nil. The code could end up here with component == NULL as well. Not sure if it is possible, but should be clarified:
if (err != NULL)
{
// what we do now
}
if (component == NULL)
error (EXIT_FAILURE, 0, "internal error: component is NULL");
There was a problem hiding this comment.
Good catch!
I must have gotten confused with the already present module code which only checks module == NULL and then prints the error.
The docs of both functions suggest err will always be non-NULL in case of an error but I think briefly checking for a NULL module/component does not hurt.
|
I marked the PR as a draft again because I don't want to trigger CI all the time. There is also a bigger change in the works which cleans up all of the Edit: Seems like CI still get triggered ... and it's now just one macro. |
Signed-off-by: Maximilian Hüter <[email protected]>
Signed-off-by: Maximilian Hüter <[email protected]>
t4chib4ne
force-pushed
the
wasmtime_wasip2
branch
from
4ddd82a to
9b541ea
Compare
Signed-off-by: Maximilian Hüter <[email protected]>
Signed-off-by: Maximilian Hüter <[email protected]>
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The symbol loading boilerplate is duplicated heavily between the module and component paths—consider refactoring common symbol initialisation (and WASI/WASIp2 config setup) into shared helper functions to reduce code duplication.
- Rather than calling exit() deep within the handler when errors occur, propagate errors back to the caller (e.g. by returning an error code) so that higher-level logic can perform any necessary cleanup.
- You’ve hardcoded the CLI interface version to "wasi:cli/[email protected]"; consider making this configurable or querying the highest supported version dynamically to avoid stale bindings as newer WASI CLI versions are released.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The symbol loading boilerplate is duplicated heavily between the module and component paths—consider refactoring common symbol initialisation (and WASI/WASIp2 config setup) into shared helper functions to reduce code duplication.
- Rather than calling exit() deep within the handler when errors occur, propagate errors back to the caller (e.g. by returning an error code) so that higher-level logic can perform any necessary cleanup.
- You’ve hardcoded the CLI interface version to "wasi:cli/[email protected]"; consider making this configurable or querying the highest supported version dynamically to avoid stale bindings as newer WASI CLI versions are released.
## Individual Comments
### Comment 1
<location> `src/libcrun/handlers/wasmtime.c:86-88` </location>
<code_context>
+ FILE *file = fopen (pathname, "rb");
+ if (! file)
+ error (EXIT_FAILURE, 0, "error loading entrypoint");
+ if (fseek (file, 0L, SEEK_END))
+ error (EXIT_FAILURE, 0, "error fully loading entrypoint");
+ size_t file_size = ftell (file);
+ wasm_byte_vec_new_uninitialized (&wasm, file_size);
+ if (fseek (file, 0L, SEEK_SET))
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential issue with ftell error handling.
Check that file_size is not (size_t)-1 after ftell to prevent passing an invalid size to wasm_byte_vec_new_uninitialized.
</issue_to_address>
### Comment 2
<location> `src/libcrun/handlers/wasmtime.c:92` </location>
<code_context>
+ wasm_byte_vec_new_uninitialized (&wasm, file_size);
+ if (fseek (file, 0L, SEEK_SET))
+ error (EXIT_FAILURE, 0, "error resetting entrypoint");
+ if (fread (wasm.data, file_size, 1, file) != 1)
+ error (EXIT_FAILURE, 0, "error reading entrypoint");
+ fclose (file);
</code_context>
<issue_to_address>
**issue (bug_risk):** No cleanup on fread failure.
The wasm buffer should be freed if fread fails to prevent a memory leak.
</issue_to_address>
### Comment 3
<location> `src/libcrun/handlers/wasmtime.c:47` </location>
<code_context>
#endif
#if HAVE_DLOPEN && HAVE_WASMTIME
+static void *
+libwasmtime_load_symbol (void *cookie, char *const symbol);
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the repeated dlsym symbol loading into an X-macro and struct-based helper to centralize and simplify symbol management.
Here’s one way to collapse almost all of the “declare + dlsym” boilerplate into an X-macro and a small helper. This keeps exactly the same semantics and error‐handling but cuts your dozens of nearly‐identical lines down to just a single list plus two small macros:
```c
// 1) Define your list of symbols once:
#define WASMTIME_SYM_LIST(V) \
V(wasm_engine_new) \
V(wasm_engine_delete) \
V(wasmtime_store_new) \
V(wasmtime_store_delete) \
V(wasmtime_store_context) \
V(wasmtime_linker_new) \
V(wasmtime_linker_define_wasi) \
V(wasmtime_module_new) \
V(wasmtime_component_new) \
V(wasmtime_wat2wasm) \
V(wasm_byte_vec_new_uninitialized) \
V(wasm_byte_vec_delete) \
V(wasmtime_error_message) \
V(wasmtime_error_delete) \
/* …add only the rest once here… */ \
V(wasmtime_func_call) \
V(wasmtime_component_func_call) \
V(wasi_config_new) \
V(wasi_config_set_argv) \
V(wasi_config_inherit_env) \
V(wasi_config_inherit_stdin) \
/* etc… */
// 2) Expand it into declarations in a struct:
typedef struct {
#define DECL_FN(name) typeof(name) * name;
WASMTIME_SYM_LIST(DECL_FN)
#undef DECL_FN
} wasmtime_syms_t;
// 3) Provide a loader that loops once over the list:
static void load_wasmtime_syms(void *cookie, wasmtime_syms_t *s) {
#define LOAD_FN(name) \
do { \
s->name = dlsym(cookie, #name); \
if (s->name == NULL) \
error(EXIT_FAILURE, 0, "could not find `%s` in libwasmtime.so", #name); \
} while(0);
WASMTIME_SYM_LIST(LOAD_FN)
#undef LOAD_FN
}
// 4) In your run_module / run_component, just do:
static void
libwasmtime_run_module(void *cookie, char *const argv[], wasm_engine_t *engine, wasm_byte_vec_t *wasm)
{
wasmtime_syms_t s;
load_wasmtime_syms(cookie, &s);
// …then use s.wasmtime_store_new, s.wasmtime_store_delete, etc…
}
// 5) Same for run_component.
```
Benefits:
- You keep one single list of symbols (`WASMTIME_SYM_LIST`) rather than dozens of copy-pasted dlsym calls.
- Adding/removing a symbol is now a one-liner in that list.
- The loader macro handles failures uniformly.
- You keep all functionality exactly the same.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (fseek (file, 0L, SEEK_END)) | ||
| error (EXIT_FAILURE, 0, "error fully loading entrypoint"); | ||
| size_t file_size = ftell (file); |
There was a problem hiding this comment.
issue (bug_risk): Potential issue with ftell error handling.
Check that file_size is not (size_t)-1 after ftell to prevent passing an invalid size to wasm_byte_vec_new_uninitialized.
| wasm_byte_vec_new_uninitialized (&wasm, file_size); | ||
| if (fseek (file, 0L, SEEK_SET)) | ||
| error (EXIT_FAILURE, 0, "error resetting entrypoint"); | ||
| if (fread (wasm.data, file_size, 1, file) != 1) |
There was a problem hiding this comment.
issue (bug_risk): No cleanup on fread failure.
The wasm buffer should be freed if fread fails to prevent a memory leak.
| #endif | ||
|
|
||
| #if HAVE_DLOPEN && HAVE_WASMTIME | ||
| static void * |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the repeated dlsym symbol loading into an X-macro and struct-based helper to centralize and simplify symbol management.
Here’s one way to collapse almost all of the “declare + dlsym” boilerplate into an X-macro and a small helper. This keeps exactly the same semantics and error‐handling but cuts your dozens of nearly‐identical lines down to just a single list plus two small macros:
// 1) Define your list of symbols once:
#define WASMTIME_SYM_LIST(V) \
V(wasm_engine_new) \
V(wasm_engine_delete) \
V(wasmtime_store_new) \
V(wasmtime_store_delete) \
V(wasmtime_store_context) \
V(wasmtime_linker_new) \
V(wasmtime_linker_define_wasi) \
V(wasmtime_module_new) \
V(wasmtime_component_new) \
V(wasmtime_wat2wasm) \
V(wasm_byte_vec_new_uninitialized) \
V(wasm_byte_vec_delete) \
V(wasmtime_error_message) \
V(wasmtime_error_delete) \
/* …add only the rest once here… */ \
V(wasmtime_func_call) \
V(wasmtime_component_func_call) \
V(wasi_config_new) \
V(wasi_config_set_argv) \
V(wasi_config_inherit_env) \
V(wasi_config_inherit_stdin) \
/* etc… */
// 2) Expand it into declarations in a struct:
typedef struct {
#define DECL_FN(name) typeof(name) * name;
WASMTIME_SYM_LIST(DECL_FN)
#undef DECL_FN
} wasmtime_syms_t;
// 3) Provide a loader that loops once over the list:
static void load_wasmtime_syms(void *cookie, wasmtime_syms_t *s) {
#define LOAD_FN(name) \
do { \
s->name = dlsym(cookie, #name); \
if (s->name == NULL) \
error(EXIT_FAILURE, 0, "could not find `%s` in libwasmtime.so", #name); \
} while(0);
WASMTIME_SYM_LIST(LOAD_FN)
#undef LOAD_FN
}
// 4) In your run_module / run_component, just do:
static void
libwasmtime_run_module(void *cookie, char *const argv[], wasm_engine_t *engine, wasm_byte_vec_t *wasm)
{
wasmtime_syms_t s;
load_wasmtime_syms(cookie, &s);
// …then use s.wasmtime_store_new, s.wasmtime_store_delete, etc…
}
// 5) Same for run_component.Benefits:
- You keep one single list of symbols (
WASMTIME_SYM_LIST) rather than dozens of copy-pasted dlsym calls. - Adding/removing a symbol is now a one-liner in that list.
- The loader macro handles failures uniformly.
- You keep all functionality exactly the same.
There was a problem hiding this comment.
First this seems a lot more complex than the current solution. Second is the IDE still able to suggest parameters of those loaded functions? The provided sample code does not seem to contain any information about the loaded function other than their name.
There was a problem hiding this comment.
Great questions! You're right that the X-macro approach can look more complex at first glance, and IDE support for parameter hints/type checking depends on how you define the macro and struct.
Type Safety & IDE Support:
- If you use only symbol names in the macro, the struct will have generic pointers (e.g.,
void *), and you'll lose type safety and IDE parameter hints. - To preserve type safety and IDE support, you can expand the macro to include both the symbol name and its function pointer type. This way, the struct fields have the correct types, and your IDE can still provide parameter hints and catch type errors.
Example:
Here's how you can modify the X-macro to keep type information:
// 1) Define your symbol list with types:
#define WASMTIME_SYM_LIST(V) \
V(wasm_engine_new, wasm_engine_t *(*)()) \
V(wasm_engine_delete, void (*)(wasm_engine_t *)) \
V(wasmtime_store_new, wasmtime_store_t *(*)(wasm_engine_t *, void *, void (*)(void *))) \
/* ... add the rest ... */
// 2) Expand into a struct with correct types:
typedef struct {
#define DECL_FN(name, type) type name;
WASMTIME_SYM_LIST(DECL_FN)
#undef DECL_FN
} wasmtime_syms_t;Now, each field in wasmtime_syms_t has the correct type, so your IDE will show parameter hints and catch type errors as usual.
Loading symbols:
You can still loop over the macro to load each symbol, but you'll need a helper macro or function for each type (or use a cast).
Summary:
- The X-macro approach can be just as type-safe and IDE-friendly as the current method, if you include types in the macro.
- It centralizes symbol management, making it easier to add/remove symbols and reduces boilerplate.
- If you prefer the current explicit style for clarity, that's totally valid too!
Let me know if you'd like a more complete example or have other concerns about this approach.
Adds WASIp2 component support for wasmtime by checking whether the given WebAssembly is a module or a component and then calling the appropriate functions.
The calls are nothing fancy just a very basic setup giving the WebAssembly component full access to the WASIp2 implementation of wasmtime.
Closes #1871
Still a Draft because of:
wasi_config_preopen_dir⇒ calling this function for a WASIp2 config also workswasi:cli/[email protected]#runshould be fine as wasmtime will call the highest version available (e.g.wasi:cli/[email protected]#run)Summary by Sourcery
Enable WASIp2 component support in wasmtime by detecting if the input is a module or component and invoking the appropriate execution path with dynamically loaded Wasmtime APIs and WASIp2 configuration
New Features:
Enhancements:
Documentation:
Summary by Sourcery
Enable WASIp2 component support in the wasmtime handler by detecting WebAssembly encoding and dispatching to separate execution paths for modules and components, dynamically loading required Wasmtime C API symbols, and configuring WASIp2 contexts with inherited I/O, arguments, environment, and preopened directories.
New Features:
Enhancements:
Documentation: