Add function OS::get_real_path()#1272
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds OS::get_real_path API with a default passthrough; implements Unix (realpath-based) and Windows (CreateFileW + GetFinalPathNameByHandleW, prefix stripping, separator normalization) resolvers; updates multiple get_executable_path() branches to use the resolver and adjusts Unix compile guards. ChangesReal path resolution (cross-platform)
Sequence Diagram(s)sequenceDiagram
participant Caller
participant OS
participant OS_Windows
participant WinAPI
Caller->>OS: get_executable_path()
OS->>OS_Windows: get_executable_path()
OS_Windows->>WinAPI: GetModuleFileNameW() -> module path
OS_Windows->>OS_Windows: get_real_path(module path)
OS_Windows->>WinAPI: CreateFileW(module path) / GetFinalPathNameByHandleW(handle)
WinAPI-->>OS_Windows: final path (may include \\?\ or \\?\UNC\ prefix)
OS_Windows-->>OS: resolved executable path (prefixes stripped, separators normalized)
OS-->>Caller: executable path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)platform/windows/os_windows.cppIn file included from platform/windows/os_windows.cpp:33: ... [truncated 1189 characters] ... all/lib/clang/18/include" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform/windows/os_windows.cpp`:
- Line 2055: The line declaring the local variable "String result;" is indented
with spaces instead of the repository's tab style; run clang-format (or reformat
that block) so the declaration uses tabs to match surrounding code and coding
style, ensuring the "String result" line indentation matches neighboring lines
in os_windows.cpp.
- Around line 2054-2078: The get_real_path function currently returns an empty
string when CreateFileW or GetFinalPathNameByHandleW fails; initialize result
from p_path at the start of OS_Windows::get_real_path so the original path is
preserved, then only overwrite result after a successful
GetFinalPathNameByHandleW call; ensure the existing logic that strips
"\\\\?\\UNC\\" and "\\\\?\\" is only applied when resolution succeeded, and
leave result (used by get_executable_path) untouched on any failure of
CreateFileW or GetFinalPathNameByHandleW.
- Around line 2062-2065: The prefix checks in OS_Windows::get_real_path are
using negation on result.substr(...) (e.g. !result.substr(0, 8) == ...) which is
wrong; replace those comparisons with proper prefix checks such as
result.begins_with("\\\\?\\UNC\\") and result.begins_with("\\\\?\\") (or use !=
for direct string compare), then perform the same stripping logic (keep the
existing result = String("\\") + result.substr(7) and result = result.substr(4)
statements) so the extended-path prefixes are detected correctly and the
UNC/\\?\ stripping runs only when the prefix is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dcc9583e-e7e6-44ee-801b-35679c86675b
📒 Files selected for processing (1)
platform/windows/os_windows.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (2)
drivers/unix/os_unix.cpp (2)
1141-1141: 💤 Low valueRemove redundant cast.
The explicit cast to
(const char *)is unnecessary becausep_path.utf8().get_data()already returnsconst char*.♻️ Simplified version
- if (realpath((const char *)p_path.utf8().get_data(), resolved_path)) { + if (realpath(p_path.utf8().get_data(), resolved_path)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drivers/unix/os_unix.cpp` at line 1141, The call to realpath uses a redundant C-style cast—remove the unnecessary cast so it calls realpath(p_path.utf8().get_data(), resolved_path) directly; update the call site in the code that references realpath, p_path.utf8().get_data(), and resolved_path to pass the already-const char* return value without casting.
1138-1145: 💤 Low valueConsider documenting the behavior for non-existent paths.
The function returns an empty string when
realpath()fails, which includes cases where the path doesn't exist. This silent failure might be intentional, but consider whether callers will be able to distinguish between an error and an empty result.This is consistent with other error-handling patterns in this file (e.g.,
get_environmentreturning empty string on missing vars), but a brief comment documenting the empty-string-on-failure behavior would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drivers/unix/os_unix.cpp` around lines 1138 - 1145, Document that OS_Unix::get_real_path returns an empty String when realpath() fails (e.g., non-existent path or other errors) so callers cannot distinguish error vs empty result; add a brief inline comment above or inside the get_real_path implementation stating this empty-string-on-failure behavior and that callers must treat an empty return as a failure to resolve the path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@drivers/unix/os_unix.cpp`:
- Line 1141: The call to realpath uses a redundant C-style cast—remove the
unnecessary cast so it calls realpath(p_path.utf8().get_data(), resolved_path)
directly; update the call site in the code that references realpath,
p_path.utf8().get_data(), and resolved_path to pass the already-const char*
return value without casting.
- Around line 1138-1145: Document that OS_Unix::get_real_path returns an empty
String when realpath() fails (e.g., non-existent path or other errors) so
callers cannot distinguish error vs empty result; add a brief inline comment
above or inside the get_real_path implementation stating this
empty-string-on-failure behavior and that callers must treat an empty return as
a failure to resolve the path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b7e8696-7d49-4846-add4-7ce448bf8401
📒 Files selected for processing (2)
drivers/unix/os_unix.cppdrivers/unix/os_unix.h
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/os/os.h`:
- Line 199: The OS base class implementation has a signature mismatch: the
header declares virtual String get_real_path(const String &p_path) const while
the cpp defines String OS::get_real_path() const and returns the unused member
_realpath; fix by updating the implementation to match the header (define String
OS::get_real_path(const String &p_path) const) and have it return p_path (i.e.,
return the input argument) as the default behavior, and remove or delete the
unused member _realpath (or at minimum mark it unused) so there is no
dead/unused state.
- Line 54: The field _realpath and the no-arg OS::get_real_path() const accessor
are unused; either remove them or ensure _realpath is populated whenever a
real-path is resolved. Fix option A: remove the member _realpath and the no-arg
OS::get_real_path() const to avoid dead state and rely only on
OS::get_real_path(const String &p_path). Fix option B: populate _realpath inside
the real-path resolver(s) — in OS::get_real_path(const String &p_path)
implementations (and any platform overrides) assign the resolved path to
_realpath before returning so the no-arg accessor returns the last-resolved
path; update constructors if needed to initialize _realpath. Use the symbols
_realpath, OS::get_real_path() const, and OS::get_real_path(const String
&p_path) to locate and apply the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89f033b5-8aa8-47ff-b342-ade336846137
📒 Files selected for processing (2)
core/os/os.cppcore/os/os.h
✅ Files skipped from review due to trivial changes (1)
- core/os/os.cpp
|
@coderabbitai resume |
|
@coderabbitai review |
✅ Actions performedReviews resumed. |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/unix/os_unix.cpp (1)
1147-1154:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCompile error:
return b;references an undefined variable.The resolved path is stored in
s(Line 1149), but the success path returnsb, which is never declared. This will not compile on__linux__builds.🐛 Proposed fix
String s = get_real_path("/proc/self/exe"); if (!FileAccess::exists(s)) { WARN_PRINT("Couldn't get executable path from /proc/self/exe, using argv[0]"); return OS::get_executable_path(); } - return b; + return s;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drivers/unix/os_unix.cpp` around lines 1147 - 1154, In OS_Unix::get_executable_path() replace the incorrect undefined return value by returning the resolved path variable `s` (the result of get_real_path("/proc/self/exe")) instead of `b`; keep the existing check using FileAccess::exists(s) and the fallback call to OS::get_executable_path() when the proc path is not available so the function returns `s` on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@drivers/unix/os_unix.cpp`:
- Around line 1147-1154: In OS_Unix::get_executable_path() replace the incorrect
undefined return value by returning the resolved path variable `s` (the result
of get_real_path("/proc/self/exe")) instead of `b`; keep the existing check
using FileAccess::exists(s) and the fallback call to OS::get_executable_path()
when the proc path is not available so the function returns `s` on success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9146983-ec6b-4bc6-b5ec-68fe87ffb2db
📒 Files selected for processing (4)
core/os/os.cppcore/os/os.hdrivers/unix/os_unix.cppplatform/windows/os_windows.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- core/os/os.cpp
- platform/windows/os_windows.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/unix/os_unix.cpp (1)
1138-1145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix portability: avoid unguarded
PATH_MAXinOS_Unix::get_real_path()
drivers/unix/os_unix.cppincludes<climits>only under__OpenBSD__, butOS_Unix::get_real_path()unconditionally declareschar resolved_path[PATH_MAX];, making non-OpenBSD builds depend on transitivePATH_MAXavailability. Switch to the POSIX.1-2008realpath(..., nullptr)form (and free the returned buffer). The current success-only assignment + fallback top_pathis fine.🛠️ Proposed fix using dynamic allocation
String OS_Unix::get_real_path(const String &p_path) const { String result = p_path; - char resolved_path[PATH_MAX]; - if (realpath(p_path.utf8().get_data(), resolved_path)) { - result = String::utf8(resolved_path); - } + char *resolved_path = realpath(p_path.utf8().get_data(), nullptr); + if (resolved_path) { + result = String::utf8(resolved_path); + free(resolved_path); + } return result; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drivers/unix/os_unix.cpp` around lines 1138 - 1145, OS_Unix::get_real_path currently declares a stack buffer char resolved_path[PATH_MAX] which is non-portable; change it to call realpath(p_path.utf8().get_data(), nullptr), check the returned char * (e.g., char *rp = realpath(...)), on success set result = String::utf8(rp) and then free(rp), otherwise leave result as p_path; ensure you include <cstdlib> for free and handle nullptr safely so behavior and fallback remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@drivers/unix/os_unix.cpp`:
- Around line 1138-1145: OS_Unix::get_real_path currently declares a stack
buffer char resolved_path[PATH_MAX] which is non-portable; change it to call
realpath(p_path.utf8().get_data(), nullptr), check the returned char * (e.g.,
char *rp = realpath(...)), on success set result = String::utf8(rp) and then
free(rp), otherwise leave result as p_path; ensure you include <cstdlib> for
free and handle nullptr safely so behavior and fallback remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32d6f6ab-ba97-431b-b18c-cf829bff8e9d
📒 Files selected for processing (1)
drivers/unix/os_unix.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/unix/os_unix.cpp (1)
1149-1153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDistinguish
realpath()failure from a successful Linux resolution.Line 1150 only checks whether the returned path exists, but
get_real_path()falls back to the original input on failure. That means this branch can still return"/proc/self/exe"when normalization failed, and downstream callers will then derive the executable directory from/proc/selfinstead of the actual binary location.Suggested fix
- String s = get_real_path("/proc/self/exe"); - if (!FileAccess::exists(s)) { + static const String self_exe = "/proc/self/exe"; + String s = get_real_path(self_exe); + if (s == self_exe || !FileAccess::exists(s)) { WARN_PRINT("Couldn't get executable path from /proc/self/exe, using argv[0]"); return OS::get_executable_path(); } return s;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drivers/unix/os_unix.cpp` around lines 1149 - 1153, get_real_path("/proc/self/exe") can return the original input on failure, so change the branch in the block using get_real_path, variable s, FileAccess::exists and OS::get_executable_path to detect a failed normalization: after calling get_real_path("/proc/self/exe"), treat it as a failure if s is empty or if s == "/proc/self/exe" (meaning realpath didn't resolve) and fall back to OS::get_executable_path(); only when s differs from "/proc/self/exe" and FileAccess::exists(s) is true should you accept s as the resolved executable path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@drivers/unix/os_unix.cpp`:
- Around line 1149-1153: get_real_path("/proc/self/exe") can return the original
input on failure, so change the branch in the block using get_real_path,
variable s, FileAccess::exists and OS::get_executable_path to detect a failed
normalization: after calling get_real_path("/proc/self/exe"), treat it as a
failure if s is empty or if s == "/proc/self/exe" (meaning realpath didn't
resolve) and fall back to OS::get_executable_path(); only when s differs from
"/proc/self/exe" and FileAccess::exists(s) is true should you accept s as the
resolved executable path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 272435ba-cacf-4bbe-8b89-3131bc91537b
📒 Files selected for processing (1)
drivers/unix/os_unix.cpp
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform/windows/os_windows.cpp`:
- Around line 67-70: Reorder the `#include` lines to match clang-format's expected
ordering (or run clang-format and commit its changes): ensure the headers
<cstdlib>, <cstring>, <cwchar>, <string> are sorted/grouped according to the
project's style (alphabetical/standard-library grouping) and then commit the
formatted file so the pipeline passes; you can also run the repo's clang-format
hook locally to apply and verify the exact ordering.
- Around line 125-145: _wrealpath currently always returns resolved_path even
when resolution fails, causing get_real_path to see a non-null but empty string;
change _wrealpath (and its use of CreateFileW/GetFinalPathNameByHandleW) to
detect failure conditions and return NULL instead of writing an empty string
into resolved_path: only copy into resolved_path when GetFinalPathNameByHandleW
succeeds and produces a non-empty result, and handle the case where returned len
>= MAX_PATH by either allocating a sufficiently large buffer and retrying or
returning NULL to signal failure; ensure callers like get_real_path rely on NULL
to perform their fallback behavior rather than receiving an empty string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24976f62-ac19-4940-b3cc-20a50f88270c
📒 Files selected for processing (1)
platform/windows/os_windows.cpp
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
platform/windows/os_windows.cpp (1)
132-134:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck for buffer overflow condition from
GetFinalPathNameByHandleW.When the resolved path exceeds
MAX_PATH,GetFinalPathNameByHandleWreturns the required buffer size but does not write valid data to the buffer. The currentif (len)check passes in this case, causingresult = ptrto read undefined buffer content.🐛 Proposed fix
DWORD len = GetFinalPathNameByHandleW(hFile, ptr, MAX_PATH, FILE_NAME_NORMALIZED | VOLUME_NAME_DOS); - if (len) { + if (len && len < MAX_PATH) { result = ptr;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform/windows/os_windows.cpp` around lines 132 - 134, GetFinalPathNameByHandleW may return a required buffer size > MAX_PATH without writing into ptr; current code only checks (len) and then uses ptr, risking reading invalid data. Change the logic around the GetFinalPathNameByHandleW call (using hFile, ptr, MAX_PATH) to verify that len is nonzero and less than or equal to MAX_PATH before assigning result = ptr; if len > MAX_PATH, allocate or resize a buffer of size len (or call again with a sufficiently large buffer) and call GetFinalPathNameByHandleW again to obtain the full path, then assign result from that valid buffer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@platform/windows/os_windows.cpp`:
- Around line 132-134: GetFinalPathNameByHandleW may return a required buffer
size > MAX_PATH without writing into ptr; current code only checks (len) and
then uses ptr, risking reading invalid data. Change the logic around the
GetFinalPathNameByHandleW call (using hFile, ptr, MAX_PATH) to verify that len
is nonzero and less than or equal to MAX_PATH before assigning result = ptr; if
len > MAX_PATH, allocate or resize a buffer of size len (or call again with a
sufficiently large buffer) and call GetFinalPathNameByHandleW again to obtain
the full path, then assign result from that valid buffer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50d92fd7-b6b4-4e09-a271-23e95501618b
📒 Files selected for processing (2)
drivers/unix/os_unix.cppplatform/windows/os_windows.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- drivers/unix/os_unix.cpp
1 participant
Summary by CodeRabbit
Refactor
Bug Fixes