Repository: RetroAchievements/RAIntegration
Pull request #819 (2021-10-26 23:49:21)

fix overflow error calculating percentage of very large measured target
Originally reported against RetroArch [here](https://retroachievements.org/viewtopic.php?t=11632&c=97999).


Patch:
diff --git a/rcheevos b/rcheevos
index d8431e92..9e5ed877 160000
--- a/rcheevos
+++ b/rcheevos
@@ -1,1 +1,1 @@
-Subproject commit d8431e92ea5305d244c0afccb0d69504893d6133
+Subproject commit 9e5ed8775522d4b46f843c03eaddaf89686f6456
diff --git a/src/data/models/TriggerValidation.cpp b/src/data/models/TriggerValidation.cpp
index 30242896..ee1367b0 100644
--- a/src/data/models/TriggerValidation.cpp
+++ b/src/data/models/TriggerValidation.cpp
@@ -6,7 +6,7 @@
 
 #include "services/ServiceLocator.hh"
 
-#include <rcheevos/include/rc_runtime_types.h>
+#include <rcheevos/src/rcheevos/rc_validate.h>
 
 namespace ra {
 namespace data {
@@ -42,257 +42,37 @@ MemSize TriggerValidation::MapRcheevosMemSize(char nSize) noexcept
     }
 }
 
-static unsigned MaxValue(const rc_operand_t* pOperand)
-{
-    if (pOperand->type == RC_OPERAND_CONST)
-        return pOperand->value.num;
-
-    if (!rc_operand_is_memref(pOperand))
-        return 0xFFFFFFFF;
-
-    return MemSizeMax(TriggerValidation::MapRcheevosMemSize(pOperand->size));
-}
-
-static int ValidateRange(unsigned nMinValue, unsigned nMaxValue, char nOperator, unsigned nMax, std::wstring& sError) {
-    switch (nOperator) {
-        case RC_OPERATOR_AND:
-            if (nMinValue > nMax) {
-                sError = L"Mask has more bits than source";
-                return false;
-            }
-            else if (nMinValue == 0 && nMaxValue == 0) {
-                sError = L"Result of mask is always 0";
-                return false;
-            }
-            break;
-
-        case RC_OPERATOR_EQ:
-            if (nMinValue > nMax) {
-                sError = L"Comparison is never true";
-                return false;
-            }
-            break;
-
-        case RC_OPERATOR_NE:
-            if (nMinValue > nMax) {
-                sError = L"Comparison is always true";
-                return false;
-            }
-            break;
-
-        case RC_OPERATOR_GE:
-            if (nMinValue > nMax) {
-                sError = L"Comparison is never true";
-                return false;
-            }
-            if (nMaxValue == 0) {
-                sError = L"Comparison is always true";
-                return false;
-            }
-            break;
-
-        case RC_OPERATOR_GT:
-            if (nMinValue >= nMax) {
-                sError = L"Comparison is never true";
-                return false;
-            }
-            break;
-
-        case RC_OPERATOR_LE:
-            if (nMinValue >= nMax) {
-                sError = L"Comparison is always true";
-                return false;
-            }
-            break;
-
-        case RC_OPERATOR_LT:
-            if (nMinValue > nMax) {
-                sError = L"Comparison is always true";
-                return false;
-            }
-            if (nMaxValue == 0) {
-                sError = L"Comparison is never true";
-                return false;
-            }
-            break;
-    }
-
-    return true;
-}
-
-static bool ValidateCondSet(const rc_condset_t* pCondSet, std::wstring& sError)
+bool TriggerValidation::Validate(const std::string& sTrigger, std::wstring& sError)
 {
-    bool bInAddHits = false;
-    bool bIsCombining = false;
-    bool bInAddAddress = false;
-    unsigned long long nAddSourceMax = 0;
-    ra::ByteAddress nMaxAddress = 0;
-
-    int nIndex = 1;
-    const rc_condition_t* pCondition = pCondSet->conditions;
-    for (; pCondition != nullptr; ++nIndex, pCondition = pCondition->next)
-    {
-        unsigned nMax = MaxValue(&pCondition->operand1); // maximum possible calculated value for comparison
-
-        if (!bInAddAddress)
-        {
-            if (nMaxAddress == 0)
-            {
-                if (ra::services::ServiceLocator::Exists<ra::data::context::EmulatorContext>())
-                {
-                    const auto& pEmulatorContext = ra::services::ServiceLocator::Get<ra::data::context::EmulatorContext>();
-                    nMaxAddress = gsl::narrow_cast<ra::ByteAddress>(pEmulatorContext.TotalMemorySize());
-                }
-
-                if (nMaxAddress == 0)
-                    nMaxAddress = 0xFFFFFFFF;
-                else
-                    nMaxAddress--;
-            }
-
-            if ((rc_operand_is_memref(&pCondition->operand1) && pCondition->operand1.value.memref && pCondition->operand1.value.memref->address > nMaxAddress) ||
-                (rc_operand_is_memref(&pCondition->operand2) && pCondition->operand2.value.memref && pCondition->operand2.value.memref->address > nMaxAddress))
-            {
-                sError = ra::StringPrintf(L"Condition %u: Address out of range (max %04X).", nIndex, nMaxAddress);
-                return false;
-            }
-        }
-
-        switch (pCondition->type)
-        {
-            case RC_CONDITION_ADD_SOURCE:
-                nAddSourceMax += nMax;
-                bIsCombining = true;
-                continue;
-
-            case RC_CONDITION_SUB_SOURCE:
-                if (nAddSourceMax < nMax) // ignore potential underflow - may be expected
-                    nAddSourceMax = 0xFFFFFFFF;
-                bIsCombining = true;
-                continue;
-
-            case RC_CONDITION_ADD_HITS:
-            case RC_CONDITION_SUB_HITS:
-                bIsCombining = true;
-                bInAddHits = true;
-                break;
-
-            case RC_CONDITION_ADD_ADDRESS:
-            case RC_CONDITION_AND_NEXT:
-            case RC_CONDITION_OR_NEXT:
-            case RC_CONDITION_RESET_NEXT_IF:
-                bIsCombining = true;
-                break;
-
-            default:
-                if (bInAddHits)
-                {
-                    if (pCondition->required_hits == 0)
-                    {
-                        sError = ra::StringPrintf(L"Condition %u: Final condition in AddHits chain must have a hit target.", nIndex);
-                        return false;
-                    }
-
-                    bInAddHits = false;
-                }
-
-                bIsCombining = false;
-                break;
-        }
-
-        if (nAddSourceMax)
-        {
-            const unsigned long long overflow = nAddSourceMax + nMax;
-            if (overflow > 0xFFFFFFFFUL)
-                nMax = 0xFFFFFFFF;
-            else
-                nMax = gsl::narrow_cast<unsigned>(overflow);
-        }
-
-        // maximum comparison value
-        const unsigned nMaxValue = MaxValue(&pCondition->operand2);
-
-        // minimum comparison value
-        const unsigned nMinValue = (pCondition->operand2.type == RC_OPERAND_CONST) ? pCondition->operand2.value.num : 0;
-
-        // if comparing two memrefs outside of an AddSource chain, make sure their sizes match
-        const bool bIsMemRef = rc_operand_is_memref(&pCondition->operand1);
-        if (bIsMemRef && nMaxValue != nMax && nAddSourceMax == 0 && rc_operand_is_memref(&pCondition->operand2))
-        {
-            sError = ra::StringPrintf(L"Condition %u: Comparing different memory sizes", nIndex);
-            return false;
-        }
-
-        // if nMax is from a memory reference or AddSource chain, or the right side is a memory reference,
-        // validate that nMax can fall between bMinValue and nMaxValue
-        if (bIsMemRef || nMinValue == 0 || nAddSourceMax)
-        {
-            if (!ValidateRange(nMinValue, nMaxValue, pCondition->oper, nMax, sError))
-            {
-                sError.insert(0, L"Condition : ");
-                sError.insert(10, std::to_wstring(nIndex));
-                return false;
-            }
-        }
-
-        // reset AddSource chain
-        nAddSourceMax = 0;
-
-        // AddAddress only ever effects the next condition
-        bInAddAddress = (pCondition->type == RC_CONDITION_ADD_ADDRESS);
-    }
-
-    if (bIsCombining)
+    const auto nSize = rc_trigger_size(sTrigger.c_str());
+    if (nSize < 0)
     {
-        sError = L"Final condition type expects another condition to follow.";
+        sError = ra::Widen(rc_error_str(nSize));
         return false;
     }
 
-    return true;
-}
+    std::string sTriggerBuffer;
+    sTriggerBuffer.resize(nSize);
+    const auto* pTrigger = rc_parse_trigger(sTriggerBuffer.data(), sTrigger.c_str(), nullptr, 0);
 
-static bool ValidateTrigger(const rc_trigger_t* pTrigger, std::wstring& sError)
-{
-    if (pTrigger->requirement && !ValidateCondSet(pTrigger->requirement, sError))
+    unsigned nMaxAddress = ra::to_unsigned(-1);
+    if (ra::services::ServiceLocator::Exists<ra::data::context::EmulatorContext>())
     {
-        if (pTrigger->alternative != nullptr)
-            sError.insert(0, L"Core: ");
-
-        return false;
+        const auto& pEmulatorContext = ra::services::ServiceLocator::Get<ra::data::context::EmulatorContext>();
+        nMaxAddress = gsl::narrow_cast<unsigned>(pEmulatorContext.TotalMemorySize()) - 1;
     }
 
-    int nAlt = 1;
-    const rc_condset_t* pCondSet = pTrigger->alternative;
-    while (pCondSet != nullptr)
+    char sErrorBuffer[256];
+    if (rc_validate_trigger(pTrigger, sErrorBuffer, sizeof(sErrorBuffer), nMaxAddress))
     {
-        if (!ValidateCondSet(pCondSet, sError))
-        {
-            sError.insert(0, L"Alt : ");
-            sError.insert(4, std::to_wstring(nAlt));
-            return false;
-        }
-
-        ++nAlt;
-        pCondSet = pCondSet->next;
+        sError.clear();
+        return true;
     }
-
-    return true;
-}
-
-bool TriggerValidation::Validate(const std::string& sTrigger, std::wstring& sError)
-{
-    const auto nSize = rc_trigger_size(sTrigger.c_str());
-    if (nSize < 0)
+    else
     {
-        sError = ra::Widen(rc_error_str(nSize));
+        sError = ra::Widen(sErrorBuffer);
         return false;
     }
-
-    std::string sTriggerBuffer;
-    sTriggerBuffer.resize(nSize);
-    const auto* pTrigger = rc_parse_trigger(sTriggerBuffer.data(), sTrigger.c_str(), nullptr, 0);
-
-    return ValidateTrigger(pTrigger, sError);
 }
 
 } // namespace models
diff --git a/src/rcheevos.vcxproj b/src/rcheevos.vcxproj
index d457b1e5..65c821db 100644
--- a/src/rcheevos.vcxproj
+++ b/src/rcheevos.vcxproj
@@ -190,6 +190,7 @@
       <PreprocessorDefinitions>_CRT_SECURE_NO_WARNINGS;%(PreprocessorDefinitions)</PreprocessorDefinitions>
     </ClCompile>
     <ClCompile Include="..\rcheevos\src\rcheevos\memref.c" />
+    <ClCompile Include="..\rcheevos\src\rcheevos\rc_validate.c" />
     <ClCompile Include="..\rcheevos\src\rcheevos\richpresence.c" />
     <ClInclude Include="..\rcheevos\src\rcheevos\rc_internal.h" />
     <ClInclude Include="..\rcheevos\src\rhash\md5.h" />
diff --git a/src/rcheevos.vcxproj.filters b/src/rcheevos.vcxproj.filters
index 199c60db..9578a802 100644
--- a/src/rcheevos.vcxproj.filters
+++ b/src/rcheevos.vcxproj.filters
@@ -130,6 +130,9 @@
     <ClCompile Include="..\rcheevos\src\rhash\md5.c">
       <Filter>rhash</Filter>
     </ClCompile>
+    <ClCompile Include="..\rcheevos\src\rcheevos\rc_validate.c">
+      <Filter>rcheevos</Filter>
+    </ClCompile>
   </ItemGroup>
   <ItemGroup>
     <Filter Include="lua">
diff --git a/src/ui/viewmodels/OverlayListPageViewModel.cpp b/src/ui/viewmodels/OverlayListPageViewModel.cpp
index 912bf394..fdfe3e21 100644
--- a/src/ui/viewmodels/OverlayListPageViewModel.cpp
+++ b/src/ui/viewmodels/OverlayListPageViewModel.cpp
@@ -177,13 +177,13 @@ void OverlayListPageViewModel::RenderList(ra::ui::drawing::ISurface& pSurface, i
             {
                 const auto nProgressBarWidth = (nWidth - nTextX - 12) * 2 / 3;
                 const auto nValue = std::min(pItem->GetProgressValue(), nTarget);
-                const auto nProgressBarFillWidth = gsl::narrow_cast<int>(((static_cast<long>(nProgressBarWidth) - 2) * nValue) / nTarget);
-                const auto nProgressBarPercent = gsl::narrow_cast<int>(static_cast<long>(nValue) * 100 / nTarget);
+                const auto nProgressBarFillWidth = gsl::narrow_cast<int>(((static_cast<long long>(nProgressBarWidth) - 2) * nValue) / nTarget);
 
                 pSurface.FillRectangle(nTextX + 12, nY + 1 + 26 + 25, nProgressBarWidth, 8, pTheme.ColorOverlayScrollBar());
                 pSurface.FillRectangle(nTextX + 12 + 2, nY + 1 + 26 + 25 + 2, nProgressBarFillWidth - 4, 8 - 4, pTheme.ColorOverlayScrollBarGripper());
 
                 const auto nProgressFont = pSurface.LoadFont(pTheme.FontOverlay(), 14, ra::ui::FontStyles::Normal);
+                const auto nProgressBarPercent = gsl::narrow_cast<int>(static_cast<long long>(nValue) * 100 / nTarget);
                 const std::wstring sProgress = pItem->IsProgressPercentage() ?
                     ra::StringPrintf(L"%d%%", nProgressBarPercent) :
                     ra::StringPrintf(L"%u/%u", nValue, nTarget);
diff --git a/src/ui/win32/AssetEditorDialog.cpp b/src/ui/win32/AssetEditorDialog.cpp
index 1a6fe47d..0251c356 100644
--- a/src/ui/win32/AssetEditorDialog.cpp
+++ b/src/ui/win32/AssetEditorDialog.cpp
@@ -657,6 +657,7 @@ AssetEditorDialog::AssetEditorDialog(AssetEditorViewModel& vmAssetEditor)
     m_bindWindow.BindVisible(IDC_RA_POINTS, AssetEditorViewModel::IsAchievementProperty);
     m_bindWindow.BindVisible(IDC_RA_CHK_AS_PERCENT, AssetEditorViewModel::IsAchievementProperty);
     m_bindWindow.BindVisible(IDC_RA_CHK_AS_PERCENT, AssetEditorViewModel::HasMeasuredProperty);
+    m_bindMeasuredAsPercent.BindCheck(TriggerViewModel::IsMeasuredTrackedAsPercentProperty);
     m_bindWindow.BindVisible(IDC_RA_LBL_BADGENAME, AssetEditorViewModel::IsAchievementProperty);
     m_bindWindow.BindVisible(IDC_RA_BADGENAME, AssetEditorViewModel::IsAchievementProperty);
     m_bindWindow.BindVisible(IDC_RA_BADGE_SPIN, AssetEditorViewModel::IsAchievementProperty);


Test patch:
diff --git a/tests/data/models/TriggerValidation_Tests.cpp b/tests/data/models/TriggerValidation_Tests.cpp
index 3018f0a9..fa6fc1c3 100644
--- a/tests/data/models/TriggerValidation_Tests.cpp
+++ b/tests/data/models/TriggerValidation_Tests.cpp
@@ -35,16 +35,16 @@ public:
 
     TEST_METHOD(TestLastConditionCombining)
     {
-        AssertValidation("0xH1234=1_A:0xH2345=2", L"Final condition type expects another condition to follow.");
-        AssertValidation("0xH1234=1_B:0xH2345=2", L"Final condition type expects another condition to follow.");
-        AssertValidation("0xH1234=1_C:0xH2345=2", L"Final condition type expects another condition to follow.");
-        AssertValidation("0xH1234=1_D:0xH2345=2", L"Final condition type expects another condition to follow.");
-        AssertValidation("0xH1234=1_N:0xH2345=2", L"Final condition type expects another condition to follow.");
-        AssertValidation("0xH1234=1_O:0xH2345=2", L"Final condition type expects another condition to follow.");
-        AssertValidation("0xH1234=1_Z:0xH2345=2", L"Final condition type expects another condition to follow.");
-
-        AssertValidation("0xH1234=1_A:0xH2345=2S0x3456=1", L"Core: Final condition type expects another condition to follow.");
-        AssertValidation("0x3456=1S0xH1234=1_A:0xH2345=2", L"Alt 1: Final condition type expects another condition to follow.");
+        AssertValidation("0xH1234=1_A:0xH2345=2", L"Final condition type expects another condition to follow");
+        AssertValidation("0xH1234=1_B:0xH2345=2", L"Final condition type expects another condition to follow");
+        AssertValidation("0xH1234=1_C:0xH2345=2", L"Final condition type expects another condition to follow");
+        AssertValidation("0xH1234=1_D:0xH2345=2", L"Final condition type expects another condition to follow");
+        AssertValidation("0xH1234=1_N:0xH2345=2", L"Final condition type expects another condition to follow");
+        AssertValidation("0xH1234=1_O:0xH2345=2", L"Final condition type expects another condition to follow");
+        AssertValidation("0xH1234=1_Z:0xH2345=2", L"Final condition type expects another condition to follow");
+
+        AssertValidation("0xH1234=1_A:0xH2345=2S0x3456=1", L"Core Final condition type expects another condition to follow");
+        AssertValidation("0x3456=1S0xH1234=1_A:0xH2345=2", L"Alt1 Final condition type expects another condition to follow");
     }
 
     TEST_METHOD(TestMiddleConditionCombining)
@@ -58,8 +58,8 @@ public:
 
     TEST_METHOD(TestAddHitsChainWithoutTarget)
     {
-        AssertValidation("C:0xH1234=1_0xH2345=2", L"Condition 2: Final condition in AddHits chain must have a hit target.");
-        AssertValidation("D:0xH1234=1_0xH2345=2", L"Condition 2: Final condition in AddHits chain must have a hit target.");
+        AssertValidation("C:0xH1234=1_0xH2345=2", L"Condition 2: Final condition in AddHits chain must have a hit target");
+        AssertValidation("D:0xH1234=1_0xH2345=2", L"Condition 2: Final condition in AddHits chain must have a hit target");
         AssertValidation("C:0xH1234=1_0xH2345=2.1.", L"");
         AssertValidation("D:0xH1234=1_0xH2345=2.1.", L"");
     }
@@ -128,9 +128,9 @@ public:
 
         // basic checks for each side
         AssertValidation("0xH1234>0xH1235", L"");
-        AssertValidation("0xH12345>0xH1235", L"Condition 1: Address out of range (max FFFF).");
-        AssertValidation("0xH1234>0xH12345", L"Condition 1: Address out of range (max FFFF).");
-        AssertValidation("0xH12345>0xH12345", L"Condition 1: Address out of range (max FFFF).");
+        AssertValidation("0xH12345>0xH1235", L"Condition 1: Address 12345 out of range (max FFFF)");
+        AssertValidation("0xH1234>0xH12345", L"Condition 1: Address 12345 out of range (max FFFF)");
+        AssertValidation("0xH12345>0xH12345", L"Condition 1: Address 12345 out of range (max FFFF)");
         AssertValidation("0xX1234>h12345", L"");
 
         // support for multiple memory blocks and edge addresses
@@ -139,11 +139,11 @@ public:
         AssertValidation("0xH12345>0xH1235", L"");
         AssertValidation("0xH0000>5", L"");
         AssertValidation("0xH1FFFF>5", L"");
-        AssertValidation("0xH20000>5", L"Condition 1: Address out of range (max 1FFFF).");
+        AssertValidation("0xH20000>5", L"Condition 1: Address 20000 out of range (max 1FFFF)");
 
         // AddAddress can use really big values for negative offsets, don't flag them.
         AssertValidation("I:0xX1234_0xHFFFFFF00>5", L"");
-        AssertValidation("I:0xX1234_0xH1234>5_0xHFFFFFF00>5", L"Condition 3: Address out of range (max 1FFFF).");
+        AssertValidation("I:0xX1234_0xH1234>5_0xHFFFFFF00>5", L"Condition 3: Address FFFFFF00 out of range (max 1FFFF)");
     }
 };
 
diff --git a/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp b/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp
index 91356d83..c195bf91 100644
--- a/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp
@@ -442,7 +442,7 @@ public:
         Assert::AreEqual(std::wstring(), editor.GetAssetValidationError());
 
         Assert::IsTrue(editor.HasAssetValidationWarning());
-        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow."), editor.GetAssetValidationWarning());
+        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow"), editor.GetAssetValidationWarning());
 
         editor.LoadAsset(nullptr);
         Assert::IsFalse(editor.HasAssetValidationError());
@@ -619,7 +619,7 @@ public:
 
         achievement.UpdateLocalCheckpoint();
         Assert::IsTrue(editor.HasAssetValidationWarning());
-        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow."), editor.GetAssetValidationWarning());
+        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow"), editor.GetAssetValidationWarning());
     }
 
     TEST_METHOD(TestSyncState)
diff --git a/tests/ui/viewmodels/AssetListViewModel_Tests.cpp b/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
index a9908f8e..76c63dfc 100644
--- a/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
@@ -1047,7 +1047,7 @@ public:
         vmAchievement.SetTrigger("A:0x1234");
         vmAchievement.UpdateLocalCheckpoint();
         Assert::IsFalse(vmAchievement.Validate());
-        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow."), pItem->GetWarning());
+        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow"), pItem->GetWarning());
     }
 
     TEST_METHOD(TestSyncAddItem)
@@ -2002,7 +2002,7 @@ public:
         const auto& sText = vmAssetList.GetUserFile(L"22");
         AssertContains(sText, "1:\"A:0x1234\":Test1b:Desc1:::::5:::::12345");
 
-        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow."), pItem->GetValidationError());
+        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow"), pItem->GetValidationError());
 
         pItem->SetName(L"Test1c");
         vmAssetList.SaveSelected();
@@ -2139,7 +2139,7 @@ public:
         pItem->SetTrigger("A:0x1234");
         pItem->UpdateLocalCheckpoint();
         Assert::AreEqual(AssetChanges::Unpublished, pItem->GetChanges());
-        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow."), pItem->GetValidationError());
+        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow"), pItem->GetValidationError());
 
         vmAssetList.FilteredAssets().GetItemAt(0)->SetSelected(true);
         vmAssetList.ForceUpdateButtons();
@@ -2151,7 +2151,7 @@ public:
         Assert::AreEqual({ 1U }, vmAssetList.PublishedAssets.size());
         Assert::IsTrue(pItem == dynamic_cast<ra::data::models::AchievementModel*>(vmAssetList.PublishedAssets.at(0)));
         Assert::AreEqual(AssetChanges::None, pItem->GetChanges());
-        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow."), pItem->GetValidationError());
+        Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow"), pItem->GetValidationError());
 
         vmAssetList.ForceUpdateButtons();
         vmAssetList.AssertButtonState(SaveButtonState::Demote);