[ML] Fail gracefully when restoring a categorizer with an out-of-range token ID - #3143
Conversation
…e 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 elastic#2895/elastic#2898. Relates to elastic#2875 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pinging @elastic/ml-core (Team:ML) |
|
Hi @edsavage, I've created a changelog YAML for you. |
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#2875 Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens categorizer state restoration in ml-cpp to avoid native crashes when encountering inconsistent/truncated persisted state. It adds post-restore validation to detect out-of-range token references (and related invalid bounds) early, returning false so the caller can fail the restore gracefully instead of later triggering out-of-bounds container access.
Changes:
- Add restore-time validation that every restored category’s referenced token IDs are within the restored token lookup bounds, failing restore if not.
- Add restore-time validation that each category’s ordered common token bounds form a valid subrange of its base token list.
- Add unit tests covering inconsistent vs consistent token ID restoration behavior (plus invalid ordered-bounds restoration).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/model/unittest/CTokenListDataCategorizerTest.cc | Adds unit tests ensuring invalid restored state fails gracefully and valid state still restores. |
| lib/model/CTokenListDataCategorizerBase.cc | Adds post-restore validation to prevent later unchecked indexing from crashing the process. |
| docs/changelog/3143.yaml | Adds a changelog entry documenting the bug fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
…e token ID (#3143) (#3152) 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 #2875 (cherry picked from commit 0f41e80) Co-authored-by: Ed Savage <ed.savage@elastic.co> Co-authored-by: Cursor <cursoragent@cursor.com>
…e token ID (#3143) (#3151) 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 #2875 (cherry picked from commit 0f41e80) Co-authored-by: Ed Savage <ed.savage@elastic.co> Co-authored-by: Cursor <cursoragent@cursor.com>
…of-range token ID (#3143) (#3150) * [ML] Fail gracefully when restoring a categorizer with an out-of-range token ID (#3143) 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 #2875 Co-authored-by: Cursor <cursoragent@cursor.com> (cherry picked from commit 0f41e80) * [ML] Add missing CJsonStateRestoreTraverser include on 8.19 backport The cherry-picked unit tests use JSON restore; 8.19's test file only included RapidXml headers, so all platform builds failed to compile. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Ed Savage <ed.savage@elastic.co> Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Fixes a residual native crash (SIGSEGV) when restoring a categorizer from an inconsistent or truncated state document, seen on a
9.6.0-SNAPSHOTbuild (see #2875).CTokenListDataCategorizerBase::acceptRestoreTraverserrestores a list of tokens (m_TokenIdLookup) and a list of categories. Each restored category references token IDs (its base and common-unique token IDs). Those IDs are later used to indexm_TokenIdLookupwithout a bounds check — e.g. incacheReverseSearch:m_TokenIdLookupis a Boost multi-index container whose random-accessoperator[]performs no bounds checking. If a corrupt/truncated state leaves a category referencing a token ID at or beyond the end of the restored lookup, this is an out-of-bounds access that returns a garbageCTokenInfoItem; calling.str()on it then dereferences a bogus pointer, crashing theautodetectprocess with a SIGSEGV inside libc (strlen/memcpy) rather than failing the restore.This matches the crash signature reported in #2875 on 9.6:
si_signo 11,si_code 1(SEGV_MAPERR), crash PC inlibc.so.6. The graceful invalid-state handling added in #2895 / #2898 does not cover this particular path.Fix
Validate, at the end of
acceptRestoreTraverser, that every token ID referenced by a restored category exists in the restored token ID lookup. If any is out of range, log an error and fail the restore gracefully (returningfalse), consistent with the intent of #2895 / #2898, instead of proceeding with an inconsistent state that crashes later.Testing
Added two unit tests in
CTokenListDataCategorizerTest:testRestoreWithInconsistentTokenIdFailsGracefully— a state with a single token but a category referencing token ID 5; restore now returnsfalse(previously undefined behaviour / crash).testRestoreWithConsistentTokenIdSucceeds— a valid state referencing the only valid token ID (0) still restores successfully, confirming the new check does not reject good state.Both pass, along with the existing
testPersistround-trip test (so valid states are unaffected).Relates to #2875