[ML] Fix cross-platform isEof() in CJsonStateRestoreTraverser - #2898
Merged
Conversation
The isEof() implementation was changed from peek()-based to eof()-based in elastic#2895, but eof() is a lagging indicator on Windows - it is not set until a read past the end is attempted. This caused CFieldDataCategorizerTest/testRestoreFromBadState and testRestoreStateRecoversWithEmptyState to fail on Windows. Two fixes: 1. isEof() now falls back to peek() when eof() returns false, making the check portable across platforms. 2. The empty array detection in start() no longer depends on isEof() at all - the token sequence [,] is sufficient to identify an empty array without checking stream state. Tested on macOS ARM, Linux aarch64, and Windows x86_64 - all 800 tests pass on all three platforms. Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
edsavage
added a commit
to edsavage/ml-cpp
that referenced
this pull request
Feb 26, 2026
…c#2898) The isEof() implementation was changed from peek()-based to eof()-based in elastic#2895, but eof() is a lagging indicator on Windows - it is not set until a read past the end is attempted. This caused CFieldDataCategorizerTest/testRestoreFromBadState and testRestoreStateRecoversWithEmptyState to fail on Windows. Two fixes: 1. isEof() now falls back to peek() when eof() returns false, making the check portable across platforms. 2. The empty array detection in start() no longer depends on isEof() at all - the token sequence [,] is sufficient to identify an empty array without checking stream state. Tested on macOS ARM, Linux aarch64, and Windows x86_64 - all tests pass on all three platforms. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
This was referenced Aug 6, 2026
edsavage
added a commit
that referenced
this pull request
Aug 13, 2026
…e 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>
elastic-vault-github-plugin-prod Bot
added a commit
that referenced
this pull request
Aug 13, 2026
…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>
elastic-vault-github-plugin-prod Bot
added a commit
that referenced
this pull request
Aug 13, 2026
…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>
elastic-vault-github-plugin-prod Bot
added a commit
that referenced
this pull request
Aug 13, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CJsonStateRestoreTraverser::isEof()to be portable across platforms by falling back topeek()wheneof()returns false. On Windows,eof()is a lagging indicator that is not set until a read past the end is attempted.isEof()dependency from empty array detection instart()— the token sequence[]is sufficient to identify an empty JSON array without checking stream state.CFieldDataCategorizerTest/testRestoreFromBadStateandCFieldDataCategorizerTest/testRestoreStateRecoversWithEmptyStatewhich were failing on Windows since [ML] Better handling of invalid JSON state documents #2895.Test plan
Made with Cursor
Labelling as
>non-issueas this is related to an as yet unreleased code change.