JIT WIP #384
JIT WIP #384
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The JIT implementation is comprehensive and includes a sophisticated x64 backend with label-based compilation, control flow analysis, and memory protection. However, there are critical security issues in the JIT compiler where certain memory access instructions (LoadInd and StoreImm) bypass memory protection checks. The concurrency model uses a sync-over-async bridge that is well-documented but has potential scalability limits.
Suggestions that couldn't be attached to a specific line
PolkaVM/Sources/CppHelper/x64_labeled_helper.cpp:1113-1168
Critical Security Issue: LoadInd instructions (LoadIndU8, LoadIndI8, etc.) are missing readMap checks. While they check bounds (< 65536 and >= memory_size), they do not verify if the page is readable in the readMap. This allows reading from non-readable pages (if any exist in the memory model). Compare with Load instructions (lines 1370-1444) which correctly check readMap.
PolkaVM/Sources/CppHelper/x64_labeled_helper.cpp:1507-1618
Critical Security Issue: StoreImm instructions (StoreImmU8, StoreImmU16, etc.) are missing writeMap checks. They check bounds but do not verify if the page is writable. This allows writing to read-only pages (e.g., the Read-Only zone). Compare with Store instructions (lines 1447-1504) which correctly check writeMap.
PolkaVM/Sources/PolkaVM/Executors/JIT/ExecutorBackendJIT.swift:422-478
The use of DispatchQueue.global() combined with semaphore.wait() (lines 737) creates a sync-over-async bridge. While this avoids blocking the Swift concurrency pool, it blocks a thread from the global dispatch queue for the duration of every host call. Under high load with many concurrent JIT executions performing host calls, this could exhaust the global queue's thread limit. Consider documenting this limitation or exploring coroutine-based yielding if possible in the future.
PolkaVM/Sources/PolkaVM/Executors/JIT/JITMemoryLayout.swift:21-150
The JITMemoryLayout class calculates packed offsets (baseOffset) for a contiguous memory layout, but JITExecutor.swift (lines 159-211) ignores these offsets and uses originalBase with mmap (sparse layout). This means the packing logic in JITMemoryLayout is effectively unused dead code. If the intention is to support both packed and sparse layouts, this should be made explicit; otherwise, JITMemoryLayout could be simplified to just extract zones without calculating packed offsets.
| case Opcode::LoadImmJump: | ||
| { | ||
| if (pc + 1 >= bytecode_size) return 0; // Need at least opcode + byte1 | ||
|
|
||
| uint8_t byte1 = bytecode[pc + 1]; | ||
| uint32_t l_X = (byte1 >> 4) & 0x07; // Length of immed_X (bits 4-6) | ||
|
|
||
| // l_Y is the remaining bytes after immed_X | ||
| // The test uses l_X=4, l_Y=1, so total size is 1+1+4+1=7 bytes | ||
| // We need to read the remaining bytes to determine actual l_Y | ||
| // For now, assume l_Y can be 0-4 bytes based on what's available | ||
| uint32_t max_l_Y = 4; | ||
| uint32_t available = bytecode_size - (pc + 2 + l_X); | ||
| uint32_t l_Y = (max_l_Y < available) ? max_l_Y : available; | ||
|
|
||
| instrSize = 2 + l_X + l_Y; // opcode + byte1 + immed_X + immed_Y | ||
|
|
||
| if (instrSize > 20) return 0; // Sanity check | ||
| } |
There was a problem hiding this comment.
The LoadImmJump size calculation logic relies on available bytes to determine l_Y if skipTable is not used. This heuristic (max_l_Y = 4) might be incorrect if LoadImmJump is followed by other instructions that are mistakenly consumed as immediate data. While JITCompiler.swift ensures skipTable is provided, this fallback logic in C++ seems fragile.
| if let nodeData = try? await backend.read(key: id), nodeData.count >= 65 { | ||
| // Reconstruct a temporary hash for parsing (TrieNode doesn't validate hash) | ||
| // The actual hash value doesn't matter here since we only need node.type and children | ||
| // Safety: id is guaranteed to be 31 bytes (from deleted set keys which are suffix(31)) | ||
| // Prefixing with 1 zero byte gives us exactly 32 bytes for Data32 | ||
| var hashBytes = Data(repeating: 0, count: 1) | ||
| hashBytes.append(id) | ||
| let node = TrieNode(hash: Data32(hashBytes)!, data: nodeData) | ||
| if node.isBranch { | ||
| let leftKey = node.left.data.suffix(31) | ||
| let rightKey = node.right.data.suffix(31) | ||
| refChanges[leftKey, default: 0] -= 1 | ||
| refChanges[rightKey, default: 0] -= 1 | ||
| } |
There was a problem hiding this comment.
The reconstruction of a temporary TrieNode from raw backend data just to access its children for reference counting is a bit heavy. Ensure that nodeData is always guaranteed to be in a format that TrieNode can parse, especially since TrieNode constructor doesn't seem to validate the hash against the data.
- Update .swift-version to 6.2.3. - Replace absolute include paths with relative ones in Package.swift to improve portability. - Add `asmjit` symlink to `Sources/asmjit`. - Remove unused include in `helper.cpp`. - Add `plans/` to .gitignore.
Implement cross-platform system call handling in `ChildProcessManager` and IPC components using `canImport(Glibc)` and `canImport(Darwin)` for macOS compatibility. Update Swift code to utilize modern syntax, including `if let ... = ... else { ... }` expressions and `some Codable` for improved readability and conciseness.
Adjust `asmjit` include paths in `Blockchain/Package.swift` to be relative. Temporarily disable unstable JIT test suites and refine JIT logging for better debugging. Remove `ulimit` command from the `Makefile`'s test target. Add new VSCode launch configurations for the `boka-sandbox`.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #384 +/- ##
==========================================
- Coverage 72.13% 64.40% -7.73%
==========================================
Files 410 438 +28
Lines 31821 36678 +4857
==========================================
+ Hits 22955 23624 +669
- Misses 8866 13054 +4188 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
No description provided.