Pointer chain checks - #311
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull request overview
This PR appears intended to introduce/support “pointer chain checks” for Warden, but the code changes themselves don’t implement a new Warden check type; instead it adds a contrib SQL seed/example file describing a new check type (244 / 0xF4) plus a small header-include change.
Changes:
- Add a new contrib SQL file (
pointer_chain_examples.sql) with examplewardentable rows for a proposedPOINTER_CHAIN_CHECKtype. - Add an extra standard library include in
WardenWin.h. - No functional logic changes are evident in
WardenWin.cpp/WardenCheckMgr.cppbeyond the diff re-rendering.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/game/Warden/WardenWin.h | Adds a standard header include (currently unused as written). |
| src/game/Warden/WardenWin.cpp | No functional change observed in the shown diff. |
| src/game/Warden/WardenCheckMgr.cpp | No functional change observed in the shown diff; review focused on DB-driven type handling. |
| contrib/warden/pointer_chain_examples.sql | Adds example SQL seeds/documentation for a new proposed Warden check type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include "Warden.h" | ||
| #include <vector> | ||
|
|
| -- ============================================================================ | ||
| -- POINTER_CHAIN_CHECK (type 244 / 0xF4) example seeds for the `warden` table. | ||
| -- | ||
| -- Wire format on the client side is identical to MEM_CHECK (243 / 0xF3); the | ||
| -- server walks a multi-hop pointer dereference chain across consecutive Warden |
| uint16 id = fields[0].GetUInt16(); | ||
| uint16 build = fields[1].GetUInt16(); | ||
| uint8 checkType = fields[2].GetUInt8(); | ||
| std::string data = fields[3].GetString(); | ||
| std::string checkResult = fields[4].GetString(); | ||
| uint32 address = fields[5].GetUInt32(); | ||
| uint8 length = fields[6].GetUInt8(); | ||
| std::string str = fields[7].GetString(); | ||
| std::string comment = fields[8].GetString(); | ||
|
|
||
| WardenCheck* wardenCheck = new WardenCheck(); | ||
| wardenCheck->Type = checkType; |
PR mangoszero#311's branch had diverged from mangoszero/master with an unrelated history that could no longer be merged. Re-base the pointer-chain-check feature (Warden CUSTOM/POINTER_CHAIN_CHECK 244: multi-hop pointer-deref scanning with optional signature-detect invert-match) cleanly onto the current master tip. Squashes the three original feature commits; no upstream history is discarded beyond the stale divergent base. Warden.h enum, WardenWin.{h,cpp} handling, WardenCheckMgr loaders, and the contrib/warden SQL examples are all preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GD6LV6acKwGd426dDQGPDF
2fb9aed to
012748e
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 012748eb2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // If a POINTER_CHAIN_CHECK chain is mid-walk, consume one slot for it (do not pop a new id). | ||
| if (_pointerChainActive) | ||
| { | ||
| _currentChecks.push_back(_pointerChainInFlight.checkId); | ||
| continue; |
There was a problem hiding this comment.
With the checked default Warden.NumMemChecks = 3, starting or resuming a pointer chain causes every remaining loop iteration to append the same check ID. The request therefore contains multiple identical reads for one hop, while HandleData advances the single global chain state for each duplicate response; later responses are interpreted as subsequent hops or as terminal bytes, producing false failures and potentially leaving Warden stuck in STATE_REQUESTED_DATA. Schedule the active chain only once per request.
Useful? React with 👍 / 👎.
| VALUES | ||
| (10002, 5875, 244, '', | ||
| '0000000000', -- TODO: per-OS captured bytes of kernel32!GetTickCount prologue | ||
| 0x00C2D154, 5, '', |
There was a problem hiding this comment.
This row uses an empty offset list, which StartPointerChain treats as a zero-hop check, so it reads five bytes directly from the IAT slot rather than dereferencing the slot and reading the GetTickCount prologue. An operator who replaces the placeholder with clean prologue bytes as instructed will therefore flag every clean client; this one-hop example needs a 0x0 offset.
Useful? React with 👍 / 👎.
| VALUES | ||
| (10003, 5875, 244, '', | ||
| '04', -- TYPEID_PLAYER | ||
| 0x00B41414, 1, '0x0,0xAC,0x14', |
There was a problem hiding this comment.
The implementation dereferences the base before consuming the first offset, so this leading 0x0 makes the chain read objMgrInstance + 0 as a pointer and then follow vtable + 0xAC. It never reads the documented objMgrInstance + 0xAC linked-list head, causing the terminal 04 comparison to target unrelated code memory; the offsets for the described chain should begin with 0xAC.
Useful? React with 👍 / 👎.
| if (isMemCheck) | ||
| { | ||
| if ((it->second->Type == MEM_CHECK) || (it->second->Type == MODULE_CHECK)) | ||
| if ((it->second->Type == MEM_CHECK) || (it->second->Type == MODULE_CHECK) || (it->second->Type == POINTER_CHAIN_CHECK)) |
There was a problem hiding this comment.
GetWardenCheckIds(false) unconditionally returns every check type, so adding pointer chains to the memory category also leaves them in _otherChecksTodo. That path never calls StartPointerChain, yet serialization reads _pointerChainInFlight; consequently a pointer row is either sent a second time using another chain's state or, when memory checks are disabled, sent with the constructor's zero address and length. The response then fails the active-chain ID check and returns without restoring Warden to its resting state.
Useful? React with 👍 / 👎.
| int cmp = memcmp(buff.contents() + buff.rpos(), rs->Result.AsByteArray(0, false), | ||
| _pointerChainInFlight.finalLength); |
There was a problem hiding this comment.
For expected results containing leading zero bytes, BigNumber does not reliably retain the configured byte width or ordering; the supplied 00003000 signature is one such case. This comparison nevertheless reads finalLength bytes from AsByteArray(0, false), so these checks compare against altered bytes and can read beyond the returned allocation when its encoded value is shorter. Pointer-check results need fixed-width byte storage or explicit length validation before this memcmp.
Useful? React with 👍 / 👎.
| { | ||
| // Intermediate hop: read 4-byte LE pointer, advance chain, await next cycle. | ||
| uint32 ptr; | ||
| memcpy(&ptr, buff.contents() + buff.rpos(), sizeof(uint32)); |
There was a problem hiding this comment.
On a supported big-endian server host, this raw memcpy interprets the Windows client's little-endian pointer bytes in host order, reversing every intermediate address and making all non-zero-hop chains read unrelated memory or fail. Use the repository's endian-aware ByteBuffer extraction or apply EndianConvert after copying.
Useful? React with 👍 / 👎.
| -- Caveat: kernel32.dll is part of the OS, so the prologue bytes vary across | ||
| -- Windows versions. In practice you'd seed `result` per-OS or use a small | ||
| -- whitelist via multiple check rows. Listed here as a textbook IAT-hook | ||
| -- pattern; consider it a template, not a drop-in. |
There was a problem hiding this comment.
The suggested multiple-row whitelist cannot work with this scheduler: every row for the same client build is independently queued and a mismatch on any one row is penalized. If operators add several GetTickCount prologues for different Windows versions as advised, each client matches at most one and fails all the others; the implementation needs explicit alternative-result semantics or an OS-specific selection key rather than separate ordinary checks.
Useful? React with 👍 / 👎.
| (10003, 5875, 244, '', | ||
| '04', -- TYPEID_PLAYER | ||
| 0x00B41414, 1, '0x0,0xAC,0x14', | ||
| 'Pointer chain: ObjMgr -> first object -> typeId byte (detect object spoof)'); |
There was a problem hiding this comment.
Even after correcting the offsets, this row requires the first object in the Object Manager list to have TYPEID_PLAYER, while the immediately preceding note acknowledges that the list head is not necessarily the local player. A clean client whose head is a unit, game object, item, or other object type will therefore fail the 04 comparison and receive the default Warden penalty; the chain must locate the local GUID first or check a genuinely invariant field.
Useful? React with 👍 / 👎.
| if (peek && peek->Type == POINTER_CHAIN_CHECK) | ||
| { | ||
| StartPointerChain(peek); | ||
| if (!_pointerChainActive) | ||
| { | ||
| // Malformed chain; loader should have filtered it. Skip without scheduling. | ||
| continue; |
There was a problem hiding this comment.
Malformed offset strings are detected only after the check ID has been popped for an individual session, at which point the code silently omits the check from the request. Because LoadWardenChecks does not perform the validation claimed by this comment, a database typo is still reported as a loaded check but disables that protection for every client and repeatedly generates runtime log noise; reject or quarantine the row during loading instead.
Useful? React with 👍 / 👎.
This change is