Add More Reliable OpenBSD Executable Path Solution#1243
Open
samuelvenable wants to merge 10 commits intoRedot-Engine:masterfrom
Open
Add More Reliable OpenBSD Executable Path Solution#1243samuelvenable wants to merge 10 commits intoRedot-Engine:masterfrom
samuelvenable wants to merge 10 commits intoRedot-Engine:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughOpenBSD's OS_Unix::get_executable_path() now uses kvm to read the process argv and file mappings, constructs and validates argv-, PATH-, PWD-, and Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as OS_Unix::get_executable_path()
participant KVM as kvm (kvm_open/getprocs/getargv/getfiles)
participant FS as Filesystem (stat, access, realpath)
participant Env as Environment (PATH, PWD, `_`)
Caller->>KVM: open kvm, read process argv & file map
KVM-->>Caller: argv[] and file entries
loop for each argv candidate
Caller->>FS: is_exe(candidate) -> stat/access/realpath
FS-->>Caller: metadata & resolved path
Caller->>KVM: verify candidate matches kvm text entry
KVM-->>Caller: match? yes/no
alt match & executable
Caller->>Caller: return resolved path
end
end
alt none matched
Caller->>Env: iterate PATH (+ defaults, $HOME/bin)
Env-->>Caller: PATH candidates
Caller->>FS: is_exe checks
FS-->>Caller: results
alt found
Caller->>Caller: return resolved path
else
Caller->>Env: try PWD/getcwd then `_`
Env-->>Caller: candidates
Caller->>FS: is_exe checks
FS-->>Caller: results
Caller->>Caller: return resolved path or fallback to realpath(OS::get_executable_path())
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drivers/unix/os_unix.cpp`:
- Around line 1199-1203: When kvm_openfiles or kvm_getargv fail (and whenever
all candidate realpath checks leave `path` empty) do not return an empty String;
instead WARN_PRINT a diagnostic and fall back to returning
OS::get_executable_path() (mirroring the FreeBSD/NetBSD branches). Update the
failure branch after kvm_openfiles (and the similar branch around the
kvm_getargv/candidate loop) to call WARN_PRINT(...) with context and then return
OS::get_executable_path() instead of returning an empty String, and ensure the
same fallback is applied at the other affected region (the branch covering the
candidate verification loop).
- Around line 1154-1277: This block uses STL types and goto-based flow; refactor
to Godot conventions by replacing std::string/ std::vector/ std::stringstream
with String/CharString/Vector<String> and splitting PATH with String::split;
extract the is_exe lambda into a static/file-local helper (e.g.
_openbsd_verify_exe) that encapsulates kvm_openfiles/kvm_getfiles/kvm_close and
returns a String, replace cppstr_getenv with a thin helper that returns String,
and eliminate goto retry/fallback by implementing a two-pass PATH search (env
PATH then built-in PATH) via a small helper (e.g. _openbsd_search_path) and an
explicit second-attempt for the "_" argv0 fallback instead of goto; update
usages in the current function to call these helpers and iterate Vector<String>
results.
- Around line 1159-1215: The OpenBSD-specific block around the
kvm_openfiles/kvm_getfiles usage (search for symbols kvm_openfiles,
kvm_getfiles, and the fallback: label / KERN_FILE_TEXT) is misformatted; run
your formatter (pre-commit run clang-format --files or clang-format -i) on the
changed file to fix clang-format violations and then stage/commit the
reformatted file so the CI pre-commit clang-format check passes.
- Around line 1274-1277: The return path conversion currently uses
String(path.c_str()) which calls the non-UTF-8 overload and will mangle
non-ASCII components; replace that with String::utf8(path) (matching the FreeBSD
branch's use of String::utf8(buf)) to preserve UTF-8 path data, and remove the
unnecessary errno = 0 assignment on the successful path so callers' errno isn't
altered; update the return site that constructs the String from the local
variable path accordingly.
- Around line 1169-1171: Replace the manual file-type test that uses (st.st_mode
& S_IFREG) with the POSIX macro S_ISREG(st.st_mode) in the conditional that
compares exe file attributes (the if that checks stat(exe.c_str(), &st),
executable bit, regular file, realpath and device/inode against
kif[i].va_fsid/va_fileid); update that expression to use S_ISREG(st.st_mode) so
the regular-file check is done portably and correctly.
- Around line 1163-1164: The loop currently assumes pseudo entries are all at
the start by using "for (int i = 0; i < cntp && kif[i].fd_fd < 0; i++)", which
is fragile; change the logic that scans the kvm_getfiles() result (kif array
from kvm_getfiles) to iterate over the entire range 0..cntp-1 and explicitly
check each entry's fd type (e.g. kif[i].fd_fd < 0 and compare kif[i].fd_type or
kif[i].fd_fd against known pseudo values such as KERN_FILE_TEXT, KERN_FILE_CDIR,
KERN_FILE_RDIR) and handle the text/pseudo entries when matched, breaking only
once the desired entry is found, instead of relying on contiguous ordering.
🪄 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: 86581f85-0b91-472d-90a8-cfa638e3033d
📒 Files selected for processing (1)
drivers/unix/os_unix.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drivers/unix/os_unix.cpp`:
- Around line 66-68: The build lacks linking of libkvm for OpenBSD which causes
unresolved symbols for kvm_openfiles, kvm_getprocs, kvm_getargv, kvm_getfiles,
and kvm_close; update the platform detection logic that currently only adds
"-lkvm" for FreeBSD (the condition around the variable that appends "-lkvm") to
also include OpenBSD so the linker flag is added when platform.system() is
"OpenBSD", ensuring libkvm is linked for the code paths in os_unix.cpp that
include <kvm.h>.
- Around line 1176-1184: The fallback that builds exe from kif[i].p_comm is
unsafe because kinfo_file::p_comm is truncated (KI_MAXCOMLEN); replace that
fallback in the block around exe/fallback with a robust resolution: first try to
obtain the full argv[0] via kvm_getargv(..., proc, 0) and use it if it’s an
absolute path and stat matches kif[i].va_fsid/kif[i].va_fileid, and if that
fails, scan the directory containing the original exe (use exe.substr(0,
last_slash_pos+1)) and stat each entry to find a file whose device/inode match
kif[i].va_fsid and kif[i].va_fileid; only use kif[i].p_comm as a last resort (or
drop it) so you don’t rely on the truncated p_comm 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: 7529962a-4d5e-4cb3-938b-a40cd9d2094a
📒 Files selected for processing (1)
drivers/unix/os_unix.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drivers/unix/os_unix.cpp`:
- Around line 1280-1287: Remove the errno reset, check realpath() return value
before using resolved_path, and construct a UTF-8 String on success;
specifically, in the fallback branch that calls
realpath(OS::get_executable_path().utf8().get_data(), resolved_path) verify the
return is non-null and only then return String::utf8(resolved_path), otherwise
handle the failure (e.g., return an empty String or the original path) and log
the error; apply the same fixes to the NetBSD branch that uses realpath and
resolved_path so both paths consistently use String::utf8 and do not clobber
errno.
🪄 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: bdbd7bce-6b2d-440a-842a-3dd9b3626a5a
📒 Files selected for processing (1)
drivers/unix/os_unix.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drivers/unix/os_unix.cpp`:
- Around line 1201-1205: The early return in OS_Unix::get_executable_path()
after failing kvm_openfiles() uses a std::string variable path and returns it
directly, causing a type mismatch with Godot's String return type and bypassing
the intended fallback; change the behavior to not return std::string there —
instead ensure you convert path.c_str() (or construct a Godot String) when
returning, or better, remove the early return and let the function fall through
to the existing tail fallback added in commit 6da2fe4 so callers like
open_dynamic_library() still receive the fallback path; specifically update the
kd/kvm_openfiles() failure branch (symbols: kd, kvm_openfiles, path,
OS_Unix::get_executable_path) to either construct and return a proper Godot
String or omit the return so the fallback code executes.
🪄 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: 1f0ba4db-2d34-4313-984c-eb0eaf1417db
📒 Files selected for processing (1)
drivers/unix/os_unix.cpp
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
1 participant
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.
Summary by CodeRabbit