Files

T

Andrei C d89d733051 fix(core/player): clamp vendor purchase quantity before validation

2026-07-26 12:51:23 +00:00

6.4 KiB

Raw Blame History

ArgusCore Fix Backlog

Tracks the incremental security/stability/correctness/blizzlike maintenance pass. One fix per entry. See repo root workflow conventions for process rules.

Legend: [ ] open · [WIP] in progress · [DONE] shipped

P0 — Security / Corruption

[WIP] Core/Player - Vendor purchase quantity desync (validate-vs-deliver truncation)

Subsystem: Entities/Player, item vendor purchase

Problem: Player::BuyItemFromVendorSlot(..., uint32 count, ...) validates stock limits, gold price, and extended-cost reagent/currency ownership against the full client-supplied count (an int32 read directly off CMSG_BUY_ITEM's Quantity field, widened to uint32). It then forwards count into Player::_StoreOrEquipNewItem(..., uint8 count, ...), which silently narrows it to count % 256. Actual item delivery, reagent/currency consumption (DestroyItemCount/RemoveCurrency), and vendor stock decrement (UpdateVendorItemCurrentCount) all use the truncated value, while the "can you afford/own this" checks earlier used the untruncated value. A single crafted packet with Quantity > 255 desyncs what was validated from what actually executes — a client-triggerable economic integrity fault.

Files: src/server/game/Entities/Player/Player.cpp (BuyItemFromVendorSlot, _StoreOrEquipNewItem), src/server/game/Handlers/ItemHandler.cpp (HandleBuyItemOpcode)

Reference: None directly ported; DestinyCore keeps count as uint8 end-to-end (no widening, so no desync there). ArgusCore widened the outer function to uint32 without updating the inner one.

Fix: Clamp count to uint8 range immediately after the existing if (count < 1) count = 1; cheat guard in BuyItemFromVendorSlot, before any validation runs, so every downstream check validates the exact quantity that will be charged and delivered.

Test: See commit / manual test instructions below.

P1 — Stability

[ ] Core/Unit - TriggerAuraHeartbeat iterator invalidation

Subsystem: Entities/Unit, Spells/Auras

Problem: Unit::TriggerAuraHeartbeat() (Unit.cpp:513-519) iterates m_appliedAuras (a std::multimap) with a plain range-based for while calling Aura::Heartbeat(), which invokes arbitrary AuraScript::OnHeartbeat hooks. If a heartbeat hook removes the aura currently being visited (a normal aura-scripting pattern — "cancel self if condition no longer met"), Unit::_UnapplyAura erases that exact map node mid-loop, which is undefined behavior for the range-for's hidden iterator.

Files: src/server/game/Entities/Unit/Unit.cpp

Reference: Not present in DestinyCore (the heartbeat-aura mechanic is Legion-specific). Fix is precedented in-tree: Unit::AtEnterCombat() / Unit::AtExitCombat() (same file, ~line 8774) invoke the same class of arbitrary aura-script hook over the same container using an erase-safe idiom (capture the pointer, advance the iterator, then invoke the hook).

Fix: Convert TriggerAuraHeartbeat's loop to the same erase-safe idiom already used by AtEnterCombat/AtExitCombat.

Status: Not started — next in queue after the vendor purchase fix.

[ ] Core/Vehicle - Null seat dereference in VehicleHandler

Subsystem: Handlers/VehicleHandler, Entities/Vehicle

Problem: HandleRequestVehiclePrevSeat, HandleRequestVehicleNextSeat, HandleMoveChangeVehicleSeats, and HandleRequestVehicleSwitchSeat (VehicleHandler.cpp:45-113) call Vehicle::GetSeatForPassenger(), which is documented to return null if the passenger isn't currently seated, and dereference the result without a null check (contrast CombatHandler.cpp:51-52, which at least ASSERTs). A rapid double-send of a vehicle-seat opcode during a seat-change race window is a plausible client-triggerable null deref.

Files: src/server/game/Handlers/VehicleHandler.cpp

Reference: Same missing check exists in DestinyCore — inherited from upstream TrinityCore, not ArgusCore-specific. Lower priority than the heartbeat bug since it's long-standing and shared across forks, but still a live crash bug.

Status: Not started.

[ ] Core/Loot - Unguarded division in HandleLootMoneyOpcode

Subsystem: Handlers/LootHandler

Problem: uint32 goldPerPlayer = uint32((loot->gold) / (playersNear.size())); (LootHandler.cpp:~168) has no guard against playersNear being empty. In normal play the looter is always included, so this is low-exploitability today, but it's an unguarded integer division on a client-triggerable path (CMSG_LOOT_MONEY).

Files: src/server/game/Handlers/LootHandler.cpp

Reference: Same pattern exists in DestinyCore; not ArgusCore-specific.

Status: Not started — low priority, defensive-only.

P2 — Core Correctness

[ ] Core/Bank - Slot/PackSlot argument order inconsistency

Subsystem: Handlers/BankHandler

Problem: HandleAutoBankReagentOpcode calls GetItemByPos(autoBankReagent.PackSlot, autoBankReagent.Slot) while HandleAutoStoreBankReagentOpcode calls GetItemByPos(autoStoreBankReagent.Slot, autoStoreBankReagent.PackSlot) — reversed argument order relative to its sibling. Each packet's Read() implementation reads the two fields in different wire order, so this may be self-consistent and not an actual bug — needs verification against a real client packet capture before touching it.

Files: src/server/game/Handlers/BankHandler.cpp

Status: Needs verification, not confirmed as a bug. Low priority.

P3 — Blizzlike Gameplay

(Not populated by this pass — see ROADMAP.md / plans/00-arguscore-analysis-and-roadmap.md for the active class-spell-porting and content workstreams, which are tracked separately from this security/stability backlog.)

P4 — Performance / Cleanup

(Not populated by this pass.)

Notes on scope

This backlog is deliberately shallow on first pass — it reflects two targeted, read-only audits (item/gold/trade exploit surface; crash/stability surface) rather than an exhaustive line-by-line review. Areas checked and found solidly guarded during this pass (not re-audited unless something changes nearby): TradeHandler, MailHandler (including mail money/COD overflow guards), AuctionHouseHandler, GuildHandler/Guild.cpp bank-item moves, VoidStorageHandler, movement/teleport coordinate validation (MovementHandler.cpp, ValidateMovementInfo), Spell.cpp/ SpellEffects.cpp target-pointer handling, LootHandler.cpp target/master-loot lookups, Group.cpp aura/member iteration.

6.4 KiB Raw Blame History