From 3ac88d4eac37eb4ec316ed20e2de28f1610493e8 Mon Sep 17 00:00:00 2001 From: Alexander Gutkin Date: Wed, 7 Oct 2026 14:38:59 -0700 Subject: [PATCH] Cap `ConstFstImpl::kMaxStates` by `StateId` and validate header counts before narrowing. In `ConstFstImpl` (`lib/const-fst.h`): 1. `nstates_` and `start_` have type `StateId` (`int32_t` for `StdArc`, max `2^31 - 1`), whereas `kMaxStates` was `1ULL << 52`. 2. In `ConstFstImpl::Read`, the 64-bit `hdr.NumStates()`, `hdr.Start()`, and `hdr.NumArcs()` values were assigned to `impl->nstates_` (`StateId`), `impl->start_` (`StateId`), and `impl->narcs_` (`size_t`) before checking bounds. As a result, a 64-bit `hdr.NumStates()` or `hdr.Start()` such as `(1LL << 32) + 1` or `1LL << 32` silently truncated to `1` or `0` in 32-bit `StateId`, bypassing the range checks (`impl->nstates_ > kMaxStates` was tautologically false for 32-bit `StateId`), and `impl->narcs_ < 0` was an unsigned comparison. Fix by: - Making `kMaxStates` and `kMaxArcs` public on `ConstFstImpl` and capping `kMaxStates` at `std::min(0x10000000000000ull, std::numeric_limits::max())`. - Counting states in `uint64_t` in `ConstFstImpl(const Fst&)` and checking `nstates > kMaxStates` before narrowing to `nstates_`. - Validating `hdr.NumStates()`, `hdr.Start()`, and `hdr.NumArcs()` as `int64_t` in `ConstFstImpl::Read` before assigning them to `impl->nstates_`, `impl->start_`, and `impl->narcs_`. - Expanding `ConstLimitsTest` in `fst_limits_test.cc` to test 64-bit values that truncate into 32-bit `StateId` against non-empty state streams. PiperOrigin-RevId: 995363968 --- openfst/lib/const-fst.h | 52 ++++++++++++++-------- openfst/test/fst_limits_test.cc | 78 +++++++++++++++++---------------- 2 files changed, 74 insertions(+), 56 deletions(-) diff --git a/openfst/lib/const-fst.h b/openfst/lib/const-fst.h index 2896c15c..1e746aaf 100644 --- a/openfst/lib/const-fst.h +++ b/openfst/lib/const-fst.h @@ -110,6 +110,12 @@ class ConstFstImpl : public FstImpl { data->ref_count = nullptr; } + // Maximum number of arcs during read. + static constexpr uint64_t kMaxArcs = 0x10000000000000ull; + // Maximum number of states. + static constexpr uint64_t kMaxStates = std::min( + 0x10000000000000ull, std::numeric_limits::max()); + private: // Used to find narcs_ and nstates_ in Write. friend class ConstFst; @@ -134,10 +140,6 @@ class ConstFstImpl : public FstImpl { static constexpr int kAlignedFileVersion = 1; // Minimum file format version supported. static constexpr int kMinFileVersion = 1; - // Maximum number of arcs during read. - static constexpr uint64_t kMaxArcs = 0x10000000000000ull; - // Maximum number of states during read. - static constexpr uint64_t kMaxStates = 0x10000000000000ull; std::unique_ptr states_region_; // Mapped file for states. std::unique_ptr arcs_region_; // Mapped file for arcs. @@ -162,10 +164,21 @@ ConstFstImpl::ConstFstImpl(const Fst& fst) { SetOutputSymbols(fst.OutputSymbols()); start_ = fst.Start(); // Counts states and arcs. + uint64_t nstates = 0; for (StateIterator> siter(fst); !siter.Done(); siter.Next()) { - ++nstates_; + ++nstates; narcs_ += fst.NumArcs(siter.Value()); } + if (nstates > kMaxStates) { + FSTERROR() << "ConstFst: Number of states " << nstates + << " exceeds maximum " << kMaxStates << " for state ID type"; + SetProperties(kError, kError); + start_ = kNoStateId; + nstates_ = 0; + narcs_ = 0; + return; + } + nstates_ = nstates; states_region_.reset(MappedFile::AllocateType(nstates_)); arcs_region_.reset(MappedFile::AllocateType(narcs_)); states_ = static_cast(states_region_->mutable_data()); @@ -201,19 +214,27 @@ ConstFstImpl* ConstFstImpl::Read( auto impl = std::make_unique(); FstHeader hdr; if (!impl->ReadHeader(strm, opts, kMinFileVersion, &hdr)) return nullptr; - impl->start_ = hdr.Start(); + if (hdr.NumStates() < 0 || hdr.NumStates() > kMaxStates) { + LOG(ERROR) << "ConstFst::Read: Invalid number of states: " + << hdr.NumStates() << " > " << kMaxStates << " for " + << opts.source; + return nullptr; + } impl->nstates_ = hdr.NumStates(); - if (impl->nstates_ < 0 || impl->nstates_ > kMaxStates) { - LOG(ERROR) << "ConstFst::Read: Invalid number of states: " << impl->nstates_ - << " > " << kMaxStates << " for " << opts.source; + if (hdr.Start() != kNoStateId && + (hdr.Start() < 0 || hdr.Start() >= impl->nstates_)) { + LOG(ERROR) << "ConstFst::Read: Invalid start state " << hdr.Start() + << " for FST with " << impl->nstates_ + << " states: " << opts.source; return nullptr; } - impl->narcs_ = hdr.NumArcs(); - if (impl->narcs_ < 0 || impl->narcs_ > kMaxArcs) { - LOG(ERROR) << "ConstFst::Read: Invalid number of arcs: " << impl->narcs_ + impl->start_ = hdr.Start(); + if (hdr.NumArcs() < 0 || hdr.NumArcs() > kMaxArcs) { + LOG(ERROR) << "ConstFst::Read: Invalid number of arcs: " << hdr.NumArcs() << " > " << kMaxArcs << " for " << opts.source; return nullptr; } + impl->narcs_ = hdr.NumArcs(); // Ensures compatibility. if (hdr.Version() == kAlignedFileVersion) { hdr.SetFlags(hdr.GetFlags() | FstHeader::IS_ALIGNED); @@ -245,13 +266,6 @@ ConstFstImpl* ConstFstImpl::Read( impl->arcs_ = static_cast(impl->arcs_region_->mutable_data()); // Check states. - if (impl->start_ != kNoStateId && - !(impl->start_ >= 0 && impl->start_ < impl->nstates_)) { - LOG(ERROR) << "ConstFst::Read: Invalid start state " << impl->start_ - << " for FST with " << impl->nstates_ - << " states: " << opts.source; - return nullptr; - } for (StateId s = 0; s < impl->nstates_; ++s) { if (impl->states_[s].pos > impl->narcs_ || impl->states_[s].narcs > impl->narcs_ - impl->states_[s].pos) { diff --git a/openfst/test/fst_limits_test.cc b/openfst/test/fst_limits_test.cc index 5518d628..af212052 100644 --- a/openfst/test/fst_limits_test.cc +++ b/openfst/test/fst_limits_test.cc @@ -158,57 +158,61 @@ TEST(CompactLimitsTest, VariableSizeNonMonotonicOffsetsTest) { // ConstFst limits tests. -TEST(ConstLimitsTest, MaxStatesTest) { +using StdConstFstImpl = internal::ConstFstImpl; + +FstHeader MakeConstHeader(int64_t nstates, int64_t narcs, int64_t start) { FstHeader hdr; hdr.SetFstType("const"); hdr.SetArcType(StdArc::Type()); hdr.SetVersion(2); - // kMaxStates for ConstFst is 1LL << 54. - hdr.SetNumStates((1LL << 54) + 1); - hdr.SetNumArcs(0); - hdr.SetStart(0); + hdr.SetNumStates(nstates); + hdr.SetNumArcs(narcs); + hdr.SetStart(start); + return hdr; +} - std::istringstream iss; - FstReadOptions opts; - opts.header = &hdr; +TEST(ConstLimitsTest, MaxStatesTest) { + // Include 1 valid ConstState (20 bytes) in the stream so that a 64-bit count + // like (1LL << 32) + 1 that truncates to 1 when narrowed to int32_t StateId + // would succeed if not validated before narrowing. + const std::string one_state_data(20, '\0'); + for (const int64_t nstates : + {static_cast(StdConstFstImpl::kMaxStates + 1), + (int64_t{1} << 32) + 1, (int64_t{1} << 54) + 1, int64_t{-1}}) { + FstHeader hdr = MakeConstHeader(nstates, /*narcs=*/0, /*start=*/0); + std::istringstream iss(one_state_data); + FstReadOptions opts; + opts.header = &hdr; - auto* impl = internal::ConstFstImpl::Read(iss, opts); - EXPECT_EQ(impl, nullptr); + EXPECT_EQ(StdConstFstImpl::Read(iss, opts), nullptr); + } } TEST(ConstLimitsTest, MaxArcsTest) { - FstHeader hdr; - hdr.SetFstType("const"); - hdr.SetArcType(StdArc::Type()); - hdr.SetVersion(2); - hdr.SetNumStates(10); - // kMaxArcs for ConstFst is 1LL << 54. - hdr.SetNumArcs((1LL << 54) + 1); - hdr.SetStart(0); - - std::istringstream iss; - FstReadOptions opts; - opts.header = &hdr; + for (const int64_t narcs : + {static_cast(StdConstFstImpl::kMaxArcs + 1), int64_t{-1}}) { + FstHeader hdr = MakeConstHeader(/*nstates=*/10, narcs, /*start=*/0); + std::istringstream iss; + FstReadOptions opts; + opts.header = &hdr; - auto* impl = internal::ConstFstImpl::Read(iss, opts); - EXPECT_EQ(impl, nullptr); + EXPECT_EQ(StdConstFstImpl::Read(iss, opts), nullptr); + } } TEST(ConstLimitsTest, StartStateOutOfRangeTest) { - FstHeader hdr; - hdr.SetFstType("const"); - hdr.SetArcType(StdArc::Type()); - hdr.SetVersion(2); - hdr.SetNumStates(10); - hdr.SetNumArcs(0); - hdr.SetStart(10); - - std::istringstream iss; - FstReadOptions opts; - opts.header = &hdr; + // Include 1 valid ConstState (20 bytes) in the stream so that a 64-bit start + // state like (1LL << 32) that truncates to 0 when narrowed to int32_t StateId + // would succeed on a 1-state FST if not validated before narrowing. + const std::string one_state_data(20, '\0'); + for (const int64_t start : {int64_t{1}, int64_t{-2}, int64_t{1} << 32}) { + FstHeader hdr = MakeConstHeader(/*nstates=*/1, /*narcs=*/0, start); + std::istringstream iss(one_state_data); + FstReadOptions opts; + opts.header = &hdr; - auto* impl = internal::ConstFstImpl::Read(iss, opts); - EXPECT_EQ(impl, nullptr); + EXPECT_EQ(StdConstFstImpl::Read(iss, opts), nullptr); + } } // VectorFst limits tests.