From e120b7ea6d4b64bfa55def140daac408936474f9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 09:29:17 +0000 Subject: [PATCH] fix: never drop a created potion reference when its recipe check fails AddPotion/AddPoison can return an existing created potion, including one the player already holds. Releasing it on a recipe mismatch could free a form an inventory still points at. Refuse it without releasing, ignore effect cost (gold value only) in the check, and log both recipes so a refused potion can be diagnosed from the client log. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01SSW5HGXzi9kAVm4zdsSaid --- .../client/Games/Skyrim/Forms/AlchemyItem.cpp | 43 +++++++++++-------- Code/client/Games/Skyrim/Forms/AlchemyItem.h | 4 +- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/Code/client/Games/Skyrim/Forms/AlchemyItem.cpp b/Code/client/Games/Skyrim/Forms/AlchemyItem.cpp index ed0607c7e..7e6ad7398 100644 --- a/Code/client/Games/Skyrim/Forms/AlchemyItem.cpp +++ b/Code/client/Games/Skyrim/Forms/AlchemyItem.cpp @@ -58,6 +58,18 @@ AlchemyItem* AlchemyItem::Find(TESObjectREFR* apOwner, const Inventory::PotionDa return nullptr; } +namespace +{ +// Effect cost only feeds the item's gold value; the engine may keep its own when it reuses a created potion. +bool SameEffects(const Inventory::PotionData& acLhs, const Inventory::PotionData& acRhs) noexcept +{ + return acLhs.IsPoison == acRhs.IsPoison && acLhs.Effects.size() == acRhs.Effects.size() && + std::is_permutation(acLhs.Effects.begin(), acLhs.Effects.end(), acRhs.Effects.begin(), [](const auto& acA, const auto& acB) { + return acA.EffectId == acB.EffectId && acA.Magnitude == acB.Magnitude && acA.Area == acB.Area && acA.Duration == acB.Duration; + }); +} +} // namespace + AlchemyItem* AlchemyItem::Create(const Inventory::PotionData& aData) noexcept { if (!aData.IsValid()) @@ -91,24 +103,21 @@ AlchemyItem* AlchemyItem::Create(const Inventory::PotionData& aData) noexcept AlchemyItem* created = nullptr; TiltedPhoques::ThisCall(aData.IsPoison ? addPoison : addPotion, manager, &created, &effects); Memory::Free(effects.data); - if (created) + if (!created) { - Inventory::PotionData check; - CaptureRecipe(created, check); - if (check == aData) - return created; - spdlog::error("[TradeService]: AddPotion returned effects/type differing from the offered potion"); - Release(created); + spdlog::error("[TradeService]: {} returned no form for a {}-effect recipe", aData.IsPoison ? "AddPoison" : "AddPotion", aData.Effects.size()); + return nullptr; } + Inventory::PotionData check; + CaptureRecipe(created, check); + if (SameEffects(check, aData)) + return created; + // The engine may hand back an existing created potion, including one in this player's own inventory. + // Dropping its reference here could free a form an inventory still holds, so it is only refused. + spdlog::error("[TradeService]: {} returned {:X} whose effects differ from the offered recipe", aData.IsPoison ? "AddPoison" : "AddPotion", created->formID); + for (const auto& effect : aData.Effects) + spdlog::error("[TradeService]: wanted {:X}:{:X} magnitude {} area {} duration {} cost {}", effect.EffectId.ModId, effect.EffectId.BaseId, effect.Magnitude, effect.Area, effect.Duration, effect.RawCost); + for (const auto& effect : check.Effects) + spdlog::error("[TradeService]: got {:X}:{:X} magnitude {} area {} duration {} cost {}", effect.EffectId.ModId, effect.EffectId.BaseId, effect.Magnitude, effect.Area, effect.Duration, effect.RawCost); return nullptr; } - -void AlchemyItem::Release(AlchemyItem* apItem) noexcept -{ - auto* manager = BGSCreatedObjectManager::Get(); - if (!apItem || !manager) - return; - TP_THIS_FUNCTION(TDecrementRef, void, BGSCreatedObjectManager, AlchemyItem*); - POINTER_SKYRIMSE(TDecrementRef, decrementRef, 36171); - TiltedPhoques::ThisCall(decrementRef, manager, apItem); -} diff --git a/Code/client/Games/Skyrim/Forms/AlchemyItem.h b/Code/client/Games/Skyrim/Forms/AlchemyItem.h index 4877f302c..90974e0e9 100644 --- a/Code/client/Games/Skyrim/Forms/AlchemyItem.h +++ b/Code/client/Games/Skyrim/Forms/AlchemyItem.h @@ -34,9 +34,9 @@ struct AlchemyItem : MagicItem static bool CaptureRecipe(TESForm* apForm, Inventory::PotionData& aData) noexcept; static void Capture(TESForm* apForm, Inventory::Entry& aEntry) noexcept; static AlchemyItem* Find(TESObjectREFR* apOwner, const Inventory::PotionData& aData) noexcept; - // The returned pointer owns one created-object-manager reference. Release it after use. + // The returned form may be an existing created potion with the same effects. Its created-object + // reference is never dropped: other inventories can point at the same form. static AlchemyItem* Create(const Inventory::PotionData& aData) noexcept; - static void Release(AlchemyItem* apItem) noexcept; }; static_assert(sizeof(AlchemyItem::Data) == 0x20);