fix: validate linker intrinsic function signatures - #2646
Merged
alexcrichton merged 2 commits intoSep 14, 2026
Merged
alexcrichton merged 2 commits into
alexcrichton merged 2 commits into
Conversation
alexcrichton
requested review from
alexcrichton
and removed request for
fitzgen
September 9, 2026 14:23
alexcrichton
approved these changes
Sep 9, 2026
alexcrichton
left a comment
alexcrichton
left a comment
Member
There was a problem hiding this comment.
Thanks! Some minor comments on tests, but otherwise seems reasonable to me.
Comment on lines
+202
to
+232
| #[test] | ||
| fn linker_intrinsic_signatures() -> Result<()> { | ||
| for (namespace, name, signature) in [ | ||
| ("env", "__wasm_get_stack_pointer", "(result i32)"), | ||
| ("env", "__wasm_set_stack_pointer", "(param i32)"), | ||
| ("env", "__wasm_get_tls_base", "(result i32)"), | ||
| ("env", "__wasm_set_tls_base", "(param i32)"), | ||
| ( | ||
| "$root", | ||
| "[thread-new-indirect-v0]", | ||
| "(param i32 i32) (result i32)", | ||
| ), | ||
| ] { | ||
| link_intrinsic(namespace, name, signature, true) | ||
| .with_context(|| format!("failed to link {namespace}.{name}"))?; | ||
| } | ||
| link_intrinsic_wat( | ||
| r#" | ||
| (module | ||
| (@dylink.0) | ||
| (type (func)) | ||
| (type (func (result i32))) | ||
| (import "env" "__wasm_get_stack_pointer" (func (type 1))) | ||
| (func (export "run") (type 0)) | ||
| ) | ||
| "#, | ||
| true, | ||
| ) | ||
| .context("failed to link an intrinsic at a nonzero type index")?; | ||
| Ok(()) | ||
| } |
Member
There was a problem hiding this comment.
Where possible I personally prefer to have tests in tests/components/* rather than unit tests, and I think tests like this can move there. That being said is this testing something that's not already tested? I would have figured we've already got tests for this.
| )), | ||
| "{error}" | ||
| ); | ||
| } |
Member
There was a problem hiding this comment.
Could you move these tests to tests/components/*?
Comment on lines
+307
to
+372
| #[test] | ||
| fn linker_intrinsic_rejects_wrong_kind_and_unsupported_type() { | ||
| let error = link_intrinsic_wat( | ||
| r#" | ||
| (module | ||
| (@dylink.0) | ||
| (import "$root" "[thread-new-indirect-v0]" (global i32)) | ||
| (func (export "run")) | ||
| ) | ||
| "#, | ||
| false, | ||
| ) | ||
| .unwrap_err(); | ||
| assert!( | ||
| format!("{error:#}") | ||
| .contains("unexpected type for $root:[thread-new-indirect-v0]: Global(GlobalType") | ||
| ); | ||
|
|
||
| let error = | ||
| link_intrinsic("env", "__wasm_set_stack_pointer", "(param v128)", false).unwrap_err(); | ||
| let error = format!("{error:#}"); | ||
| assert!(error.contains("failed to read function type for `env.__wasm_set_stack_pointer`")); | ||
| assert!(error.contains("V128 not yet supported")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn linker_intrinsic_rejects_invalid_type_index() { | ||
| use { | ||
| std::borrow::Cow, | ||
| wasm_encoder::{CustomSection, EntityType, ImportSection, Module, TypeSection}, | ||
| }; | ||
|
|
||
| let mut module = Module::new(); | ||
| module.section(&CustomSection { | ||
| name: Cow::Borrowed("dylink.0"), | ||
| data: Cow::Borrowed(&[]), | ||
| }); | ||
| let mut types = TypeSection::new(); | ||
| types.ty().function([], []); | ||
| module.section(&types); | ||
| let mut imports = ImportSection::new(); | ||
| imports.import("env", "__wasm_get_stack_pointer", EntityType::Function(1)); | ||
| module.section(&imports); | ||
|
|
||
| let module = module.finish(); | ||
| let mut linker = wit_component::Linker::default(); | ||
| linker.library("app.wasm", &module, false).unwrap(); | ||
| let error = linker.encode().unwrap_err(); | ||
| assert!( | ||
| format!("{error:#}") | ||
| .contains("invalid function type index 1 for `env.__wasm_get_stack_pointer`") | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn linker_intrinsic_requires_matching_namespace() { | ||
| let error = link_intrinsic( | ||
| "other", | ||
| "__wasm_get_stack_pointer", | ||
| "(param i64) (result i32)", | ||
| false, | ||
| ) | ||
| .unwrap_err(); | ||
| let error = format!("{error:#}"); | ||
| assert!(error.contains("module requires an import interface named `other`")); | ||
| assert!(!error.contains("required linker ABI")); |
Member
There was a problem hiding this comment.
It's ok to be not quite so exhaustive in testing, these are pretty noisy tests and there's not necessarily a need to test every single possible observable behavior. Can these either be dropped or moved to tests/components/*?
Member
There was a problem hiding this comment.
This is already tested in wit-component, so it's ok to skip the test here as well
Comment on lines
+115
to
+117
| type_index: u32, | ||
| module: &str, | ||
| name: &str, |
Member
There was a problem hiding this comment.
One possible refactoring to simplify this slightly would be to take the wasmparser::Import here directly, and internally this would validate the type is a function to get the type index. That way not quite as many arguments need to be passed.
alexcrichton
approved these changes
Sep 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Invalid stack, TLS, and thread intrinsic signatures can reach final component validation before the linker reports an error.
Metadata extraction now checks five intrinsic signatures, enforces the
i32stack ABI, and reports required and actual types.Verification: 1,845 workspace tests passed. Six existing tests remain ignored. Clippy, formatting, and the component-only feature check passed.
Related to #2591.
Codex assisted with implementation, tests, and review.