fix(runtime): remove /dev/shm mmap file immediately after open#425
Merged
zoowii merged 3 commits intoDTVMStack:mainfrom Mar 27, 2026
Merged
fix(runtime): remove /dev/shm mmap file immediately after open#425zoowii merged 3 commits intoDTVMStack:mainfrom
zoowii merged 3 commits intoDTVMStack:mainfrom
Conversation
⚡ Performance Regression Check Results✅ Performance Check Passed (interpreter)Performance Benchmark Results (threshold: 25%)
Summary: 194 benchmarks, 0 regressions ✅ Performance Check Passed (multipass)Performance Benchmark Results (threshold: 25%)
Summary: 194 benchmarks, 0 regressions |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes x64 singlepass JIT memory addressing for large offsets (avoiding disp32 sign-extension / 32-bit overflow corner cases) and addresses mmap-backed linear memory temp-file cleanup, with new WAST regressions for the reported OOB false-positive/false-negative scenarios.
Changes:
- Add spec-extra WAST regressions for (1) large offset that should be in-bounds and (2) base+offset overflow that must trap.
- Update x64 singlepass codegen load/store addressing to avoid incorrect effective addresses when offsets exceed signed disp32 range.
- Unlink the mmap init file after opening to prevent
/dev/shmtemp-file accumulation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/wast/spec_extra/store_large_offset_false_positive_oob.wast | New regression covering large offset addressing that should remain in-bounds (no trap). |
| tests/wast/spec_extra/store_imm_base_overflow_false_negative_oob.wast | New regression covering base+offset overflow that must trap (detect missed OOB). |
| src/singlepass/x64/codegen.h | Adjusts load/store address construction to avoid disp32 sign-extension pitfalls for large offsets. |
| src/runtime/memory.cpp | Unlinks mmap init file after open and removes destructor-time forced removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/runtime/memory.cpp
Outdated
| Stats.revertRecord(Timer); | ||
| goto try_use_mmap_init; | ||
| } | ||
| ::remove(Path); |
There was a problem hiding this comment.
::remove(Path) is now the only cleanup for the temp mmap backing file (destructor removal was deleted). Since the return value is ignored, a failed unlink would reintroduce the /dev/shm file leak with no fallback cleanup. Please check the return value and either log+fallback to removing in the destructor (when it exists) or treat failure as disabling mmap-bucket for this module.
Suggested change
| ::remove(Path); | |
| if (::remove(Path) != 0) { | |
| ZEN_LOG_WARN("failed to remove mmap memory file '%s' (%s), " | |
| "disabling mmap-bucket for this module", | |
| Path, std::strerror(errno)); | |
| ::close(MmapFileFd); | |
| UseMmapBucket = false; | |
| Stats.revertRecord(Timer); | |
| goto try_use_mmap_init; | |
| } |
Comment on lines
+11
to
+13
| (module | ||
| (memory (;0;) 65131) | ||
|
|
There was a problem hiding this comment.
This test declares an initial memory of 65131 pages (~4.27 GiB). In builds where ZEN_ENABLE_CPU_EXCEPTION is off (so MemAllocOptions.UseMmap is forced false and allocations go through SysMemPool/malloc), this is likely to abort/OOM during spec test execution. Consider moving this test to a category that only runs with CPU-exception/mmap enabled (e.g. tests/wast/exception) or otherwise gating it so it doesn't require allocating multi-GiB linear memory in non-mmap configurations.
cmgCr
force-pushed
the
cmgCr/fix_memory
branch
2 times, most recently
from
8d7d752 to
74712bc
Compare
cmgCr
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
199
to
202
| } | ||
| if (MmapMemoryInitFd > 0) { | ||
| ::close(MmapMemoryInitFd); | ||
| // delete the memory file forcely | ||
| int Status = ::remove(MmapMemoryFilepath); | ||
| if (Status != 0) { | ||
| ZEN_LOG_WARN("failed to remove mmap tmp memory file %s due to '%s'", | ||
| MmapMemoryFilepath, std::strerror(errno)); | ||
| } | ||
| } |
There was a problem hiding this comment.
MmapMemoryInitFd uses 0 as the “invalid” sentinel, and the destructor (and checkWasmMemoryCanUseMmap()) only close/accept the fd when > 0. Since POSIX open() can legally return fd 0 (e.g., if stdin is closed), this can leak the fd and—now that the file is unlinked—prevent tmpfs storage from being reclaimed until process exit. Use -1 as the sentinel and treat >= 0 as valid in both the close and the checkWasmMemoryCanUseMmap() logic.
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
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.
1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):
2. What is the scope of this PR (e.g. component or file name):
3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):
4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):
5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:
6. Release note