Fix integer overflow in dump for split thresholds beyond int32 range - #12289
Open
momomuchu wants to merge 2 commits into
Open
Fix integer overflow in dump for split thresholds beyond int32 range#12289momomuchu wants to merge 2 commits into
momomuchu wants to merge 2 commits into
Conversation
TextGenerator::Integer and JsonGenerator::Integer round a kInteger feature's split condition and cast it to an integer with no range check. The original int32 cast was undefined behavior for thresholds outside int32; widening to int64 only moved the boundary to ~9.22e18, since static_cast to int64 of an out-of-range double is still UB. Clamp the rounded value to [INT64_MIN, INT64_MAX] before the cast, so an out-of-range threshold saturates deterministically instead of emitting platform-dependent garbage. This is unreachable for real models (genuine integer features stay well within int64) but keeps the dump deterministic for a mislabeled >int64 float feature. predict() and the float (kQuantitive) path are untouched, and reachable-range dump output is byte-identical. Closes dmlc#10035
Member
|
I think we can fix it by simply using |
Per review, drop the double based saturating clamp and just widen the split threshold cast in TextGenerator::Integer and JsonGenerator::Integer from int32_t to int64_t. Larger integers are not supported for training, so the extra out-of-int64 saturation path is unnecessary. Removes the now-obsolete beyond-int64 saturation test; the other regression tests for the int32 overflow (dmlc#10035) are unchanged and still pass.
trivialfis
self-requested a review
July 29, 2026 09:44
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
2 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.
Fixes #10035.
get_dump()andtrees_to_dataframe()report a garbage threshold (INT32_MIN or INT32_MAX) instead of the real split condition when akIntegerfeature's threshold exceeds the int32 range. For example a model split at ~3e10 dumps2147483647.predict()is unaffected: the predictor compares against the raw floatSplitCondand never goes through the dump formatter, so only the text/JSON/dataframe dumps were wrong.Root cause: the two
Integer()dump formatters insrc/tree/tree_model.cc(TextGeneratorandJsonGenerator) do an uncheckedstatic_cast<int32_t>on the floored split condition, which wraps around (and is UB for out-of-range values).This widens the cast to
int64_tso the real threshold is reported, and clamps to the int64 range before the cast so a value beyond int64 (only reachable if a >9.2e18 float feature is mislabeled as int) saturates deterministically rather than being UB. The clamp is a no-op for every value a genuine int feature can hold. Both dump paths andtrees_to_dataframenow report the true threshold.Added tests for the int64 overflow, the in-range boundary (no regression), float thresholds being unaffected, and the beyond-int64 saturation.
plot_tree/graphviz was checked and never had this bug (it renders the raw float, not the integer formatter), so it is left unchanged.One open question for you: I widened to int64 to preserve the value; the alternative is clamping to int32 with a warning if you would rather keep a strict 32-bit dump range for downstream consumers. Happy to switch if you prefer.