From a7bdac83923de13ccf09f99cb14bdadcf5f8aad7 Mon Sep 17 00:00:00 2001 From: Emerson Freitas Date: Tue, 17 Mar 2026 15:13:14 -0300 Subject: [PATCH] fix: harden troop processing race conditions (#98) (#138) --- CHANGELOG.md | 13 ++++++++++ GameEngine/Automation.php | 50 ++++++++++++++++++++++++--------------- GameEngine/Database.php | 10 ++++++++ GameEngine/Units.php | 11 ++++++--- 4 files changed, 62 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 33378419..54d3197c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,19 @@ Format inspired by Keep a Changelog and Semantic Versioning principles. ## [Unreleased] +### Fixed +- Troop duplication race-condition hardening for issue #98. +- `sendunitsComplete()` now uses atomic per-record movement claim (`proc = 1` only when current worker owns the row) before processing reinforcements. +- `returnunitsComplete()` now uses the same atomic claim flow for returning troops and canceled-settler returns. +- `sendSettlersComplete()` now claims movement rows atomically before processing settle/return outcomes. + +### Security and Safety +- Added idempotent A2B request claiming in troop send flow to prevent duplicate processing from concurrent/replayed submissions. +- Introduced `claimA2b()` in database layer and consumed it in `sendTroops()` before any troop/resource mutation. + +### Changed +- Movement completion status updates for the hardened flows are now claim-first (atomic) instead of late batch marking. + ## [2026-03-14] - Stability, PHP 8, Docker and Installer Hardening ### Added diff --git a/GameEngine/Automation.php b/GameEngine/Automation.php index a7221fbf..d2b12a97 100644 --- a/GameEngine/Automation.php +++ b/GameEngine/Automation.php @@ -856,6 +856,20 @@ class Automation { } } + private function claimMovementRecord($moveid) { + global $database; + + $moveid = (int)$moveid; + if ($moveid <= 0) { + return false; + } + + $q = "UPDATE ".TB_PREFIX."movement SET proc = 1 WHERE moveid = $moveid AND proc = 0"; + mysqli_query($database->dblink, $q); + + return (mysqli_affected_rows($database->dblink) === 1); + } + private function sendunitsComplete() { global $bid19, $bid23, $bid34, $u99, $database, $battle, $technology, $units; @@ -2625,8 +2639,11 @@ class Automation { $database->getVillageByWorldID($vilIDs); // calculate reinforcements data - $movementProcIDs = []; foreach($dataarray as $data) { + if (!$this->claimMovementRecord($data['moveid'])) { + continue; + } + $isoasis = $database->isVillageOases($data['to']); if($isoasis == 0){ $to = $database->getMInfo($data['to']); @@ -2649,7 +2666,6 @@ class Automation { $database->modifyEnforce($reinf['id'], 31, 1, 1); $data_fail = '0,0,4,1,0,0,0,0,0,0,0,0,0,0'; $database->addNotice($to['owner'], $to['wref'], (isset($targetally) ? $targetally : 0), 8, 'village of the elders reinforcement ' . addslashes($to['name']), $data_fail, $AttackArrivalTime); - $movementProcIDs[] = $data['moveid']; }else{ //set base things $from = $database->getMInfo($data['from']); @@ -2718,8 +2734,6 @@ class Automation { if($from['owner'] != $to['owner']) { $database->addNotice($to['owner'],$to['wref'],(isset($targetally) ? $targetally : 0),8,''.addslashes($from['name']).' reinforcement '.addslashes($to_name).'',$data_fail,(isset($AttackArrivalTime) ? $AttackArrivalTime : time())); } - //update status - $movementProcIDs[] = $data['moveid']; } //Update starvation data @@ -2733,8 +2747,6 @@ class Automation { $q = "DELETE FROM ".TB_PREFIX."enforcement WHERE ".$e_units." AND (vref=".(int) $data['to']." OR `from`=".(int) $data['to'].")"; $database->query($q); } - - $database->setMovementProc(implode(', ', $movementProcIDs)); } } @@ -2770,8 +2782,11 @@ class Automation { $database->getOasisEnforce($vilIDs, 0); $database->getOasisEnforce($vilIDs, 1); - $movementProcIDs = []; foreach($dataarray as $data) { + if (!$this->claimMovementRecord($data['moveid'])) { + continue; + } + $tribe = $database->getUserField($database->getVillageField($data['to'], "owner"), "tribe", 0); $u = $tribe == 1 ? "" : $tribe - 1; $database->modifyUnit( @@ -2786,31 +2801,28 @@ class Automation { $database->modifyResource($data['to'], $data['wood'], $data['clay'], $data['iron'], $data['crop'], 1); } - $movementProcIDs[] = $data['moveid']; - //Update starvation data $database->addStarvationData($data['to']); } - - $database->setMovementProc(implode(', ', $movementProcIDs)); + $this->pruneResource(); } // Settlers $q = "SELECT `to`, moveid FROM ".TB_PREFIX."movement where ref = 0 and proc = '0' and sort_type = '4' and endtime < $time"; $dataarray = $database->query_return($q); - $movementProcIDs = []; - if ($dataarray && count($dataarray)) { foreach($dataarray as $data) { + if (!$this->claimMovementRecord($data['moveid'])) { + continue; + } + $tribe = $database->getUserField($database->getVillageField($data['to'], "owner"), "tribe", 0); $database->modifyUnit($data['to'], [$tribe."0"], [3], [1]); //If a settling is canceled, add 750 for each resource type $database->modifyResource($data['to'], 750, 750, 750, 750, 1); - $movementProcIDs[] = $data['moveid']; } - $database->setMovementProc(implode(', ', $movementProcIDs)); } } @@ -2821,7 +2833,6 @@ class Automation { $q = "SELECT `to`, `from`, moveid, starttime, ref FROM ".TB_PREFIX."movement where proc = 0 and sort_type = 5 and endtime < $time"; $dataarray = $database->query_return($q); - $movementProcIDs = []; $fieldIDs = []; $addUnitsWrefs = []; $addTechWrefs = []; @@ -2845,6 +2856,10 @@ class Automation { $database->getVillageByWorldID($vilIDs); foreach($dataarray as $data) { + if (!$this->claimMovementRecord($data['moveid'])) { + continue; + } + $ownerID = $database->getUserField($database->getVillageField($data['from'], "owner"), "id", 0); $to = $database->getMInfo($data['from']); $user = addslashes($database->getUserField($to['owner'], 'username', 0)); @@ -2856,7 +2871,6 @@ class Automation { $addUnitsWrefs[] = $data['to']; $addTechWrefs[] = $data['to']; $addABTechWrefs[] = $data['to']; - $movementProcIDs[] = $data['moveid']; $exp1 = $database->getVillageField($data['from'], 'exp1'); $exp2 = $database->getVillageField($data['from'], 'exp2'); @@ -2882,12 +2896,10 @@ class Automation { $refs[] = $data['ref']; $times[] = $time; $endtimes[] = $time + ($time - $data['starttime']); - $movementProcIDs[] = $data['moveid']; } } $database->addMovement($types, $froms, $tos, $refs, $times, $endtimes); - $database->setMovementProc(implode(', ', $movementProcIDs)); $database->setFieldTaken($fieldIDs); $database->addUnits($addUnitsWrefs); $database->addTech($addTechWrefs); diff --git a/GameEngine/Database.php b/GameEngine/Database.php index f1dde1dd..5af163b3 100755 --- a/GameEngine/Database.php +++ b/GameEngine/Database.php @@ -5684,6 +5684,16 @@ References: User ID/Message ID, Mode return mysqli_query($this->dblink,$q); } + function claimA2b($id, $ckey) { + $id = (int)$id; + list($ckey) = $this->escape_input($ckey); + + $q = "DELETE FROM " . TB_PREFIX . "a2b WHERE id = $id AND ckey = '".$ckey."' LIMIT 1"; + mysqli_query($this->dblink, $q); + + return (mysqli_affected_rows($this->dblink) === 1); + } + // no need to cache this method function getA2b($ckey) { list($ckey) = $this->escape_input($ckey); diff --git a/GameEngine/Units.php b/GameEngine/Units.php index 2afa9136..8e58bce7 100755 --- a/GameEngine/Units.php +++ b/GameEngine/Units.php @@ -276,6 +276,14 @@ class Units { header( "Location: a2b.php" ); exit; }else{ + if (empty($data['id']) || !$database->claimA2b((int)$data['id'], $post['timestamp_checksum'])) { + $form->addError("error", "This troop send request was already processed. Please try again."); + $_SESSION['errorarray'] = $form->getErrors(); + $_SESSION['valuearray'] = $_POST; + header("Location: a2b.php"); + exit; + } + $u = ($session->tribe == 1) ? "" : $session->tribe - 1; $database->modifyUnit( @@ -401,9 +409,6 @@ class Units { exit; } - // prevent re-use of the same attack via re-POSTing the same data - $database->remA2b($data['id']); - header("Location: build.php?id=39"); exit; }