From 7cd13b7743f8f4cff5f768203e032e67a50022db Mon Sep 17 00:00:00 2001 From: Alexander Gutkin Date: Thu, 22 Oct 2026 15:55:55 -0700 Subject: [PATCH] Detect phi cycles in PhiMatcher::Find and PhiMatcher::Final. `PhiMatcher::Find` and `PhiMatcher::Final` followed phi transitions until a match or final weight was found, checking only for a single-state phi self-loop (`nextstate == s`). A phi cycle of two or more states (or a phi self-loop in `Find` when `phi_loop` is `false`) caused an infinite loop in both fatal and non-fatal error modes. Detect phi cycles in `PhiMatcher::Find` and `PhiMatcher::Final` using Brent's O(1)-space cycle detection algorithm (Richard P. Brent, "An improved Monte Carlo factorization algorithm," *BIT Numerical Mathematics*, 20(2):176-184, 1980), report `FSTERROR()` and set `error_ = true` when a phi cycle is encountered, and stop following phi self-loops in `Find` when `phi_loop_` is `false`. PiperOrigin-RevId: 1005116724 --- openfst/lib/matcher.h | 61 +++++++++++++++++++++----- openfst/test/BUILD.bazel | 3 ++ openfst/test/matcher_test.cc | 84 ++++++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 10 deletions(-) diff --git a/openfst/lib/matcher.h b/openfst/lib/matcher.h index 01c060d0..cf0f505b 100644 --- a/openfst/lib/matcher.h +++ b/openfst/lib/matcher.h @@ -613,8 +613,9 @@ enum MatcherRewriteMode { // underlying matching. By default, the underlying matcher is // constructed by PhiMatcher. The user can instead pass in this // object; in that case, PhiMatcher takes its ownership. -// Phi non-determinism not supported. No non-consuming symbols other -// than epsilon supported with the underlying template argument matcher. +// Phi non-determinism and non-trivial phi cycles are not supported. No +// non-consuming symbols other than epsilon supported with the underlying +// template argument matcher. template class PhiMatcher : public MatcherBase { public: @@ -720,13 +721,31 @@ class PhiMatcher : public MatcherBase { } weight = Weight::One(); matcher_->SetState(s); + // For cycle detection uses Brent's O(1)-space algorithm. See: + // + // Brent, R. P. 1980. An improved Monte Carlo factorization algorithm. + // BIT Numerical Mathematics 20(2): 176-184. + StateId checkpoint = s; + size_t power = 1; + size_t length = 0; while (matcher_->Final(s) == Weight::Zero()) { if (!matcher_->Find(phi_label_ == 0 ? -1 : phi_label_)) break; weight = Times(weight, matcher_->Value().weight); - if (s == matcher_->Value().nextstate) { - return Weight::Zero(); // Does not follow phi self-loops. + const auto nextstate = matcher_->Value().nextstate; + // Does not follow phi self-loops. + if (s == nextstate) return Weight::Zero(); + if (nextstate == checkpoint) { + FSTERROR() << "PhiMatcher: Phi cycle detected"; + error_ = true; + return Weight::Zero(); + } + ++length; + if (length == power) { + checkpoint = nextstate; + power *= 2; + length = 0; } - s = matcher_->Value().nextstate; + s = nextstate; matcher_->SetState(s); } weight = Times(weight, matcher_->Final(s)); @@ -768,7 +787,7 @@ class PhiMatcher : public MatcherBase { Weight phi_weight_; // Product of the weights of phi transitions taken. bool phi_loop_; // When true, phi self-loop are allowed and treated // as rho (required for Aho-Corasick). - bool error_; // Error encountered? + mutable bool error_; // Error encountered? PhiMatcher& operator=(const PhiMatcher&) = delete; }; @@ -801,21 +820,43 @@ inline bool PhiMatcher::Find(Label label) { return matcher_->Find(label); } auto s = state_; + // For cycle detection uses Brent's O(1)-space algorithm. See: + // + // Brent, R. P. 1980. An improved Monte Carlo factorization algorithm. + // BIT Numerical Mathematics 20(2): 176-184. + StateId checkpoint = s; + size_t power = 1; + size_t length = 0; while (!matcher_->Find(label)) { // Look for phi transition (if phi_label_ == 0, we need to look // for -1 to avoid getting the virtual self-loop) if (!matcher_->Find(phi_label_ == 0 ? -1 : phi_label_)) return false; - if (phi_loop_ && matcher_->Value().nextstate == s) { - phi_match_ = label; - return true; + const auto nextstate = matcher_->Value().nextstate; + if (nextstate == s) { + if (phi_loop_) { + phi_match_ = label; + return true; + } + return false; // Does not follow phi self-loops. } phi_weight_ = Times(phi_weight_, matcher_->Value().weight); - s = matcher_->Value().nextstate; matcher_->Next(); if (!matcher_->Done()) { FSTERROR() << "PhiMatcher: Phi non-determinism not supported"; error_ = true; } + if (nextstate == checkpoint) { + FSTERROR() << "PhiMatcher: Phi cycle detected"; + error_ = true; + return false; + } + ++length; + if (length == power) { + checkpoint = nextstate; + power *= 2; + length = 0; + } + s = nextstate; matcher_->SetState(s); } return true; diff --git a/openfst/test/BUILD.bazel b/openfst/test/BUILD.bazel index 55dc7fc6..c97dab23 100644 --- a/openfst/test/BUILD.bazel +++ b/openfst/test/BUILD.bazel @@ -1485,6 +1485,9 @@ cc_test( ":test_main", "//openfst/compat:file_path", "//openfst/lib", + "//openfst/lib:util", + "@com_google_absl//absl/flags:flag", + "@com_google_absl//absl/flags:reflection", "@com_google_googletest//:gtest", ], ) diff --git a/openfst/test/matcher_test.cc b/openfst/test/matcher_test.cc index df86406f..e77a3d58 100644 --- a/openfst/test/matcher_test.cc +++ b/openfst/test/matcher_test.cc @@ -26,6 +26,8 @@ #include "openfst/compat/file_path.h" #include "gtest/gtest.h" +#include "absl/flags/flag.h" +#include "absl/flags/reflection.h" #include "openfst/lib/arc.h" #include "openfst/lib/arcsort.h" #include "openfst/lib/cache.h" @@ -33,7 +35,9 @@ #include "openfst/lib/compose.h" #include "openfst/lib/equal.h" #include "openfst/lib/fst.h" +#include "openfst/lib/properties.h" #include "openfst/lib/relabel.h" +#include "openfst/lib/util.h" #include "openfst/lib/vector-fst.h" #include "openfst/lib/verify.h" @@ -183,6 +187,9 @@ class MatcherTest : public testing::Test { std::unique_ptr> lm_a_fst_; std::unique_ptr> lm_bb_fst_; std::unique_ptr> lm_a_bb_fst_; + + private: + const absl::FlagSaver fs_; }; TEST_F(MatcherTest, ComposeWithRhoMatcher) { @@ -409,6 +416,83 @@ TEST_F(MatcherTest, PhiMatcherFinal) { } } +TEST_F(MatcherTest, PhiMatcherCycle) { + absl::SetFlag(&FLAGS_fst_error_fatal, false); + + // States 0 and 1 form a 2-state phi cycle (0 -phi-> 1 -phi-> 0); state 1 + // also has a matching transition on label 1 to final state 2. + StdVectorFst cyclic_fst; + const auto s0 = cyclic_fst.AddState(); + const auto s1 = cyclic_fst.AddState(); + const auto s2 = cyclic_fst.AddState(); + cyclic_fst.SetStart(s0); + cyclic_fst.AddArc(s0, Arc(kSpecialLabel, kSpecialLabel, Weight(0.5), s1)); + cyclic_fst.AddArc(s1, Arc(kSpecialLabel, kSpecialLabel, Weight(0.25), s0)); + cyclic_fst.AddArc(s1, Arc(1, 1, Weight(1.0), s2)); + cyclic_fst.SetFinal(s2, Weight::One()); + ArcSort(&cyclic_fst, StdILabelCompare()); + + // Matching label 1 from state 0 succeeds after one phi step without hitting + // the cycle. + { + TestPhiMatcher matcher(cyclic_fst, MATCH_INPUT, kSpecialLabel); + matcher.SetState(s0); + ASSERT_TRUE(matcher.Find(1)); + EXPECT_EQ(matcher.Value().nextstate, s2); + EXPECT_EQ(matcher.Value().weight, Weight(1.5)); + EXPECT_EQ(matcher.Properties(0) & kError, 0); + } + + // Searching for an unmatched label (2) detects the 0 -> 1 -> 0 phi cycle, + // returns false, and sets kError. + { + TestPhiMatcher matcher(cyclic_fst, MATCH_INPUT, kSpecialLabel); + matcher.SetState(s0); + EXPECT_FALSE(matcher.Find(2)); + EXPECT_EQ(matcher.Properties(0) & kError, kError); + } + + // Final(s0) when neither s0 nor s1 is final detects the 0 -> 1 -> 0 phi + // cycle, returns Zero(), and sets kError. + { + TestPhiMatcher matcher(cyclic_fst, MATCH_INPUT, kSpecialLabel); + EXPECT_EQ(matcher.Final(s0), Weight::Zero()); + EXPECT_EQ(matcher.Properties(0) & kError, kError); + } + + // Composing with an FST that triggers the phi cycle terminates and sets + // kError on the result. + { + StdVectorFst query; + const auto q0 = query.AddState(); + const auto q1 = query.AddState(); + query.SetStart(q0); + query.AddArc(q0, Arc(2, 2, Weight::One(), q1)); + query.SetFinal(q1, Weight::One()); + + ComposeFst cfst( + query, cyclic_fst, + ComposeFstOptions( + CacheOptions(), new TestPhiMatcher(query, MATCH_NONE, kNoLabel), + new TestPhiMatcher(cyclic_fst, MATCH_INPUT, kSpecialLabel))); + StdVectorFst composed(cfst); + EXPECT_TRUE(composed.Properties(kError, false)); + } + + // A phi self-loop with phi_loop=false does not loop forever in Find(). + { + StdVectorFst self_loop_fst; + const auto s = self_loop_fst.AddState(); + self_loop_fst.SetStart(s); + self_loop_fst.AddArc(s, Arc(kSpecialLabel, kSpecialLabel, Weight(0.5), s)); + TestPhiMatcher matcher(self_loop_fst, MATCH_INPUT, kSpecialLabel, + /*phi_loop=*/false); + matcher.SetState(s); + EXPECT_FALSE(matcher.Find(2)); + EXPECT_EQ(matcher.Properties(0) & kError, 0); + } +} + TEST_F(MatcherTest, SortedMatcher) { SortedMatcher> sorted_matcher(*mfst2_, MATCH_INPUT); sorted_matcher.SetState(0);