Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds new Desync and Automatch debug configuration options; implements PatchDesync and PatchAutomatch modules that locate game functions, install Detours hooks, and apply runtime memory patches; integrates these patches into startup (dllmain) and project files; minor ParsePattern catch-name cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application (dllmain)
participant Config as Config System
participant PatchD as PatchDesync
participant PatchA as PatchAutomatch
participant Game as Game Logic
App->>Config: Load configuration
Config-->>App: Desync & Automatch flags
App->>PatchD: GetInstance() / Initialize
App->>PatchA: GetInstance() / Initialize
alt Desync enabled
PatchD->>Game: Find patterns, attach hooks (CheckDesync/HandleDesync)
end
alt Automatch debug enabled
PatchA->>Game: Find patterns, attach hooks (joinStagingRoom/NetworkInit/isConnected)
end
Game->>PatchD: Invoke CheckDesync hook
PatchD-->>App: Log or force CRC based on config
Game->>PatchD: Invoke HandleDesync hook
PatchD-->>Game: Suppress dialog if configured
Game->>PatchA: Invoke automatch hooks
PatchA-->>App: Emit debug logs
PatchA->>Game: Forward to original functions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config.json.example`:
- Around line 12-17: The example desync block is inconsistent with the runtime
parser: update the example JSON to match the keys consumed by Config::Config()
by removing unused "logSubsystemCRC" and adding the actual keys "crcInterval"
and "disableObjectCRC", and ensure "logMismatch", "suppressDialog", and
"forceCRCMatch" remain present and correctly named so the example mirrors the
parser's expected fields.
In `@ra3-proxy/dllmain.cpp`:
- Around line 739-741: The code in dllmain.cpp narrows execStem by
static_cast<char> into logPrefix which corrupts non-ASCII names; replace the
manual char cast with a proper UTF-8 conversion from the wide string execStem
into std::string logPrefix (e.g., call WideCharToMultiByte(CP_UTF8, ...) or an
equivalent UTF-8 encoder) and handle the returned byte length to reserve/assign
logPrefix; update the block around logPrefix.reserve(execStem.size()) and the
loop so logPrefix contains a valid UTF-8 representation of execStem.
- Around line 779-783: The executable name check uses a case-sensitive
wide-string comparison on gameInfo.executableName, so desync hooks may not
install if casing differs; update the condition that compares
gameInfo.executableName to L"cnc3game.dat" to use a case-insensitive comparison
(e.g., _wcsicmp or normalize both strings to lowercase) before proceeding to
create the desync patch (PatchDesync::GetInstance()), keeping the rest of the
logic and config flags (config->logDesyncMismatch, config->suppressDesyncDialog,
config->forceCRCMatch, config->crcInterval, config->disableObjectCRC) unchanged.
In `@ra3-proxy/patch/RA3/PatchDesync.cpp`:
- Around line 211-266: The code currently always logs "Desync patches applied
successfully." and returns TRUE even if individual patches fail; change
PatchDesync logic to track overall success with a boolean (e.g., bool
allSucceeded = true), set it false when any requested patch fails (failed
VirtualProtect, missing liteCRCSet, or failed writes for
needCRCInterval/needDisableObjectCRC), log specific errors as already done, and
at the end log success only if allSucceeded is true and return TRUE; otherwise
log a consolidated failure message and return FALSE; reference symbols:
needCRCInterval, needDisableObjectCRC, liteCRCSet, VirtualProtect,
pDefaultInterval/pNetInterval, pXObjectCRC, and the final return value.
In `@ra3-proxy/patch/RA3/PatchDesync.hpp`:
- Around line 3-4: Normalize the Framework header include casing/path to match
the repository filename: update the include in PatchDesync.hpp (currently
`#include` "../../Framework.h") to the exact filename used in the repo (e.g.,
"../../framework.h" if the header is lowercase) and make the identical change in
PatchDesync.cpp (the include at the top around the existing line that references
Framework). Ensure both files use the same exact case-sensitive path so the
compiler can resolve the header on case-sensitive filesystems.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af576704-3c72-4cdd-a72a-3abe3e82a3df
📒 Files selected for processing (8)
config.json.examplera3-proxy/Config.cppra3-proxy/Config.hppra3-proxy/dllmain.cppra3-proxy/patch/RA3/PatchDesync.cppra3-proxy/patch/RA3/PatchDesync.hppra3-proxy/patch/RA3/PatchSSL.cppra3-proxy/ra3-proxy.vcxproj
Comment on lines
+12
to
+17
| "desync": { | ||
| "logMismatch": false, | ||
| "suppressDialog": false, | ||
| "logSubsystemCRC": false, | ||
| "forceCRCMatch": false | ||
| }, |
There was a problem hiding this comment.
Example desync keys are out of sync with runtime config parser.
Line 15 (logSubsystemCRC) is not consumed by Config::Config(), while crcInterval and disableObjectCRC are missing from the example. This will mislead users and silently disable intended behavior.
🔧 Proposed alignment with actual config keys
"desync": {
"logMismatch": false,
"suppressDialog": false,
- "logSubsystemCRC": false,
- "forceCRCMatch": false
+ "forceCRCMatch": false,
+ "crcInterval": 0,
+ "disableObjectCRC": false
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "desync": { | |
| "logMismatch": false, | |
| "suppressDialog": false, | |
| "logSubsystemCRC": false, | |
| "forceCRCMatch": false | |
| }, | |
| "desync": { | |
| "logMismatch": false, | |
| "suppressDialog": false, | |
| "forceCRCMatch": false, | |
| "crcInterval": 0, | |
| "disableObjectCRC": false | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config.json.example` around lines 12 - 17, The example desync block is
inconsistent with the runtime parser: update the example JSON to match the keys
consumed by Config::Config() by removing unused "logSubsystemCRC" and adding the
actual keys "crcInterval" and "disableObjectCRC", and ensure "logMismatch",
"suppressDialog", and "forceCRCMatch" remain present and correctly named so the
example mirrors the parser's expected fields.
Comment on lines
+739
to
+741
| logPrefix.reserve(execStem.size()); | ||
| for (wchar_t wc : execStem) | ||
| logPrefix.push_back(static_cast<char>(wc)); |
There was a problem hiding this comment.
Avoid lossy wide-to-narrow conversion for log prefix.
Line 741 narrows wchar_t to char, which breaks non-ASCII executable names and can produce invalid log filenames.
🔧 Proposed UTF-8-safe conversion
- logPrefix.reserve(execStem.size());
- for (wchar_t wc : execStem)
- logPrefix.push_back(static_cast<char>(wc));
+ int sizeNeeded = WideCharToMultiByte(CP_UTF8, 0, execStem.c_str(), -1, nullptr, 0, nullptr, nullptr);
+ if (sizeNeeded > 1) {
+ std::string utf8(sizeNeeded - 1, '\0');
+ WideCharToMultiByte(CP_UTF8, 0, execStem.c_str(), -1, utf8.data(), sizeNeeded, nullptr, nullptr);
+ logPrefix = std::move(utf8);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| logPrefix.reserve(execStem.size()); | |
| for (wchar_t wc : execStem) | |
| logPrefix.push_back(static_cast<char>(wc)); | |
| int sizeNeeded = WideCharToMultiByte(CP_UTF8, 0, execStem.c_str(), -1, nullptr, 0, nullptr, nullptr); | |
| if (sizeNeeded > 1) { | |
| std::string utf8(sizeNeeded - 1, '\0'); | |
| WideCharToMultiByte(CP_UTF8, 0, execStem.c_str(), -1, utf8.data(), sizeNeeded, nullptr, nullptr); | |
| logPrefix = std::move(utf8); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ra3-proxy/dllmain.cpp` around lines 739 - 741, The code in dllmain.cpp
narrows execStem by static_cast<char> into logPrefix which corrupts non-ASCII
names; replace the manual char cast with a proper UTF-8 conversion from the wide
string execStem into std::string logPrefix (e.g., call
WideCharToMultiByte(CP_UTF8, ...) or an equivalent UTF-8 encoder) and handle the
returned byte length to reserve/assign logPrefix; update the block around
logPrefix.reserve(execStem.size()) and the loop so logPrefix contains a valid
UTF-8 representation of execStem.
Comment on lines
+779
to
+783
| if ((config->logDesyncMismatch || config->suppressDesyncDialog || | ||
| config->forceCRCMatch || config->crcInterval > 0 || | ||
| config->disableObjectCRC) && | ||
| gameInfo.executableName == L"cnc3game.dat") { | ||
| const PatchDesync* desyncPatch = &PatchDesync::GetInstance(); |
There was a problem hiding this comment.
Use case-insensitive executable check before installing desync patch.
Line 782 uses an exact-case comparison. If the process filename casing differs, desync hooks won’t install even when enabled.
🔧 Proposed robust comparison
- if ((config->logDesyncMismatch || config->suppressDesyncDialog ||
- config->forceCRCMatch || config->crcInterval > 0 ||
- config->disableObjectCRC) &&
- gameInfo.executableName == L"cnc3game.dat") {
+ if ((config->logDesyncMismatch || config->suppressDesyncDialog ||
+ config->forceCRCMatch || config->crcInterval > 0 ||
+ config->disableObjectCRC) &&
+ _wcsicmp(gameInfo.executableName.c_str(), L"cnc3game.dat") == 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ra3-proxy/dllmain.cpp` around lines 779 - 783, The executable name check uses
a case-sensitive wide-string comparison on gameInfo.executableName, so desync
hooks may not install if casing differs; update the condition that compares
gameInfo.executableName to L"cnc3game.dat" to use a case-insensitive comparison
(e.g., _wcsicmp or normalize both strings to lowercase) before proceeding to
create the desync patch (PatchDesync::GetInstance()), keeping the rest of the
logic and config flags (config->logDesyncMismatch, config->suppressDesyncDialog,
config->forceCRCMatch, config->crcInterval, config->disableObjectCRC) unchanged.
Comment on lines
+211
to
+266
| if (needCRCInterval) | ||
| { | ||
| DWORD* pDefaultInterval = GlobalPtr(0x00bf3600); | ||
| DWORD* pNetInterval = GlobalPtr(0x00bf360c); | ||
| DWORD oldProtect; | ||
|
|
||
| if (VirtualProtect(pDefaultInterval, 4, PAGE_READWRITE, &oldProtect)) | ||
| { | ||
| *pDefaultInterval = static_cast<DWORD>(config.crcInterval); | ||
| VirtualProtect(pDefaultInterval, 4, oldProtect, &oldProtect); | ||
| } | ||
| if (VirtualProtect(pNetInterval, 4, PAGE_READWRITE, &oldProtect)) | ||
| { | ||
| *pNetInterval = static_cast<DWORD>(config.crcInterval); | ||
| VirtualProtect(pNetInterval, 4, oldProtect, &oldProtect); | ||
| } | ||
|
|
||
| BOOST_LOG_TRIVIAL(info) << "CRC interval overridden to " << config.crcInterval << " frames."; | ||
| } | ||
|
|
||
| // --- Disable object CRC --- | ||
| // GameLogic_Update forces g_liteCRC=1 around CRC computation, which ignores all | ||
| // exclusion flags. We NOP that write so g_xObjectCRC is honored. | ||
| // | ||
| // 004ee3f8: C6 05 53 34 BF 00 01 MOV BYTE [g_liteCRC], 1 | ||
| // 004ee406: C6 05 53 34 BF 00 00 MOV BYTE [g_liteCRC], 0 | ||
| if (needDisableObjectCRC) | ||
| { | ||
| auto liteCRCPattern = ParsePattern("C6 05 53 34 BF 00 01"); | ||
| std::byte* liteCRCSet = FindPattern(ptr, size_, liteCRCPattern); | ||
| if (liteCRCSet != nullptr) | ||
| { | ||
| DWORD oldProtect; | ||
| if (VirtualProtect(liteCRCSet, 7, PAGE_EXECUTE_READWRITE, &oldProtect)) | ||
| { | ||
| memset(liteCRCSet, 0x90, 7); | ||
| VirtualProtect(liteCRCSet, 7, oldProtect, &oldProtect); | ||
| } | ||
|
|
||
| BYTE* pXObjectCRC = reinterpret_cast<BYTE*>(GlobalPtr(0x00bf3448)); | ||
| if (VirtualProtect(pXObjectCRC, 1, PAGE_READWRITE, &oldProtect)) | ||
| { | ||
| *pXObjectCRC = 1; | ||
| VirtualProtect(pXObjectCRC, 1, oldProtect, &oldProtect); | ||
| } | ||
|
|
||
| BOOST_LOG_TRIVIAL(info) << "Disabled object CRC (g_xObjectCRC=1, liteCRC override NOPed)."; | ||
| } | ||
| else | ||
| { | ||
| BOOST_LOG_TRIVIAL(error) << "Failed to find liteCRC override pattern!"; | ||
| } | ||
| } | ||
|
|
||
| BOOST_LOG_TRIVIAL(info) << "Desync patches applied successfully."; | ||
| return TRUE; |
There was a problem hiding this comment.
Do not report success when requested desync patches fail to apply.
In this block, failures (e.g., missing liteCRC pattern or failed VirtualProtect) can occur, but Line 265 still logs success and Line 266 returns TRUE. This masks partial patch application.
🔧 Proposed fail-fast / aggregated-failure handling
BOOL PatchDesync::Patch() const
{
@@
+ bool patchFailed = false;
@@
if (needCRCInterval)
{
@@
- if (VirtualProtect(pDefaultInterval, 4, PAGE_READWRITE, &oldProtect))
+ if (VirtualProtect(pDefaultInterval, 4, PAGE_READWRITE, &oldProtect))
{
*pDefaultInterval = static_cast<DWORD>(config.crcInterval);
VirtualProtect(pDefaultInterval, 4, oldProtect, &oldProtect);
}
+ else {
+ BOOST_LOG_TRIVIAL(error) << "Failed to update g_defaultCRCInterval.";
+ patchFailed = true;
+ }
- if (VirtualProtect(pNetInterval, 4, PAGE_READWRITE, &oldProtect))
+ if (VirtualProtect(pNetInterval, 4, PAGE_READWRITE, &oldProtect))
{
*pNetInterval = static_cast<DWORD>(config.crcInterval);
VirtualProtect(pNetInterval, 4, oldProtect, &oldProtect);
}
+ else {
+ BOOST_LOG_TRIVIAL(error) << "Failed to update g_NetCRCInterval.";
+ patchFailed = true;
+ }
@@
if (needDisableObjectCRC)
{
@@
else
{
BOOST_LOG_TRIVIAL(error) << "Failed to find liteCRC override pattern!";
+ patchFailed = true;
}
}
- BOOST_LOG_TRIVIAL(info) << "Desync patches applied successfully.";
- return TRUE;
+ if (patchFailed) {
+ BOOST_LOG_TRIVIAL(error) << "Desync patching completed with failures.";
+ return FALSE;
+ }
+ BOOST_LOG_TRIVIAL(info) << "Desync patches applied successfully.";
+ return TRUE;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ra3-proxy/patch/RA3/PatchDesync.cpp` around lines 211 - 266, The code
currently always logs "Desync patches applied successfully." and returns TRUE
even if individual patches fail; change PatchDesync logic to track overall
success with a boolean (e.g., bool allSucceeded = true), set it false when any
requested patch fails (failed VirtualProtect, missing liteCRCSet, or failed
writes for needCRCInterval/needDisableObjectCRC), log specific errors as already
done, and at the end log success only if allSucceeded is true and return TRUE;
otherwise log a consolidated failure message and return FALSE; reference
symbols: needCRCInterval, needDisableObjectCRC, liteCRCSet, VirtualProtect,
pDefaultInterval/pNetInterval, pXObjectCRC, and the final return value.
Comment on lines
+3
to
+4
| #include "../../Framework.h" | ||
| #include "PatchSSL.hpp" // For PatternByte, FindPattern, ParsePattern |
There was a problem hiding this comment.
Normalize framework include path/casing to avoid case-sensitive build failures.
Static analysis flags Line 3 include resolution failure. Please align include casing/path with the actual header filename used in the repo, and mirror the same fix in ra3-proxy/patch/RA3/PatchDesync.cpp Line 11.
🔧 Proposed include fix (if header is lowercase in repo)
-#include "../../Framework.h"
+#include "../../framework.h"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #include "../../Framework.h" | |
| #include "PatchSSL.hpp" // For PatternByte, FindPattern, ParsePattern | |
| `#include` "../../framework.h" | |
| `#include` "PatchSSL.hpp" // For PatternByte, FindPattern, ParsePattern |
🧰 Tools
🪛 Clang (14.0.6)
[error] 3-3: '../../Framework.h' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ra3-proxy/patch/RA3/PatchDesync.hpp` around lines 3 - 4, Normalize the
Framework header include casing/path to match the repository filename: update
the include in PatchDesync.hpp (currently `#include` "../../Framework.h") to the
exact filename used in the repo (e.g., "../../framework.h" if the header is
lowercase) and make the identical change in PatchDesync.cpp (the include at the
top around the existing line that references Framework). Ensure both files use
the same exact case-sensitive path so the compiler can resolve the header on
case-sensitive filesystems.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
ra3-proxy/patch/RA3/PatchAutomatch.hpp (1)
11-19: Consider using the Meyer's Singleton pattern for thread-safety and simplicity.The current implementation uses a raw pointer with manual null-checking, which:
- May have a race condition if
GetInstance()is called from multiple threads before initialization.- Leaks memory (the
new-allocated instance is never deleted).C++11+ guarantees thread-safe initialization of function-local statics, so the idiomatic approach is simpler and safer:
♻️ Suggested refactor to Meyer's Singleton
static PatchAutomatch& GetInstance() { - static PatchAutomatch* instance; - - if (instance == nullptr) - instance = new PatchAutomatch(); - - return *instance; + static PatchAutomatch instance; + return instance; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ra3-proxy/patch/RA3/PatchAutomatch.hpp` around lines 11 - 19, Replace the manual pointer-based singleton in PatchAutomatch::GetInstance with a function-local static object to use the Meyer's Singleton: remove the raw pointer and new/nullptr checks in GetInstance and instead declare a static PatchAutomatch instance and return it by reference (relying on C++11+ thread-safe initialization); ensure PatchAutomatch's constructors/copy operations are appropriately private or deleted if needed to prevent copies (references: PatchAutomatch and GetInstance).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ra3-proxy/dllmain.cpp`:
- Around line 791-797: The code currently does a case-sensitive check on
gameInfo.executableName when deciding to install the automatch hooks; change
that to a case-insensitive comparison so differing filename casing still
matches. Replace the direct equality check of gameInfo.executableName ==
L"cnc3ep1.dat" with a case-insensitive comparison (for example use a wide-string
lowercase transform or a platform function like _wcsicmp / _wcsicmp_l, or
boost::iequals on converted std::wstring) before calling
PatchAutomatch::GetInstance() and automatchPatch->Patch(); keep the surrounding
flow (config->debugAutomatch guard and the error log) unchanged.
In `@ra3-proxy/patch/RA3/PatchAutomatch.cpp`:
- Line 161: The BOOST_LOG_NAMED_SCOPE macro invocation in PatchAutomatch.cpp is
missing a trailing semicolon; update the call to end with a semicolon (i.e., add
";" after BOOST_LOG_NAMED_SCOPE("AutomatchPatch")) so it matches other usages
(e.g., dllmain.cpp) and maintains consistent, correct syntax.
- Around line 118-128: hookIsConnected currently dereferences (ctx + 0x33c8)
without checking ctx; add a null check for ctx at the top of hookIsConnected and
avoid reading connState when ctx is NULL. Specifically, call
pOrigIsConnected(ctx) and return its result if ctx is null (and log a debug
message saying ctx is null), otherwise read connState from (ctx + 0x33c8) and
log it before returning the pOrigIsConnected result; update references in
hookIsConnected and keep use of pOrigIsConnected unchanged.
- Around line 96-113: hookNetworkInit dereferences ctx to read alreadyInit
without checking for null; add a guard at the top of hookNetworkInit to handle
ctx==0 (like hookJoinStagingRoom does): if ctx is null, log a warning/info, skip
the *(ctx + 0x1e08) access, and then call pOrigNetworkInit(ctx, param_2,
param_3) (or return its result) so the function behaves safely. Ensure
references to alreadyInit only occur after confirming ctx != 0 and keep the
existing logging of the returned result from pOrigNetworkInit.
- Around line 163-167: The pattern search uses ptr = entryPoint_ but passes the
full module size_ into FindPattern, which can read past the module when offset_
is non-zero; change the search length to the remaining bytes from entryPoint_
(e.g., compute remaining = size_ - offset_ or size_ - (entryPoint_ -
baseAddress) as appropriate) and call FindPattern(ptr, remaining, pattern)
inside the lambda (findFunc) to bound the search; apply the same change to the
identical code in PatchDesync (adjust use of size_ when calling FindPattern for
entryPoint_/ptr).
---
Nitpick comments:
In `@ra3-proxy/patch/RA3/PatchAutomatch.hpp`:
- Around line 11-19: Replace the manual pointer-based singleton in
PatchAutomatch::GetInstance with a function-local static object to use the
Meyer's Singleton: remove the raw pointer and new/nullptr checks in GetInstance
and instead declare a static PatchAutomatch instance and return it by reference
(relying on C++11+ thread-safe initialization); ensure PatchAutomatch's
constructors/copy operations are appropriately private or deleted if needed to
prevent copies (references: PatchAutomatch and GetInstance).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c3223a6-7568-4ed5-9fdb-6e43dc84ada3
📒 Files selected for processing (6)
ra3-proxy/Config.cppra3-proxy/Config.hppra3-proxy/dllmain.cppra3-proxy/patch/RA3/PatchAutomatch.cppra3-proxy/patch/RA3/PatchAutomatch.hppra3-proxy/ra3-proxy.vcxproj
🚧 Files skipped from review as they are similar to previous changes (1)
- ra3-proxy/ra3-proxy.vcxproj
Comment on lines
+791
to
+797
| if (config->debugAutomatch && gameInfo.executableName == L"cnc3ep1.dat") { | ||
| const PatchAutomatch* automatchPatch = &PatchAutomatch::GetInstance(); | ||
|
|
||
| if (!automatchPatch->Patch()) { | ||
| BOOST_LOG_TRIVIAL(error) << "Failed to install automatch debug hooks."; | ||
| } | ||
| } |
There was a problem hiding this comment.
Use case-insensitive executable check before installing automatch patch.
Line 791 uses an exact-case comparison for cnc3ep1.dat. This has the same issue flagged in the previous review for the desync patch check—if the process filename casing differs, automatch hooks won't install even when enabled.
🔧 Proposed robust comparison
- if (config->debugAutomatch && gameInfo.executableName == L"cnc3ep1.dat") {
+ if (config->debugAutomatch &&
+ _wcsicmp(gameInfo.executableName.c_str(), L"cnc3ep1.dat") == 0) {
const PatchAutomatch* automatchPatch = &PatchAutomatch::GetInstance();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (config->debugAutomatch && gameInfo.executableName == L"cnc3ep1.dat") { | |
| const PatchAutomatch* automatchPatch = &PatchAutomatch::GetInstance(); | |
| if (!automatchPatch->Patch()) { | |
| BOOST_LOG_TRIVIAL(error) << "Failed to install automatch debug hooks."; | |
| } | |
| } | |
| if (config->debugAutomatch && | |
| _wcsicmp(gameInfo.executableName.c_str(), L"cnc3ep1.dat") == 0) { | |
| const PatchAutomatch* automatchPatch = &PatchAutomatch::GetInstance(); | |
| if (!automatchPatch->Patch()) { | |
| BOOST_LOG_TRIVIAL(error) << "Failed to install automatch debug hooks."; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ra3-proxy/dllmain.cpp` around lines 791 - 797, The code currently does a
case-sensitive check on gameInfo.executableName when deciding to install the
automatch hooks; change that to a case-insensitive comparison so differing
filename casing still matches. Replace the direct equality check of
gameInfo.executableName == L"cnc3ep1.dat" with a case-insensitive comparison
(for example use a wide-string lowercase transform or a platform function like
_wcsicmp / _wcsicmp_l, or boost::iequals on converted std::wstring) before
calling PatchAutomatch::GetInstance() and automatchPatch->Patch(); keep the
surrounding flow (config->debugAutomatch guard and the error log) unchanged.
Comment on lines
+96
to
+113
| static int __cdecl hookNetworkInit(int ctx, int param_2, short param_3) | ||
| { | ||
| int alreadyInit = *(int*)(ctx + 0x1e08); | ||
|
|
||
| BOOST_LOG_TRIVIAL(info) | ||
| << "[AUTOMATCH] NetworkInit: ctx=0x" << std::hex << ctx << std::dec | ||
| << " alreadyInit=" << alreadyInit | ||
| << " param_2=" << param_2 | ||
| << " param_3=" << param_3; | ||
|
|
||
| int result = pOrigNetworkInit(ctx, param_2, param_3); | ||
|
|
||
| BOOST_LOG_TRIVIAL(info) | ||
| << "[AUTOMATCH] NetworkInit returned: " << result | ||
| << (alreadyInit ? " (was already initialized — returned false)" : ""); | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Add null check for ctx before dereferencing.
Unlike hookJoinStagingRoom which checks ctx != 0 before accessing memory offsets, hookNetworkInit dereferences ctx + 0x1e08 at line 98 without a null check. If ctx is ever null, this will crash.
🛡️ Proposed fix
static int __cdecl hookNetworkInit(int ctx, int param_2, short param_3)
{
+ if (ctx == 0) {
+ BOOST_LOG_TRIVIAL(warning) << "[AUTOMATCH] NetworkInit: ctx is NULL!";
+ return pOrigNetworkInit(ctx, param_2, param_3);
+ }
+
int alreadyInit = *(int*)(ctx + 0x1e08);
BOOST_LOG_TRIVIAL(info)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ra3-proxy/patch/RA3/PatchAutomatch.cpp` around lines 96 - 113,
hookNetworkInit dereferences ctx to read alreadyInit without checking for null;
add a guard at the top of hookNetworkInit to handle ctx==0 (like
hookJoinStagingRoom does): if ctx is null, log a warning/info, skip the *(ctx +
0x1e08) access, and then call pOrigNetworkInit(ctx, param_2, param_3) (or return
its result) so the function behaves safely. Ensure references to alreadyInit
only occur after confirming ctx != 0 and keep the existing logging of the
returned result from pOrigNetworkInit.
Comment on lines
+118
to
+128
| static int __cdecl hookIsConnected(int ctx) | ||
| { | ||
| int connState = *(int*)(ctx + 0x33c8); | ||
| int result = pOrigIsConnected(ctx); | ||
|
|
||
| BOOST_LOG_TRIVIAL(debug) | ||
| << "[AUTOMATCH] isConnected: state=" << connState | ||
| << " result=" << result; | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Add null check for ctx before dereferencing.
Similar to hookNetworkInit, hookIsConnected dereferences ctx + 0x33c8 at line 120 without verifying ctx is non-null.
🛡️ Proposed fix
static int __cdecl hookIsConnected(int ctx)
{
+ if (ctx == 0) {
+ BOOST_LOG_TRIVIAL(warning) << "[AUTOMATCH] isConnected: ctx is NULL!";
+ return pOrigIsConnected(ctx);
+ }
+
int connState = *(int*)(ctx + 0x33c8);
int result = pOrigIsConnected(ctx);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ra3-proxy/patch/RA3/PatchAutomatch.cpp` around lines 118 - 128,
hookIsConnected currently dereferences (ctx + 0x33c8) without checking ctx; add
a null check for ctx at the top of hookIsConnected and avoid reading connState
when ctx is NULL. Specifically, call pOrigIsConnected(ctx) and return its result
if ctx is null (and log a debug message saying ctx is null), otherwise read
connState from (ctx + 0x33c8) and log it before returning the pOrigIsConnected
result; update references in hookIsConnected and keep use of pOrigIsConnected
unchanged.
|
|
||
| BOOL PatchAutomatch::Patch() const | ||
| { | ||
| BOOST_LOG_NAMED_SCOPE("AutomatchPatch") |
There was a problem hiding this comment.
Missing semicolon after BOOST_LOG_NAMED_SCOPE macro.
The BOOST_LOG_NAMED_SCOPE macro invocation should be followed by a semicolon for correctness. Without it, the code may compile due to how the macro expands, but it's inconsistent with usage elsewhere in the codebase (e.g., dllmain.cpp line 747).
🔧 Proposed fix
BOOL PatchAutomatch::Patch() const
{
- BOOST_LOG_NAMED_SCOPE("AutomatchPatch")
+ BOOST_LOG_NAMED_SCOPE("AutomatchPatch");
std::byte* ptr = reinterpret_cast<std::byte*>(entryPoint_);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| BOOST_LOG_NAMED_SCOPE("AutomatchPatch") | |
| BOOL PatchAutomatch::Patch() const | |
| { | |
| BOOST_LOG_NAMED_SCOPE("AutomatchPatch"); | |
| std::byte* ptr = reinterpret_cast<std::byte*>(entryPoint_); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ra3-proxy/patch/RA3/PatchAutomatch.cpp` at line 161, The
BOOST_LOG_NAMED_SCOPE macro invocation in PatchAutomatch.cpp is missing a
trailing semicolon; update the call to end with a semicolon (i.e., add ";" after
BOOST_LOG_NAMED_SCOPE("AutomatchPatch")) so it matches other usages (e.g.,
dllmain.cpp) and maintains consistent, correct syntax.
Comment on lines
+163
to
+167
| std::byte* ptr = reinterpret_cast<std::byte*>(entryPoint_); | ||
|
|
||
| auto findFunc = [&](const char* patternStr, const char* name) -> std::byte* { | ||
| auto pattern = ParsePattern(patternStr); | ||
| std::byte* addr = FindPattern(ptr, size_, pattern); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other patch files handle the search bounds
rg -n "FindPattern.*size_" --type=cpp -C3Repository: sokie/cnc_game_proxy
Length of output: 5232
🏁 Script executed:
#!/bin/bash
# Read PatchAutomatch.cpp to understand the context around line 163-167
head -200 ra3-proxy/patch/RA3/PatchAutomatch.cpp | tail -150Repository: sokie/cnc_game_proxy
Length of output: 5056
🏁 Script executed:
#!/bin/bash
# Find variable declarations for entryPoint_, offset_, baseAddress_, size_
rg -n "entryPoint_|offset_|baseAddress_|size_" ra3-proxy/patch/RA3/PatchAutomatch.cpp | head -30Repository: sokie/cnc_game_proxy
Length of output: 488
🏁 Script executed:
#!/bin/bash
# Check the class definition and initialization
rg -n "class.*Automatch|entryPoint_\s*=|offset_\s*=|baseAddress_\s*=" ra3-proxy/patch/RA3/PatchAutomatch.cpp -B2 -A2Repository: sokie/cnc_game_proxy
Length of output: 307
🏁 Script executed:
#!/bin/bash
# Check PatchDesync.cpp context around line 163
sed -n '140,170p' ra3-proxy/patch/RA3/PatchDesync.cppRepository: sokie/cnc_game_proxy
Length of output: 1091
🏁 Script executed:
#!/bin/bash
# Check if PatchDesync.cpp also has similar initialization pattern
rg -n "baseAddress_|offset_|entryPoint_" ra3-proxy/patch/RA3/PatchDesync.cpp | head -20Repository: sokie/cnc_game_proxy
Length of output: 304
Pattern search may exceed module bounds.
The search starts at entryPoint_ (base + offset) but uses size_ which is the full module size. This means the search could read beyond the module's memory region if offset_ is non-zero. The issue also exists identically in PatchDesync.cpp.
🔧 Proposed fix to adjust search length
std::byte* ptr = reinterpret_cast<std::byte*>(entryPoint_);
auto findFunc = [&](const char* patternStr, const char* name) -> std::byte* {
auto pattern = ParsePattern(patternStr);
+ size_t searchLen = size_ > offset_ ? size_ - offset_ : 0;
- std::byte* addr = FindPattern(ptr, size_, pattern);
+ std::byte* addr = FindPattern(ptr, searchLen, pattern);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ra3-proxy/patch/RA3/PatchAutomatch.cpp` around lines 163 - 167, The pattern
search uses ptr = entryPoint_ but passes the full module size_ into FindPattern,
which can read past the module when offset_ is non-zero; change the search
length to the remaining bytes from entryPoint_ (e.g., compute remaining = size_
- offset_ or size_ - (entryPoint_ - baseAddress) as appropriate) and call
FindPattern(ptr, remaining, pattern) inside the lambda (findFunc) to bound the
search; apply the same change to the identical code in PatchDesync (adjust use
of size_ when calling FindPattern for entryPoint_/ptr).
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