fix(moduleloader): isolate module namespaces and cache loaded modules#1132
fix(moduleloader): isolate module namespaces and cache loaded modules#1132Marshall-Hallenbeck wants to merge 1 commit intomainfrom
Conversation
The module loader used the same module name "NXCModule" for every file via spec_from_file_location(), causing sys.modules namespace collisions when multiple modules are loaded together (-M mod1 -M mod2). Each subsequent module overwrites the NXCModule class in the shared namespace, so modules loaded earlier resolve a different class than their own. This is the underlying mechanism behind multi-module ordering bugs reported in #879, #880, and #882. Changes: - Use unique module names (nxc_module_<filename>) and the modern module_from_spec() + exec_module() API instead of the deprecated load_module() to give each module its own isolated namespace - Cache loaded modules so each file is only parsed and executed once, regardless of how many targets are scanned (~60x faster than re-loading per target) - Extract shared loading logic into load_module_file() classmethod Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes multi-module ordering bugs by ensuring each loaded module file gets an isolated import namespace (avoiding sys.modules["NXCModule"] collisions) and by caching loaded module files to avoid repeated parsing/execution across many targets.
Changes:
- Introduces a class-level module cache and a shared
load_module_file()loader usingmodule_from_spec()+exec_module(). - Updates
load_module()andget_module_info()to use the shared loader (and benefit from isolation + caching).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if module_path not in cls.module_cache: | ||
| module_name = f"nxc_module_{basename(module_path)[:-3]}" | ||
| spec = importlib.util.spec_from_file_location(module_name, module_path) | ||
| module = importlib.util.module_from_spec(spec) | ||
| spec.loader.exec_module(module) | ||
| cls.module_cache[module_path] = module | ||
| return cls.module_cache[module_path] |
There was a problem hiding this comment.
module_cache is accessed from multiple worker threads (see ThreadPoolExecutor(max_workers=args.threads) in nxc/netexec.py). The current if module_path not in cls.module_cache: ... cls.module_cache[module_path] = module sequence is not synchronized, so the same module can still be executed multiple times concurrently, defeating the “executed once” guarantee and potentially causing side effects in module top-level code. Consider guarding cache population with a class-level lock (or using a threadsafe memoization pattern) so only one thread loads/executes a given module_path.
| Each module gets a unique name to avoid sys.modules collisions that | ||
| caused NXCModule class references to be overwritten when loading | ||
| multiple modules. Results are cached so each file is only parsed | ||
| and executed once regardless of how many targets are scanned. | ||
| """ | ||
| if module_path not in cls.module_cache: | ||
| module_name = f"nxc_module_{basename(module_path)[:-3]}" | ||
| spec = importlib.util.spec_from_file_location(module_name, module_path) |
There was a problem hiding this comment.
module_name is derived only from basename(module_path), but the docstring claims each module file gets a unique name. If two different module paths share the same filename (e.g., built-in vs user modules), they’ll get the same module_name, which can make debugging confusing and can reintroduce collisions if the loader (or module code) ever relies on sys.modules. Consider incorporating a stable unique component from the full path (e.g., a short hash) or using Path(module_path).resolve() to build the name.
| """ | ||
| if module_path not in cls.module_cache: | ||
| module_name = f"nxc_module_{basename(module_path)[:-3]}" | ||
| spec = importlib.util.spec_from_file_location(module_name, module_path) |
There was a problem hiding this comment.
spec_from_file_location() may return None (or a spec with loader is None) for an invalid/unreadable path. As written, module_from_spec(spec) / spec.loader.exec_module(module) will raise less-informative exceptions (e.g., attribute errors) which then get logged as a generic load failure. Consider explicitly validating spec and spec.loader and raising a clearer error message when they’re missing.
| spec = importlib.util.spec_from_file_location(module_name, module_path) | |
| spec = importlib.util.spec_from_file_location(module_name, module_path) | |
| if spec is None: | |
| raise ImportError(f"Cannot load module from {module_path}: invalid module spec (spec is None)") | |
| if spec.loader is None: | |
| raise ImportError(f"Cannot load module from {module_path}: missing loader in module spec") |
Description
The module loader uses
spec_from_file_location("NXCModule", module_path)with the same module name"NXCModule"for every module file. When multiple modules are loaded via-M, each subsequent module overwrites theNXCModuleclass in the sharedsys.modules["NXCModule"]namespace. This means modules loaded earlier resolve a different module'sNXCModuleclass when referencing it — causing attribute errors, wrong method calls, or other subtle breakage depending on which module loads last.This is the underlying mechanism behind the multi-module ordering bugs reported in #879, #880, and #882 — all of which describe symptoms where the order of
-Mflags changes behavior or causes failures.Changes:
Isolated namespaces — Each module file now gets a unique name (
nxc_module_<filename>) and is loaded via the modernmodule_from_spec()+exec_module()API instead of the deprecatedload_module(). This gives each module its own namespace — no more cross-module pollution.Module caching — Loaded modules are cached in a class-level dict so each file is only parsed and executed once, regardless of how many targets are scanned. This is ~60x faster than the old code for multi-target scans:
load_modulewithsys.modules)exec_module)DRY — Extracted shared loading logic into
load_module_file()classmethod, used by bothload_module()andget_module_info().AI disclosure: Claude Code (Claude Opus 4.6) was used to assist with root cause analysis, benchmarking, and drafting the fix. The bug was discovered during a real pentest scan, root cause was traced and verified by human and AI together, and the fix was human-reviewed and tested on live targets.
Type of change
Setup guide for the review
How to demonstrate the namespace collision (without this fix):
Tested on:
Screenshots (if appropriate):
N/A — this is an internal loader change with no user-visible output difference (other than fixing the errors).
Checklist:
poetry run ruff check ., use--fixto automatically fix what it can)tests/e2e_commands.txtfile if necessary (new modules or features are required to be added to the e2e tests)