Fixes

Author: SirTyson

src/catchup/CatchupWork.cpp

@@ -27,6 +27,7 @@
 #include "work/WorkWithCallback.h"
 #include <Tracy.hpp>
 #include <fmt/format.h>
+#include <limits>
 
 namespace stellar
 {
@@ -407,6 +408,21 @@ CatchupWork::runCatchupStep()
     // HAS is fetched and validated at this point
     releaseAssert(mHAS->currentLedger > LedgerManager::GENESIS_LEDGER_SEQ);
 
+    // Reject HAS whose currentLedger is so large that checkpoint arithmetic
+    // would overflow. checkpointContainingLedger saturates to UINT32_MAX on
+    // overflow; treat that as failure here so downstream code never sees a
+    // wrapped/bogus ledger.
+    if (HistoryManager::checkpointContainingLedger(mHAS->currentLedger,
+                                                   mApp.getConfig()) ==
+        std::numeric_limits<uint32_t>::max())
+    {
+        CLOG_ERROR(History,
+                   "HAS currentLedger {} is too large for checkpoint "
+                   "arithmetic",
+                   mHAS->currentLedger);
+        return State::WORK_FAILURE;
+    }
+
     auto resolvedConfiguration =
         mCatchupConfiguration.resolve(mHAS->currentLedger);
     auto catchupRange =

src/history/HistoryArchive.cpp

@@ -20,10 +20,10 @@
 #include "util/GlobalChecks.h"
 #include "util/Logging.h"
 #include "util/ProtocolVersion.h"
+#include "util/types.h"
 #include <Tracy.hpp>
 #include <fmt/format.h>
 
-#include <cctype>
 #include <cereal/archives/json.hpp>
 #include <cereal/cereal.hpp>
 #include <cereal/types/vector.hpp>
@@ -159,7 +159,7 @@ isValidHexHash(std::string const& s)
     }
     for (unsigned char c : s)
     {
-        if (!std::isxdigit(c))
+        if (!isAsciiHexDigit(c))
         {
             return false;
         }
@@ -195,13 +195,6 @@ validateHASAfterDeserialization(HistoryArchiveState const& has)
                         has.hotArchiveBuckets.size()));
     }
 
-    // Prevent integer overflow in downstream CheckpointRange calculations
-    if (has.currentLedger > HistoryArchiveState::MAX_CURRENT_LEDGER)
-    {
-        throw std::runtime_error(fmt::format(
-            FMT_STRING("currentLedger {} is too large"), has.currentLedger));
-    }
-
     // Validate all bucket hash strings are well-formed 64-character hex
     auto validateHashesInBuckets = [](auto const& buckets,
                                       std::string const& name) {

src/history/HistoryArchive.h

@@ -88,14 +88,6 @@ struct HistoryArchiveState
     // 50KB; 10MB is extremely generous.
     static constexpr size_t MAX_HAS_FILE_SIZE = 10 * 1024 * 1024; // 10 MB
 
-    // Upper bound on currentLedger to prevent uint32_t overflow in
-    // downstream arithmetic. Callers round currentLedger up to the next
-    // checkpoint boundary and then step one checkpoint further, so we add up to
-    // 128 to this number. We'll never hit this in practice, so use 256 for a
-    // little wiggle room so we don't overflow.
-    static constexpr uint32_t MAX_CURRENT_LEDGER =
-        std::numeric_limits<uint32_t>::max() - 256;
-
     enum Version : unsigned
     {
         HISTORY_ARCHIVE_STATE_VERSION_BEFORE_HOT_ARCHIVE = 1,

src/history/HistoryManager.h

@@ -9,6 +9,7 @@
 #include "overlay/StellarXDR.h"
 #include "util/GlobalChecks.h"
 #include <functional>
+#include <limits>
 #include <memory>
 
 /**
@@ -222,14 +223,24 @@ class HistoryManager
     // Return checkpoint that contains given ledger. Checkpoint is identified
     // by last ledger in range. This does not consult the network nor take
     // account of manual checkpoints.
+    //
+    // Saturates to UINT32_MAX when rounding up to the next checkpoint would
+    // overflow uint32_t. Callers that take untrusted ledger numbers (e.g.
+    // HAS-derived) must treat a UINT32_MAX result as failure.
     static uint32_t
     checkpointContainingLedger(uint32_t ledger, Config const& cfg)
     {
         uint32_t freq = getCheckpointFrequency(cfg);
         // Round-up to next multiple of freq, then subtract 1 since checkpoints
         // are numbered for (and cover ledgers up to) the last ledger in them,
-        // which is one-before the next multiple of freq.
-        return (((ledger / freq) + 1) * freq) - 1;
+        // which is one-before the next multiple of freq. Done in 64-bit to
+        // avoid wraparound near UINT32_MAX.
+        uint64_t next = (static_cast<uint64_t>(ledger) / freq + 1) * freq;
+        if (next > std::numeric_limits<uint32_t>::max())
+        {
+            return std::numeric_limits<uint32_t>::max();
+        }
+        return static_cast<uint32_t>(next) - 1;
     }
 
     // Return true iff closing `ledger` should cause publishing a checkpoint.

src/history/test/HistoryArchiveFormatTests.cpp

@@ -165,29 +165,6 @@ TEST_CASE("HAS rejects invalid version", "[history][archive-format]")
     }
 }
 
-TEST_CASE("HAS rejects extreme currentLedger", "[history][archive-format]")
-{
-    // UINT32_MAX
-    {
-        auto json = makeHASJson(std::numeric_limits<uint32_t>::max());
-        HistoryArchiveState has;
-        REQUIRE_THROWS_AS(has.fromString(json), std::runtime_error);
-    }
-
-    // Just above MAX_CURRENT_LEDGER
-    {
-        auto json = makeHASJson(HistoryArchiveState::MAX_CURRENT_LEDGER + 1);
-        HistoryArchiveState has;
-        REQUIRE_THROWS_AS(has.fromString(json), std::runtime_error);
-    }
-
-    // Exactly at MAX_CURRENT_LEDGER (should be accepted)
-    {
-        auto json = makeHASJson(HistoryArchiveState::MAX_CURRENT_LEDGER);
-        HistoryArchiveState has;
-        REQUIRE_NOTHROW(has.fromString(json));
-    }
-}
 TEST_CASE("HAS rejects wrong-sized bucket vectors", "[history][archive-format]")
 {
     // too many currentBuckets

src/history/test/HistoryTests.cpp

@@ -105,6 +105,15 @@ TEST_CASE("checkpoint containing ledger", "[history]")
           0x13f);
     CHECK(HistoryManager::checkpointContainingLedger(258, app->getConfig()) ==
           0x13f);
+
+    // Saturates at UINT32_MAX rather than silently wrapping.
+    uint32_t const MAX = std::numeric_limits<uint32_t>::max();
+    CHECK(HistoryManager::checkpointContainingLedger(MAX, app->getConfig()) ==
+          MAX);
+    CHECK(HistoryManager::checkpointContainingLedger(MAX - 63,
+                                                     app->getConfig()) == MAX);
+    CHECK(HistoryManager::checkpointContainingLedger(
+              MAX - 64, app->getConfig()) == MAX - 64);
 }
 
 TEST_CASE("HistoryManager compress", "[history]")

src/util/types.h

@@ -193,6 +193,14 @@ isAsciiAlphaNumeric(char c)
     return false;
 }
 
+inline bool
+isAsciiHexDigit(char c)
+{
+    unsigned char uc = static_cast<unsigned char>(c);
+    return ('0' <= uc && uc <= '9') || ('a' <= uc && uc <= 'f') ||
+           ('A' <= uc && uc <= 'F');
+}
+
 inline bool
 isAsciiNonControl(char c)
 {