fix: prevent module point to wild pointer#13021
Conversation
Rsdoctor Bundle Diff AnalysisFound 5 projects in monorepo, 0 projects with changes. 📊 Quick Summary
Generated by Rsdoctor GitHub Action |
📦 Binary Size-limit
❌ Size increased by 25.50KB from 48.65MB to 48.68MB (⬆️0.05%) |
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a critical memory-safety issue in Rspack’s Rust↔JS module interop by ensuring module heap addresses remain stable during build, preventing JS from holding dangling pointers to moved/freed Rust module memory.
Changes:
- Change
Module::buildto consumeself: Box<Self>and require returning the (boxed) module viaBuildResult { module, ... }. - Refactor
NormalModule(and loader runner context) to avoidmem::replace-style moves during loader execution. - Update core/plugins to the new build contract, including module concatenation and module-graph build pipeline.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/rspack_core/src/module.rs | Changes Module::build signature; makes BuildResult carry the built BoxModule; adds BoxModule::build helper. |
| crates/rspack_core/src/normal_module.rs | Removes move-based ownership transfer; runs loaders while keeping module allocation stable; returns module via BuildResult. |
| crates/rspack_core/src/loader/loader_runner.rs | Stores RunnerContext.module as Box<NormalModule> to keep allocation stable during loader execution. |
| crates/rspack_binding_api/src/plugins/js_loader/context.rs | Adjusts JS loader context module pointer creation for the boxed module storage. |
| crates/rspack_core/src/compilation/build_module_graph/graph_updater/repair/build.rs | Updates build task flow to retrieve the built module from BuildResult.module. |
| crates/rspack_core/src/concatenated_module.rs | Updates ConcatenatedModule::build to the new boxed build contract and returns itself in BuildResult. |
| crates/rspack_plugin_javascript/src/plugin/module_concatenation_plugin.rs | Updates concatenation flow to build via BoxModule::build and use identifier() with BoxModule. |
| crates/rspack_core/src/context_module.rs | Adapts ContextModule::build to boxed self + BuildResult.module. |
| crates/rspack_core/src/external_module.rs | Adapts ExternalModule::build to boxed self + BuildResult.module. |
| crates/rspack_core/src/raw_module.rs | Adds build implementation returning itself in BuildResult under the new trait contract. |
| crates/rspack_core/src/self_module.rs | Adds build implementation returning itself in BuildResult under the new trait contract. |
| crates/rspack_core/src/cache/persistent/occasion/make/alternatives/module.rs | Adds build implementation for TempModule to satisfy the new trait contract. |
| crates/rspack_macros/src/runtime_module.rs | Updates runtime-module macro output to implement boxed build returning BuildResult.module. |
| crates/rspack_plugin_extract_css/src/css_module.rs | Updates CssModule::build to return itself in BuildResult. |
| crates/rspack_plugin_lazy_compilation/src/module.rs | Updates lazy compilation proxy module build to boxed self + BuildResult.module. |
| crates/rspack_plugin_dll/src/dll_entry/dll_module.rs | Updates DllModule::build to boxed self + BuildResult.module. |
| crates/rspack_plugin_dll/src/dll_reference/delegated_module.rs | Updates DelegatedModule::build to boxed self + BuildResult.module. |
| crates/rspack_plugin_mf/src/container/container_entry_module.rs | Updates MF container entry module build to boxed self + BuildResult.module. |
| crates/rspack_plugin_mf/src/container/fallback_module.rs | Updates MF fallback module build to boxed self + BuildResult.module. |
| crates/rspack_plugin_mf/src/container/remote_module.rs | Updates MF remote module build to boxed self + BuildResult.module. |
| crates/rspack_plugin_mf/src/sharing/consume_shared_module.rs | Updates MF consume-shared module build to boxed self + BuildResult.module. |
| crates/rspack_plugin_mf/src/sharing/provide_shared_module.rs | Updates MF provide-shared module build to boxed self + BuildResult.module. |
Comments suppressed due to low confidence (1)
crates/rspack_core/src/module.rs:240
BuildResult's doc comment ondependenciessays "Whether the result is cacheable" which doesn't match the field type/meaning (it's a list of dependencies). This looks misleading after the refactor; consider updating/removing the comment or moving it to the actual cacheability field (if any).
pub module: BoxModule,
/// Whether the result is cacheable, i.e shared between builds.
pub dependencies: Vec<BoxDependency>,
pub blocks: Vec<Box<AsyncDependenciesBlock>>,
pub optimization_bailouts: Vec<String>,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Fix: #12899
This PR fixes a critical memory safety issue where JavaScript modules could access freed Rust module memory through dangling pointers.
Problem
Currently in Rspack, JavaScript modules access Rust modules via raw pointers, which requires that Rust module addresses remain stable throughout their lifetime.
However, the existing code uses
std::mem::replacein theNormalModule::buildmethod, which violates this assumption by moving the module to a different memory location. This creates a race condition where:std::mem::replaceSolution
This PR resolves the issue by changing the
Module::buildmethod signature from:to:
The method now takes ownership of the boxed module and returns it within
BuildResult, ensuring the module's heap allocation remains stable and its address doesn't change.Next Steps
This PR lays the groundwork for a more comprehensive solution. In a follow-up PR, I plan to refactor
BoxModuleto store it within aBindingCell, which will provide stronger guarantees against dangling pointer issues at the type system level.Related links
Checklist