Session/agent 0d19c700 7fb5 44d4 ab75 2b8ab9741f18#6
Conversation
- convert structured exception handling blocks (__try/__except) to C++ try/catch in SkinChanger for robustness - replace plain strncpy with strncpy_s for safe name copying - fix lowercasing logic in item search with explicit casts for correctness - expose internal function pointers (s_fn_create_econ_item, s_fn_set_dynamic_attr, s_fn_get_econ_item_system) as public - move g_h_module to file scope and declare extern WndProc before subclassing window procedure - sdk/inventory.hpp: generalize CallVFunc to support multiple Args and proper forwarding; adjust Windows include and add type_traits/utility
Reviewer's GuideRefactors the skinchanger and inventory interaction code for safer, more standard-compliant C++ usage (replacing SEH with C++ exceptions, tightening casts and string handling), introduces a more robust vtable call helper, and performs a few naming and initialization cleanups. Class diagram for updated cs2::CallVFunc helperclassDiagram
class cs2 {
+CallVFunc~T, Index, Args...~(thisptr : void*, args : Args...) T
}
%% Template details
class CallVFuncTemplate {
<<template>>
+T
+Index : size_t
+Args...
+Fn = T(__thiscall*)(void*, remove_reference_t<Args>...)
}
cs2 ..> CallVFuncTemplate : uses
Class diagram for SkinChanger static API and fieldsclassDiagram
class SkinChanger {
<<static>>
-s_initialized : bool
-s_client_base : uintptr_t
+s_fn_create_econ_item : uintptr_t
+s_fn_set_dynamic_attr : uintptr_t
+s_fn_get_econ_item_system : uintptr_t
+s_fn_inventory_manager : uintptr_t
+s_fn_set_model : uintptr_t
+s_fn_set_mesh_group_mask : uintptr_t
+Shutdown() void
+CreateAndEquipItem(inventory : uintptr_t, manager : uintptr_t, team : int, slot : int, def_index : int, paint_kit_id : int, seed : int, wear : float, stattrak : int, custom_name : std::string) bool
+ApplyGloves(inventory : uintptr_t, pawn : uintptr_t, view_model : uintptr_t) void
+OnFrameStageNotify(stage : int) void
+OnSetModel(entity : void*, model_path : const char*&) void
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe pull request refactors exception handling from Windows SEH to C++ try-catch, updates the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Replacing
__try/__exceptwith C++try/catch(...)changes what kinds of faults are actually caught (e.g., access violations are no longer handled unless compiled with/EHa), so if these wrappers are meant to guard against unsafe external calls you may want to keep SEH or explicitly document/ensure the exception model used. - The new
CallVFuncsignature requires the vtable index as a template parameter (Index) instead of a runtimesize_t index; if there are existing call sites still passing an index at runtime they will now fail to compile or need refactoring, so it’s worth double-checking or adding a small helper/wrapper to ease the transition.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Replacing `__try/__except` with C++ `try/catch(...)` changes what kinds of faults are actually caught (e.g., access violations are no longer handled unless compiled with `/EHa`), so if these wrappers are meant to guard against unsafe external calls you may want to keep SEH or explicitly document/ensure the exception model used.
- The new `CallVFunc` signature requires the vtable index as a template parameter (`Index`) instead of a runtime `size_t index`; if there are existing call sites still passing an index at runtime they will now fail to compile or need refactoring, so it’s worth double-checking or adding a small helper/wrapper to ease the transition.
## Individual Comments
### Comment 1
<location path="src/features/skinchanger/skinchanger.cpp" line_range="140-141" />
<code_context>
using Fn = uintptr_t(__fastcall*)();
- __try { return ((Fn)SkinChanger::s_fn_inventory_manager)(); }
- __except (EXCEPTION_EXECUTE_HANDLER) { return 0; }
+ try { return ((Fn)SkinChanger::s_fn_inventory_manager)(); }
+ catch (...) { return 0; }
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Using C++ try/catch instead of SEH __try/__except may no longer guard against access violations.
Previously this used `__try/__except`, which can handle SEH exceptions (e.g., access violations) when calling engine/game function pointers. Standard C++ `try`/`catch (...)` will not catch SEH unless the code is compiled with `/EHa`, so access violations may now crash the process instead of being handled. Either keep SEH for this boundary call or ensure `/EHa` is required and documented.
</issue_to_address>
### Comment 2
<location path="src/features/skinchanger/skinchanger.cpp" line_range="170-171" />
<code_context>
using Fn = CEconItem_t*(__cdecl*)();
- __try { return ((Fn)SkinChanger::s_fn_create_econ_item)(); }
- __except (EXCEPTION_EXECUTE_HANDLER) { return nullptr; }
+ try { return ((Fn)SkinChanger::s_fn_create_econ_item)(); }
+ catch (...) { return nullptr; }
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Same SEH vs C++ exception handling concern for econ item creation and attribute setting.
This function (and `SetDynamicAttributeValue`, `ApplyGloves`, `SetModel`, `SetMeshGroupMask`) now uses `try`/`catch (...)` instead of `__try/__except`. If these engine function pointers can raise SEH faults (e.g., due to engine changes or invalid state), C++ exceptions will not catch them and the process may crash. To maintain the previous fault-tolerance, either keep SEH around these calls or document and enforce that only C++ exceptions can be thrown here (via build config/contracts).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| try { return ((Fn)SkinChanger::s_fn_inventory_manager)(); } | ||
| catch (...) { return 0; } |
There was a problem hiding this comment.
issue (bug_risk): Using C++ try/catch instead of SEH __try/__except may no longer guard against access violations.
Previously this used __try/__except, which can handle SEH exceptions (e.g., access violations) when calling engine/game function pointers. Standard C++ try/catch (...) will not catch SEH unless the code is compiled with /EHa, so access violations may now crash the process instead of being handled. Either keep SEH for this boundary call or ensure /EHa is required and documented.
| try { return ((Fn)SkinChanger::s_fn_create_econ_item)(); } | ||
| catch (...) { return nullptr; } |
There was a problem hiding this comment.
issue (bug_risk): Same SEH vs C++ exception handling concern for econ item creation and attribute setting.
This function (and SetDynamicAttributeValue, ApplyGloves, SetModel, SetMeshGroupMask) now uses try/catch (...) instead of __try/__except. If these engine function pointers can raise SEH faults (e.g., due to engine changes or invalid state), C++ exceptions will not catch them and the process may crash. To maintain the previous fault-tolerance, either keep SEH around these calls or document and enforce that only C++ exceptions can be thrown here (via build config/contracts).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors several components, including the CallVFunc template, exception handling mechanisms, and string processing logic. Key feedback includes concerns regarding the replacement of Structured Exception Handling (SEH) with standard C++ exceptions, which may not catch hardware exceptions like access violations in this context. Additionally, the CallVFunc refactor incorrectly strips reference qualifiers, potentially leading to stack corruption, and the inline extern declaration of WndProc should be moved to improve code maintainability.
| try { return ((Fn)SkinChanger::s_fn_inventory_manager)(); } | ||
| catch (...) { return 0; } |
There was a problem hiding this comment.
Replacing Structured Exception Handling (__try/__except) with standard C++ exceptions (try/catch) is dangerous in this context. Standard C++ exceptions do not catch hardware exceptions like Access Violations (EXCEPTION_ACCESS_VIOLATION) on Windows unless the code is compiled with the /EHa flag. Since this code interacts with game memory via potentially unstable pointers, using try/catch (...) will likely fail to prevent crashes that the original SEH code would have handled. This pattern is repeated throughout this file (e.g., lines 170, 177, 362, 490, 502, 542).
| using Fn = T(__thiscall*)(void*, std::remove_reference_t<Args>...); | ||
| return (*reinterpret_cast<Fn**>(thisptr))[Index](thisptr, std::forward<Args>(args)...); |
There was a problem hiding this comment.
The use of std::remove_reference_t<Args>... in the function pointer signature strips reference qualifiers. If a virtual function expects a reference (e.g., const Vector&), the Fn type will incorrectly be defined as taking the object by value, leading to stack corruption or incorrect behavior. Using Args... directly in the function pointer signature is more appropriate as it preserves the value category (lvalue/rvalue) of the arguments passed to the template.
using Fn = T(__thiscall*)(void*, Args...);
return (*reinterpret_cast<Fn**>(thisptr))[Index](thisptr, std::forward<Args>(args)...);|
|
||
| // Subclass window | ||
| // Subclass window - declare WndProc first | ||
| extern LRESULT CALLBACK WndProc(HWND, UINT, WPARAM, LPARAM); |
There was a problem hiding this comment.
Declaring an extern function inside a function body is generally discouraged as it obscures dependencies and can make the code harder to maintain. It is better to declare WndProc at the top of the file or in a shared header file.
There was a problem hiding this comment.
Pull request overview
This PR appears focused on improving low-level SDK/helpers used by the skin changer (vtable calling helpers, Windows/UI glue code) and tightening up a few string/character handling details.
Changes:
- Refactors
CallVFuncto use a compile-time vtable index and perfect-forwarded args. - Adjusts
SkinChangerheader visibility for scanned function pointers and updates several engine-call guard patterns inskinchanger.cpp. - Improves safety/typing in a few utility spots (e.g.,
strncpy_s,std::tolowercasting) and tweaks Win32 WndProc wiring.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/sdk/inventory.hpp |
Updates vfunc-calling helper to be index-at-compile-time and adds needed headers. |
src/features/skinchanger/skinchanger.hpp |
Changes access control to expose scanned function pointers via a new public: block. |
src/features/skinchanger/skinchanger.cpp |
Replaces several SEH guards with C++ try/catch, adjusts some call casts, uses strncpy_s, and updates knife config field naming. |
src/features/skinchanger/items.cpp |
Ensures tolower results are safely converted back to char. |
src/dllmain.cpp |
Moves g_h_module global and adds an in-function WndProc forward declaration before subclassing. |
Comments suppressed due to low confidence (1)
src/features/skinchanger/skinchanger.hpp:125
- Adding a second
public:section here makes the scanned function pointer addresses (s_fn_*) part of the public API, which exposes internal hooking/offset details to any includer. Since these are only used by helper functions in skinchanger.cpp, consider keeping them private and moving those helpers intoSkinChangeras private static methods (or using friend declarations) to preserve encapsulation.
static uintptr_t s_client_base;
public:
static uintptr_t s_fn_create_econ_item;
static uintptr_t s_fn_set_dynamic_attr;
static uintptr_t s_fn_get_econ_item_system;
static uintptr_t s_fn_inventory_manager;
static uintptr_t s_fn_update_subclass;
static uintptr_t s_fn_set_mesh_group_mask;
static uintptr_t s_fn_set_model;
static uintptr_t s_force_viewmodel_fn;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { return ((Fn)SkinChanger::s_fn_inventory_manager)(); } | ||
| catch (...) { return 0; } |
There was a problem hiding this comment.
Replacing SEH (__try/__except) with C++ try/catch here will not catch access violations or other structured exceptions under the current build flags (CMakeLists.txt does not enable /EHa or an SEH-to-C++ translator). These calls are invoking scanned function pointers, so a bad address will still crash the process. Please restore __try/__except for these guard calls, or explicitly enable SEH translation (/EHa or _set_se_translator) and justify the behavior change.
| try { return ((Fn)SkinChanger::s_fn_inventory_manager)(); } | |
| catch (...) { return 0; } | |
| __try { | |
| return ((Fn)SkinChanger::s_fn_inventory_manager)(); | |
| } | |
| __except (EXCEPTION_EXECUTE_HANDLER) { | |
| return 0; | |
| } |
| static CEconItem_t* CreateEconItem() { | ||
| if (!SkinChanger::s_fn_create_econ_item) return nullptr; | ||
| using Fn = CEconItem_t*(__cdecl*)(); | ||
| __try { return ((Fn)SkinChanger::s_fn_create_econ_item)(); } | ||
| __except (EXCEPTION_EXECUTE_HANDLER) { return nullptr; } | ||
| try { return ((Fn)SkinChanger::s_fn_create_econ_item)(); } | ||
| catch (...) { return nullptr; } |
There was a problem hiding this comment.
Same issue as above: C++ catch(...) will not intercept SEH faults from calling a potentially-invalid scanned function pointer, so this no longer provides crash protection. Prefer keeping the original __try/__except guard (or add /EHa / SEH translator if you intend to rely on catch(...)).
| static void SetDynamicAttributeValue(CEconItem_t* item, uint16_t attr_index, float value) { | ||
| if (!SkinChanger::s_fn_set_dynamic_attr || !item) return; | ||
| using Fn = void(__fastcall*)(CEconItem_t*, uint16_t, float); | ||
| __try { ((Fn)SkinChanger::s_fn_set_dynamic_attr)(item, attr_index, value); } | ||
| __except (EXCEPTION_EXECUTE_HANDLER) {} | ||
| try { ((Fn)SkinChanger::s_fn_set_dynamic_attr)(item, attr_index, value); } | ||
| catch (...) {} |
There was a problem hiding this comment.
Same issue as above: this try/catch won’t catch structured exceptions (e.g., access violations) thrown by the engine call, so the guard is ineffective with the current exception model. Consider reverting to __try/__except for this boundary call, or introduce explicit SEH translation in the build/runtime.
| try { ((Fn)s_fn_set_mesh_group_mask)(view_model, 1); } | ||
| catch (...) {} |
There was a problem hiding this comment.
C++ catch(...) here won’t catch access violations from the engine call unless you compile with /EHa or install an SEH-to-C++ translator. This code previously used SEH to prevent crashes; consider reverting to __try/__except around this external call boundary.
| try { ((Fn)s_fn_set_mesh_group_mask)(view_model, 1); } | |
| catch (...) {} | |
| __try { | |
| ((Fn)s_fn_set_mesh_group_mask)(view_model, 1); | |
| } __except (EXCEPTION_EXECUTE_HANDLER) { | |
| } |
| using SetModelFn = void(__fastcall*)(uintptr_t, const char*); | ||
| __try { | ||
| try { | ||
| ((SetModelFn)s_fn_set_model)(entity, def->model_path.c_str()); | ||
| if (view_model) | ||
| ((SetModelFn)s_fn_set_model)(view_model, def->model_path.c_str()); | ||
| } | ||
| __except (EXCEPTION_EXECUTE_HANDLER) {} | ||
| catch (...) {} | ||
| } |
There was a problem hiding this comment.
The try/catch guard around this scanned function pointer call will not catch SEH exceptions (e.g., AV) with the current build configuration, so it won’t prevent hard crashes. Please restore __try/__except (or add /EHa / SEH translation) for these engine boundary calls.
| // MeshGroupMask | ||
| if (s_fn_set_mesh_group_mask) { | ||
| using Fn = void(__fastcall*)(uintptr_t, uint32_t); | ||
| __try { | ||
| try { | ||
| ((Fn)s_fn_set_mesh_group_mask)(entity, 2); | ||
| if (view_model) | ||
| ((Fn)s_fn_set_mesh_group_mask)(view_model, 2); | ||
| } | ||
| __except (EXCEPTION_EXECUTE_HANDLER) {} | ||
| catch (...) {} |
There was a problem hiding this comment.
Same as above: catch(...) won’t intercept structured exceptions from this engine call without /EHa or an SEH translator, so the crash-guard behavior has effectively been removed. Consider reverting to __try/__except around this call site.
| if (s_fn_set_mesh_group_mask) { | ||
| using Fn = void(__fastcall*)(uintptr_t, uint32_t); | ||
| bool legacy = false; | ||
| if (const DumpedPaintKit* pk = ItemDatabase::FindPaintKit(skin.paint_kit_id)) | ||
| legacy = pk->is_legacy; | ||
| uint32_t mask = legacy ? 2 : 1; | ||
| __try { | ||
| try { | ||
| ((Fn)s_fn_set_mesh_group_mask)(entity, mask); | ||
| if (view_model) { | ||
| uint32_t vm_weapon = *reinterpret_cast<uint32_t*>(view_model + 0x38); | ||
| if (vm_weapon == static_cast<uint32_t>(i)) | ||
| ((Fn)s_fn_set_mesh_group_mask)(view_model, mask); | ||
| } | ||
| } | ||
| __except (EXCEPTION_EXECUTE_HANDLER) {} | ||
| catch (...) {} |
There was a problem hiding this comment.
Same issue: this try/catch does not protect against access violations from calling into engine code/function pointers under /EHsc. If the intent is crash hardening, prefer SEH (__try/__except) at this boundary or add explicit SEH translation support.
| // Subclass window - declare WndProc first | ||
| extern LRESULT CALLBACK WndProc(HWND, UINT, WPARAM, LPARAM); | ||
| g_original_wnd_proc = reinterpret_cast<WNDPROC>( | ||
| SetWindowLongPtr(g_hwnd, GWLP_WNDPROC, reinterpret_cast<LONG_PTR>(WndProc))); |
There was a problem hiding this comment.
This forward declaration uses extern (external linkage), but the actual WndProc definition below is static (internal linkage). That linkage mismatch is a compile error in C++. Fix by forward-declaring it as static (ideally at file scope above HookedPresent), or remove the extern and move a correct prototype before first use.
Summary by Sourcery
Modernize skin changer and inventory helper code for safer C++ usage and minor API fixes.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
Bug Fixes
Chores