Fix NVRTC include path option storage#1571
Conversation
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIn ChangesNVRTC include-path option storage refactor
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes a stack buffer overflow in
Confidence Score: 5/5The change is a well-scoped buffer-overflow fix with correct reserve math and proper pointer-stability management; safe to merge. The reserve capacity of max(num_cuda_include_dirs, 0) + 3 precisely accounts for all conditional stored_options push_backs (1 for the main include path, 1 for an optional pch-dir, 1 for an optional compile-time-trace, plus the loop), so no reallocation can occur after c_str() pointers are stored in opts. The removal of the incorrect max_path guard and the unsafe strcpy/strcat is clean. No unrelated code is touched. No files require special attention. Important Files Changed
Reviews (1): Last reviewed commit: "Fix NVRTC include path option storage" | Re-trigger Greptile |
|
Thanks for the PR. Before we consider this further, can you clarify what real circumstance led you to this change? Was this based on an actual Warp failure you encountered, or was it found by static analysis / code inspection? If it was an observed failure, please share the platform, install path layout, CUDA environment, reproduction steps, and validation showing that this change fixes it. For non-trivial changes, especially in CUDA runtime internals, we want contributions to be driven by demonstrated user issues, production failures, or clearly validated feature work. We generally do not want to establish a pattern of accepting speculative hardening PRs for hypothetical edge cases, even when the patch itself is small. If this is only a theoretical issue involving an unusually long Warp install path, we are unlikely to prioritize it. If there is a concrete real-world case behind it, please add those details so we can evaluate the impact. |
Description
wp_cuda_compile_program()built the main NVRTC include option in a fixed-size stack buffer. The guard checked onlyinclude_dir, but the buffer also had to hold the--include-path=prefix and the null terminator. That allowed paths that passed the check to overflow the stack buffer while constructing the option.This changes the main include option to use the same dynamic string storage pattern used for the other generated NVRTC options, while reserving capacity up front so the
c_str()pointers passed to NVRTC stay stable until compilation.Changes
--include-path=option instd::vector<std::string>instead of a fixed-sizecharbuffer.c_str()pointers.strcpy()/strcat()construction.Checklist
Unreleasedsection.Validation summary
I verified the focused source diff and formatting locally:
git diff --checkuvx pre-commit run --files warp/native/warp.cuuv run build_lib.py --quickThe quick build succeeded on macOS arm64 in CPU-only mode. This machine does not have a CUDA toolchain available, so I could not run an NVRTC/CUDA compile-path regression test locally.
Bug fix
The issue can be triggered when Warp is installed or checked out under a very long path and a CUDA kernel compile uses that path as
warp_home/nativefor the include directory. Without this change, the fixed stack buffer can overflow while prepending--include-path=.New feature / enhancement
Not applicable.
Summary by CodeRabbit
Release Notes