Fix/linker crash tls symbol without tls base #6008
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a potential deadlock in the Linker and addresses a crash that occurs when resolving TLS symbols from modules that don't export their __tls_base. The deadlock fix involves moving the is_closure check earlier in the call chain to avoid holding multiple locks simultaneously.
- Refactored
is_closureto be called before acquiring exclusive store access, preventing potential deadlocks - Added proper error handling for TLS symbols without TLS base exports
- Updated error messages for better clarity when TLS resolution fails
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/wasix/src/syscalls/wasix/reflect_signature.rs | Moves the is_closure check to execute before data_and_store_mut() to avoid deadlock; reuses the computed cacheable value in error handling |
| lib/wasix/src/state/linker.rs | Adds Display impl for ModuleHandle; adds MissingTlsBaseExport error variant; updates is_closure signature to require FunctionEnvMut parameter; improves TLS error handling with better error propagation |
| lib/wasix/src/state/handles/mod.rs | Updates is_closure to be a static method taking FunctionEnvMut parameter, consistent with the linker API changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Check if an indirect_function_table entry is reserved for closures. | ||
| /// | ||
| /// Returns false if the entry is not reserved for closures. | ||
| pub fn is_closure(&self, function_id: u32) -> bool { | ||
| // TODO: Check if this can result in a deadlock | ||
| let linker = self.linker_state.read().unwrap(); | ||
| linker | ||
| // TODO: we can cache this information within the group state so we don't | ||
| // need a write lock on the linker state here | ||
| pub fn is_closure( | ||
| &self, | ||
| function_id: u32, | ||
| ctx: &mut FunctionEnvMut<'_, WasiEnv>, | ||
| ) -> Result<bool, LinkError> { |
There was a problem hiding this comment.
The documentation for this method should be updated to explain why it now requires a FunctionEnvMut parameter. This is a breaking API change and callers need to understand that this method may perform pending DL operations as a side effect when acquiring the linker state lock.
| /// Check if an indirect_function_table entry is reserved for closures. | ||
| /// | ||
| /// Returns false if the entry is not reserved for closures. | ||
| pub fn is_closure(&self, function_id: u32) -> bool { | ||
| match self { | ||
| WasiModuleTreeHandles::Static(_) => false, | ||
| WasiModuleTreeHandles::Dynamic { linker, .. } => linker.is_closure(function_id), | ||
| pub fn is_closure( | ||
| ctx: &mut FunctionEnvMut<'_, WasiEnv>, | ||
| function_id: u32, | ||
| ) -> Result<bool, LinkError> { |
There was a problem hiding this comment.
The documentation for this method should be updated to reflect the new signature. It should explain that the method is now a static method taking a FunctionEnvMut parameter, and why this change was necessary (to avoid potential deadlocks when accessing the linker state).
| match ctx.data().inner() { | ||
| WasiModuleTreeHandles::Static(_) => Ok(false), | ||
| WasiModuleTreeHandles::Dynamic { linker, .. } => { | ||
| linker.clone().is_closure(function_id, ctx) |
There was a problem hiding this comment.
The clone here is unnecessary since is_closure takes &self rather than self. The call can be simplified to linker.is_closure(function_id, ctx) without the clone.
| linker.clone().is_closure(function_id, ctx) | |
| linker.is_closure(function_id, ctx) |
| )); | ||
| } | ||
| Err(e) => { | ||
| Err(e).expect("Internal error: bad in-progress symbol resolution") |
There was a problem hiding this comment.
The pattern Err(e).expect(...) will always panic unconditionally. If this is intentional, it would be clearer to use panic!("Internal error: bad in-progress symbol resolution: {:?}", e) instead. This makes the intent more explicit and includes the error details in the panic message.
| Err(e).expect("Internal error: bad in-progress symbol resolution") | |
| panic!("Internal error: bad in-progress symbol resolution: {:?}", e) |
This PR also contains a fix to a potential deadlock in the Linker. Mostly the same implementation as #5879 otherwise.