From 4a3f2592b113d296f479d23fe57f221a499b32e6 Mon Sep 17 00:00:00 2001 From: Ed Savage Date: Thu, 6 Aug 2026 13:24:07 +1200 Subject: [PATCH 1/4] [ML] Fail gracefully when restoring a categorizer with an out-of-range token ID An inconsistent or truncated categorizer state document can leave a restored category referencing a token ID at or beyond the end of the restored token ID lookup. That ID was later used to index the token ID lookup unchecked (for example when building a reverse search), which is an out-of-bounds access that can crash the autodetect process with a SIGSEGV inside libc rather than failing the restore. Validate, at the end of CTokenListDataCategorizerBase::acceptRestoreTraverser, that every token ID referenced by a restored category exists in the restored token ID lookup, and fail the restore gracefully if not. This is consistent with the graceful invalid-state handling added in #2895/#2898. Relates to elastic/ml-cpp#2875 Co-authored-by: Cursor --- lib/model/CTokenListDataCategorizerBase.cc | 24 +++++++++ .../unittest/CTokenListDataCategorizerTest.cc | 53 +++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/lib/model/CTokenListDataCategorizerBase.cc b/lib/model/CTokenListDataCategorizerBase.cc index 3482742ca5..a4b0c8b828 100644 --- a/lib/model/CTokenListDataCategorizerBase.cc +++ b/lib/model/CTokenListDataCategorizerBase.cc @@ -378,6 +378,30 @@ bool CTokenListDataCategorizerBase::acceptRestoreTraverser(core::CStateRestoreTr } } while (traverser.next()); + // Validate that every token ID referenced by a restored category exists in + // the restored token ID lookup. An inconsistent or truncated state document + // can leave a category referencing a token ID at or beyond the end of + // m_TokenIdLookup. Such an ID is later used to index m_TokenIdLookup + // unchecked (for example when building a reverse search), which is an + // out-of-bounds access resulting in a crash. Fail the restore gracefully + // here instead of proceeding with an inconsistent state. + const std::size_t numTokens{m_TokenIdLookup.size()}; + const auto tokenIdsInRange = [numTokens](const TSizeSizePrVec& tokenIds) { + return std::all_of(tokenIds.begin(), tokenIds.end(), + [numTokens](const TSizeSizePr& tokenId) { + return tokenId.first < numTokens; + }); + }; + for (const auto& category : m_Categories) { + if (tokenIdsInRange(category.baseTokenIds()) == false || + tokenIdsInRange(category.commonUniqueTokenIds()) == false) { + LOG_ERROR(<< "Cannot restore categorizer - a category references a token ID " + "outside the restored token ID lookup (size " + << numTokens << "); the state document is inconsistent"); + return false; + } + } + // Categories are persisted in order of creation, but this list needs to be // sorted by descending count instead std::stable_sort(m_CategoriesByCount.begin(), m_CategoriesByCount.end(), diff --git a/lib/model/unittest/CTokenListDataCategorizerTest.cc b/lib/model/unittest/CTokenListDataCategorizerTest.cc index be4e82a83d..4dea85f218 100644 --- a/lib/model/unittest/CTokenListDataCategorizerTest.cc +++ b/lib/model/unittest/CTokenListDataCategorizerTest.cc @@ -541,6 +541,59 @@ BOOST_FIXTURE_TEST_CASE(testPersist, CTestFixture) { checkMemoryUsageInstrumentation(restoredCategorizer); } +BOOST_FIXTURE_TEST_CASE(testRestoreWithInconsistentTokenIdFailsGracefully, CTestFixture) { + // A corrupt or truncated state document can leave a category referencing a + // token ID beyond the end of the restored token ID lookup. Such an ID was + // previously used to index the lookup unchecked (for example when building a + // reverse search), causing an out-of-bounds access and a crash. The restore + // must now fail gracefully instead. + // See https://github.com/elastic/ml-cpp/issues/2875 + // + // This state has a single token (so index 0 is the only valid token ID) but + // a category that references token ID 5 in both its base and common unique + // token lists. + const std::string inconsistentState{ + R"({"topLevel":{)" + R"("a":"foo","b":1,)" + R"("c":{"a":"foo bar","b":5,"c":1,"d":7,"j":0,"e":1,"f":5,"g":1,"h":1,"i":2,"k":7},)" + R"("d":0}})"}; + + TTokenListDataCategorizerKeepsFields categorizer{ + m_Limits, NO_REVERSE_SEARCH_CREATOR, 0.7, "whatever"}; + + std::istringstream stateStrm{inconsistentState}; + ml::core::CJsonStateRestoreTraverser traverser{stateStrm}; + const bool restored{traverser.traverseSubLevel( + [&categorizer](ml::core::CStateRestoreTraverser& traverser_) { + return categorizer.acceptRestoreTraverser(traverser_); + })}; + + BOOST_REQUIRE_EQUAL(false, restored); +} + +BOOST_FIXTURE_TEST_CASE(testRestoreWithConsistentTokenIdSucceeds, CTestFixture) { + // Sanity check that the token-ID range validation added for issue #2875 does + // not reject a valid, consistent state document. Same shape as the test + // above, but the category references the only valid token ID (0). + const std::string consistentState{ + R"({"topLevel":{)" + R"("a":"foo","b":1,)" + R"("c":{"a":"foo","b":0,"c":1,"d":3,"j":0,"e":1,"f":0,"g":1,"h":1,"i":2,"k":3},)" + R"("d":0}})"}; + + TTokenListDataCategorizerKeepsFields categorizer{ + m_Limits, NO_REVERSE_SEARCH_CREATOR, 0.7, "whatever"}; + + std::istringstream stateStrm{consistentState}; + ml::core::CJsonStateRestoreTraverser traverser{stateStrm}; + const bool restored{traverser.traverseSubLevel( + [&categorizer](ml::core::CStateRestoreTraverser& traverser_) { + return categorizer.acceptRestoreTraverser(traverser_); + })}; + + BOOST_REQUIRE_EQUAL(true, restored); +} + BOOST_FIXTURE_TEST_CASE(testLongReverseSearch, CTestFixture) { TTokenListDataCategorizerKeepsFields::TTokenListReverseSearchCreatorCPtr reverseSearchCreator{ new ml::model::CTokenListReverseSearchCreator{"_raw"}}; From 5dbfb8d53cc409f376a11e45f1d1e8b074c0fb86 Mon Sep 17 00:00:00 2001 From: Ed Savage Date: Thu, 6 Aug 2026 13:26:13 +1200 Subject: [PATCH 2/4] Update docs/changelog/3143.yaml --- docs/changelog/3143.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/3143.yaml diff --git a/docs/changelog/3143.yaml b/docs/changelog/3143.yaml new file mode 100644 index 0000000000..17165307ca --- /dev/null +++ b/docs/changelog/3143.yaml @@ -0,0 +1,5 @@ +area: Machine Learning +issues: [] +pr: 3143 +summary: Fail gracefully when restoring a categorizer with an out-of-range token ID +type: bug From e4a5f051bf501dd90e37d7d67c70ccbd0a12224c Mon Sep 17 00:00:00 2001 From: Ed Savage Date: Thu, 6 Aug 2026 13:34:51 +1200 Subject: [PATCH 3/4] [ML] Also validate restored ordered common token bounds A category's ordered common token begin/end indices are restored directly from the state document and used to index the category's base token list unchecked (for example in updateOrderedCommonTokenIds and containsCommonInOrderTokensInOrder). A corrupt or truncated state can set an end index beyond the base token list, causing an out-of-bounds access. Extend the restore validation to also require, per category, that the ordered common token bounds describe a valid sub-range of the base token list (begin <= end <= baseTokenIds().size()), failing the restore gracefully otherwise. Relates to elastic/ml-cpp#2875 Co-authored-by: Cursor --- lib/model/CTokenListDataCategorizerBase.cc | 30 ++++++++++++---- .../unittest/CTokenListDataCategorizerTest.cc | 35 +++++++++++++++++-- 2 files changed, 55 insertions(+), 10 deletions(-) diff --git a/lib/model/CTokenListDataCategorizerBase.cc b/lib/model/CTokenListDataCategorizerBase.cc index a4b0c8b828..5d40522434 100644 --- a/lib/model/CTokenListDataCategorizerBase.cc +++ b/lib/model/CTokenListDataCategorizerBase.cc @@ -378,13 +378,11 @@ bool CTokenListDataCategorizerBase::acceptRestoreTraverser(core::CStateRestoreTr } } while (traverser.next()); - // Validate that every token ID referenced by a restored category exists in - // the restored token ID lookup. An inconsistent or truncated state document - // can leave a category referencing a token ID at or beyond the end of - // m_TokenIdLookup. Such an ID is later used to index m_TokenIdLookup - // unchecked (for example when building a reverse search), which is an - // out-of-bounds access resulting in a crash. Fail the restore gracefully - // here instead of proceeding with an inconsistent state. + // Validate the restored categories against an inconsistent or truncated + // state document. Such a document can leave a category referencing data that + // is later used to index a container unchecked, which is an out-of-bounds + // access resulting in a crash. Fail the restore gracefully here instead of + // proceeding with an inconsistent state. const std::size_t numTokens{m_TokenIdLookup.size()}; const auto tokenIdsInRange = [numTokens](const TSizeSizePrVec& tokenIds) { return std::all_of(tokenIds.begin(), tokenIds.end(), @@ -393,6 +391,9 @@ bool CTokenListDataCategorizerBase::acceptRestoreTraverser(core::CStateRestoreTr }); }; for (const auto& category : m_Categories) { + // Every token ID referenced by a category must exist in the restored + // token ID lookup, as these IDs index m_TokenIdLookup unchecked (for + // example when building a reverse search). if (tokenIdsInRange(category.baseTokenIds()) == false || tokenIdsInRange(category.commonUniqueTokenIds()) == false) { LOG_ERROR(<< "Cannot restore categorizer - a category references a token ID " @@ -400,6 +401,21 @@ bool CTokenListDataCategorizerBase::acceptRestoreTraverser(core::CStateRestoreTr << numTokens << "); the state document is inconsistent"); return false; } + + // The ordered common token bounds must describe a valid sub-range of the + // category's base token IDs, as they are used to index m_BaseTokenIds + // unchecked (for example when matching or updating a category). + const TSizeSizePr orderedBounds{category.orderedCommonTokenBounds()}; + if (orderedBounds.first > orderedBounds.second || + orderedBounds.second > category.baseTokenIds().size()) { + LOG_ERROR(<< "Cannot restore categorizer - a category has ordered common token " + "bounds [" + << orderedBounds.first << ", " << orderedBounds.second + << ") outside its base token list (size " + << category.baseTokenIds().size() + << "); the state document is inconsistent"); + return false; + } } // Categories are persisted in order of creation, but this list needs to be diff --git a/lib/model/unittest/CTokenListDataCategorizerTest.cc b/lib/model/unittest/CTokenListDataCategorizerTest.cc index 4dea85f218..e1c3dca7d5 100644 --- a/lib/model/unittest/CTokenListDataCategorizerTest.cc +++ b/lib/model/unittest/CTokenListDataCategorizerTest.cc @@ -571,10 +571,39 @@ BOOST_FIXTURE_TEST_CASE(testRestoreWithInconsistentTokenIdFailsGracefully, CTest BOOST_REQUIRE_EQUAL(false, restored); } +BOOST_FIXTURE_TEST_CASE(testRestoreWithInvalidOrderedTokenBoundsFailsGracefully, CTestFixture) { + // A corrupt or truncated state document can leave a category whose ordered + // common token bounds fall outside its base token list. Those bounds are + // used to index the base token list unchecked (for example when matching or + // updating a category), so an out-of-range end index is an out-of-bounds + // access. The restore must fail gracefully instead. + // See https://github.com/elastic/ml-cpp/issues/2875 + // + // The category has a single, in-range base token ID (0) but an ordered + // common token end index of 5, well beyond the single base token. + const std::string inconsistentState{ + R"({"topLevel":{)" + R"("a":"foo","b":1,)" + R"("c":{"a":"foo","b":0,"c":1,"d":3,"j":0,"e":5,"f":0,"g":1,"h":1,"i":2,"k":3},)" + R"("d":0}})"}; + + TTokenListDataCategorizerKeepsFields categorizer{ + m_Limits, NO_REVERSE_SEARCH_CREATOR, 0.7, "whatever"}; + + std::istringstream stateStrm{inconsistentState}; + ml::core::CJsonStateRestoreTraverser traverser{stateStrm}; + const bool restored{traverser.traverseSubLevel( + [&categorizer](ml::core::CStateRestoreTraverser& traverser_) { + return categorizer.acceptRestoreTraverser(traverser_); + })}; + + BOOST_REQUIRE_EQUAL(false, restored); +} + BOOST_FIXTURE_TEST_CASE(testRestoreWithConsistentTokenIdSucceeds, CTestFixture) { - // Sanity check that the token-ID range validation added for issue #2875 does - // not reject a valid, consistent state document. Same shape as the test - // above, but the category references the only valid token ID (0). + // Sanity check that the range validation added for issue #2875 does not + // reject a valid, consistent state document. The category references the + // only valid token ID (0) and has in-range ordered common token bounds. const std::string consistentState{ R"({"topLevel":{)" R"("a":"foo","b":1,)" From 3aad9dd53a3ee09a8bcff8026a1de3863ec99114 Mon Sep 17 00:00:00 2001 From: Ed Savage Date: Thu, 6 Aug 2026 15:37:38 +1200 Subject: [PATCH 4/4] [ML] Apply clang-format Co-authored-by: Cursor --- lib/model/CTokenListDataCategorizerBase.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/model/CTokenListDataCategorizerBase.cc b/lib/model/CTokenListDataCategorizerBase.cc index 5d40522434..610b73cf5c 100644 --- a/lib/model/CTokenListDataCategorizerBase.cc +++ b/lib/model/CTokenListDataCategorizerBase.cc @@ -410,8 +410,7 @@ bool CTokenListDataCategorizerBase::acceptRestoreTraverser(core::CStateRestoreTr orderedBounds.second > category.baseTokenIds().size()) { LOG_ERROR(<< "Cannot restore categorizer - a category has ordered common token " "bounds [" - << orderedBounds.first << ", " << orderedBounds.second - << ") outside its base token list (size " + << orderedBounds.first << ", " << orderedBounds.second << ") outside its base token list (size " << category.baseTokenIds().size() << "); the state document is inconsistent"); return false;