fest: Add resolver to win#64
Merged
Seele-Official merged 4 commits intoclice-io:mainfrom Mar 23, 2026
Merged
Conversation
- Updated `CreateProcessA` and `CreateProcessW` to use template functions for proxy path and IPC ID retrieval. - Refactored `Resolver` class to improve path resolution logic and added new methods for dynamic environment variable retrieval. - Introduced `CallWinApiWithGrowingBuffer` to handle Windows API calls with dynamic buffer sizes. - Created `resolver.cc` to implement the `Resolver` class methods. - Added unit tests for resolver functionality, including application name resolution and command line parsing. - Removed redundant code from `util.cc` and `util.h`, consolidating functionality into the `Resolver` class.
Contributor
📝 WalkthroughWalkthroughRefactors Windows payload: adds WinAPI buffer helpers, a Resolver for app-name resolution, payload utilities for path/proxy/ipc retrieval, updates CreateProcess detours to use those utilities, adds unit tests, and adds a module-definition plus build changes to include new sources and linker def file. Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as Hook (CreateProcess detour)
participant Util as Payload Utils
participant Resolver as Resolver
participant WinAPI as WinAPI / OS (SearchPath, Env)
participant FS as FileSystem
Hook->>Util: resolve_abspath(applicationName, commandLine)
Util->>Resolver: select resolver (app-name or cmdline)
Resolver->>WinAPI: SearchPathDynamic(...) / GetModuleDirectory / GetSystemDirectory
WinAPI-->>Resolver: resolved path (or none)
Resolver-->>Util: resolved absolute path
Util->>WinAPI: GetEnvironmentVariableDynamic(CATTER_PROXY_PATH / CATTER_IPC_ID)
WinAPI-->>Util: env values
Util-->>Hook: proxy_path, ipc_id, resolved_path
Hook->>FS: launch process using resolved_path and proxy/ipc settings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/unit/catter-hook/win/payload/util.cc (1)
14-36: Consider extractingScopedEnvVarto a shared test utility.This RAII helper is duplicated in
tests/unit/catter-hook/win/payload/resolver.cc(lines 63-85). Extracting it to a common test utility header would reduce duplication and ensure consistent behavior across tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/catter-hook/win/payload/util.cc` around lines 14 - 36, ScopedEnvVar is duplicated; extract it into a shared test utility header and replace the two copies with an include. Create a header (e.g., test_util/scoped_env_var.h) that declares and defines the ScopedEnvVar struct (keep the same members, constructor behavior that uses GetEnvironmentVariableW and SetEnvironmentVariableW, and the destructor logic), update the original definition in util.cc and the duplicate in resolver.cc to remove the local struct, and include the new header where needed so both tests use the single shared ScopedEnvVar implementation.tests/unit/catter-hook/win/payload/resolver.cc (1)
44-61: Consider handlingMAX_PATHlimitation for edge cases.
ScopedCurrentDirectoryuses a fixedMAX_PATHbuffer (260 chars). While this is typically sufficient, long paths enabled via registry or manifests could exceed this. For test code this is likely acceptable, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/catter-hook/win/payload/resolver.cc` around lines 44 - 61, ScopedCurrentDirectory currently uses a fixed wchar_t[MAX_PATH] buffer when calling GetCurrentDirectoryW which can fail for long paths; update the constructor (ScopedCurrentDirectory) to handle longer paths by calling GetCurrentDirectoryW to obtain the required length and then allocate a dynamic buffer (e.g., std::wstring or std::vector<wchar_t>) sized to len+1 and call GetCurrentDirectoryW again, check for errors (len == 0 or returned length larger than buffer) and only assign original when successful; keep using SetCurrentDirectoryW(path.wstring().c_str()) and in the destructor call SetCurrentDirectoryW(original.c_str()) if original was captured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/catter-hook/win/payload/main.cc`:
- Around line 68-73: The wide-character CreateProcessW::detour builds
converted_cmdline using potentially empty returns from
catter::win::payload::get_proxy_path<wchar_t>(),
catter::win::payload::get_ipc_id<wchar_t>() and
catter::win::payload::resolve_abspath(...); add explicit validation for each of
those calls before calling std::format in the CreateProcessW::detour path: if
any return is empty, log or propagate an error and abort the detour (or
substitute a safe default) so you never format with empty values; specifically
check the results of get_proxy_path<wchar_t>(), get_ipc_id<wchar_t>(), and
resolve_abspath(lpApplicationName, lpCommandLine) and handle empty strings
consistently (return false / log) prior to constructing converted_cmdline.
- Around line 32-37: The formatted command line built in converted_cmdline using
catter::win::payload::get_proxy_path<char>() and
catter::win::payload::get_ipc_id<char>() can be malformed if either function
returns an empty string; add validation after calling get_proxy_path<char>() and
get_ipc_id<char>() (before constructing converted_cmdline) to detect empty
results and either supply a sane default/fallback value or log an error and fail
gracefully; update the logic around converted_cmdline construction (and any
callers) so you never format with empty proxy path or ipc id—use the function
names get_proxy_path<char>(), get_ipc_id<char>(), and converted_cmdline to
locate where to add the checks and fallback/error handling.
In `@src/catter-hook/win/payload/resolver.cc`:
- Around line 58-67: The function Resolver<char_t>::fix_app_name calls
app_name.back() which is UB for empty input; add an early guard that returns an
empty std::basic_string<char_t> if app_name.empty() (or otherwise handle empty
deterministically) before evaluating contains_path(app_name), app_name.back(),
or has_extension(app_name). Update the function (referencing
Resolver<char_t>::fix_app_name, contains_path, has_extension, exe_suffix, and
resolve()) so the empty check occurs first to avoid calling app_name.back() on
an empty view.
In `@src/catter-hook/win/payload/resolver.h`:
- Around line 41-55: The file declares the factory templates
create_app_name_resolver and create_command_line_resolver twice; remove the
duplicate set so each template declaration appears only once. Locate the
duplicated template declarations for template <CharT char_t> Resolver<char_t>
create_app_name_resolver(); and template <CharT char_t> Resolver<char_t>
create_command_line_resolver(); (they reference Resolver and CharT/char_t) and
delete the second occurrence, leaving a single, unique declaration pair
alongside the extern template class Resolver<char>/Resolver<wchar_t> lines.
In `@src/catter-hook/win/payload/winapi.h`:
- Around line 174-189: GetModuleDirectory's current two-step separator search on
module_path (using pos and find_last_of) fails for mixed-separator paths;
replace the two-step search with a single find_last_of that looks for both '\\'
and '/' at once (choose the correct literal for char_t, e.g. "\\/" for char or
L"\\/" for wchar_t, or construct a std::basic_string<char_t> containing both
separators) so pos becomes the last index of either separator and substr returns
the proper directory; update references to pos and module_path in
GetModuleDirectory and ensure CharT/char_t type selection is used when building
the separator string.
---
Nitpick comments:
In `@tests/unit/catter-hook/win/payload/resolver.cc`:
- Around line 44-61: ScopedCurrentDirectory currently uses a fixed
wchar_t[MAX_PATH] buffer when calling GetCurrentDirectoryW which can fail for
long paths; update the constructor (ScopedCurrentDirectory) to handle longer
paths by calling GetCurrentDirectoryW to obtain the required length and then
allocate a dynamic buffer (e.g., std::wstring or std::vector<wchar_t>) sized to
len+1 and call GetCurrentDirectoryW again, check for errors (len == 0 or
returned length larger than buffer) and only assign original when successful;
keep using SetCurrentDirectoryW(path.wstring().c_str()) and in the destructor
call SetCurrentDirectoryW(original.c_str()) if original was captured.
In `@tests/unit/catter-hook/win/payload/util.cc`:
- Around line 14-36: ScopedEnvVar is duplicated; extract it into a shared test
utility header and replace the two copies with an include. Create a header
(e.g., test_util/scoped_env_var.h) that declares and defines the ScopedEnvVar
struct (keep the same members, constructor behavior that uses
GetEnvironmentVariableW and SetEnvironmentVariableW, and the destructor logic),
update the original definition in util.cc and the duplicate in resolver.cc to
remove the local struct, and include the new header where needed so both tests
use the single shared ScopedEnvVar implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fe4e253-3c50-4607-a56f-c50ece7a66f2
📒 Files selected for processing (11)
src/catter-hook/win/payload/exports.defsrc/catter-hook/win/payload/main.ccsrc/catter-hook/win/payload/resolver.ccsrc/catter-hook/win/payload/resolver.hsrc/catter-hook/win/payload/util.ccsrc/catter-hook/win/payload/util.hsrc/catter-hook/win/payload/winapi.htests/unit/catter-hook/win/payload/main.cctests/unit/catter-hook/win/payload/resolver.cctests/unit/catter-hook/win/payload/util.ccxmake.lua
💤 Files with no reviewable changes (1)
- tests/unit/catter-hook/win/payload/main.cc
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/catter-hook/win/payload/resolver.h (1)
20-20: Clarify thatresolve()can return the original token.
resolve()reads like it always returns a resolved path, butsrc/catter-hook/win/payload/resolver.cc:82-100falls back to the input on a miss, andsrc/catter-hook/win/payload/util.cc:43-63forwards that value directly. A short contract comment here would keep callers from assuming the result is always absolute or existing.✏️ Proposed clarification
- std::basic_string<char_t> resolve(std::basic_string_view<char_t> app_name) const; + // Returns the resolved path when SearchPath succeeds; otherwise returns + // `app_name` unchanged. + std::basic_string<char_t> resolve(std::basic_string_view<char_t> app_name) const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/catter-hook/win/payload/resolver.h` at line 20, Add a short contract comment to the declaration of resolve() clarifying that resolve(std::basic_string_view<char_t> app_name) may return the original app_name token unmodified on a miss (i.e., it does not guarantee an absolute or existing path); update the comment above resolve() in resolver.h so callers (and downstream users like the code in util.cc that forwards the value) know to validate the returned string before treating it as a resolved filesystem path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/catter-hook/win/payload/resolver.h`:
- Line 20: Add a short contract comment to the declaration of resolve()
clarifying that resolve(std::basic_string_view<char_t> app_name) may return the
original app_name token unmodified on a miss (i.e., it does not guarantee an
absolute or existing path); update the comment above resolve() in resolver.h so
callers (and downstream users like the code in util.cc that forwards the value)
know to validate the returned string before treating it as a resolved filesystem
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52bef704-ab37-445f-961d-86cc57ac05e2
📒 Files selected for processing (3)
src/catter-hook/win/payload/resolver.ccsrc/catter-hook/win/payload/resolver.hsrc/catter-hook/win/payload/winapi.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/catter-hook/win/payload/resolver.cc
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
New Features
Improvements
Tests