-
Notifications
You must be signed in to change notification settings - Fork 671
[FIL] Validate Treelite model input to prevent integer overflow and OOB memory access #8016
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
af890c6
Validate Treelite input to avoid overflow
chyunsu3 e1c4606
Add validation to add_categorical_node
chyunsu3 0dac58a
Incorporate feedback from CodeRabbit
chyunsu3 b91ec68
Elide sign check if cat_t is unsigned
chyunsu3 d0395af
Merge remote-tracking branch 'origin/main' into validate_treelite
chyunsu3 8632d9e
Address review comments
chyunsu3 5eed8ef
Update integer check in add_categorical_node()
chyunsu3 bb12e5b
Add missing inline keywords
chyunsu3 b404011
Simplify category bounds check + add gtest
chyunsu3 99d13f6
Add bound check for tree_index
chyunsu3 ecfe23a
Add check on node::node(); clean up exceptions
chyunsu3 7007801
Add gtest coverage for node bound check
chyunsu3 36315bf
Safe float casting in get_decision_forest()
chyunsu3 90f0851
Merge remote-tracking branch 'origin/main' into validate_treelite
chyunsu3 d20d069
Use 64-bit for average factor, if possible
chyunsu3 d5cbde8
Enhance error handling in treelite_importer and simplify decision_for…
csadorf afce4e1
Revert "Add bound check for tree_index"
csadorf ba78634
Validate treelite tree before building decision forest.
csadorf 02e8e23
Add tests for decision forest builder to validate error handling
csadorf File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,5 +1,5 @@ | ||||||
| /* | ||||||
| * SPDX-FileCopyrightText: Copyright (c) 2023-2025, NVIDIA CORPORATION. | ||||||
| * SPDX-FileCopyrightText: Copyright (c) 2023-2026, NVIDIA CORPORATION. | ||||||
| * SPDX-License-Identifier: Apache-2.0 | ||||||
| */ | ||||||
| #pragma once | ||||||
|
|
@@ -22,6 +22,9 @@ struct bitset { | |||||
| using storage_type = storage_t; | ||||||
| using index_type = index_t; | ||||||
|
|
||||||
| // Ensrue that index_t is unsigned. Bound checks below rely on index_t being unsigned | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| static_assert(std::is_unsigned_v<index_t>, "index_t must be unsigned"); | ||||||
|
chyunsu3 marked this conversation as resolved.
|
||||||
|
|
||||||
| auto constexpr static const bin_width = index_type(sizeof(storage_type) * 8); | ||||||
|
|
||||||
| HOST DEVICE bitset() : data_{nullptr}, num_bits_{0} {} | ||||||
|
|
@@ -39,12 +42,13 @@ struct bitset { | |||||
| // Standard bit-wise mutators and accessor | ||||||
| HOST DEVICE auto& set(index_type index) | ||||||
| { | ||||||
| data_[bin_from_index(index)] |= mask_in_bin(index); | ||||||
| // Guard against OOB writes; silently ignored to preserve memory safety | ||||||
| if (index < num_bits_) { data_[bin_from_index(index)] |= mask_in_bin(index); } | ||||||
|
chyunsu3 marked this conversation as resolved.
|
||||||
| return *this; | ||||||
| } | ||||||
| HOST DEVICE auto& clear(index_type index) | ||||||
| { | ||||||
| data_[bin_from_index(index)] &= ~mask_in_bin(index); | ||||||
| if (index < num_bits_) { data_[bin_from_index(index)] &= ~mask_in_bin(index); } | ||||||
| return *this; | ||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||
| } | ||||||
| HOST DEVICE auto test(index_type index) const | ||||||
|
|
||||||
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,17 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: Copyright (c) 2023-2025, NVIDIA CORPORATION. | ||
| * SPDX-FileCopyrightText: Copyright (c) 2023-2026, NVIDIA CORPORATION. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| #pragma once | ||
| #include <cuml/fil/detail/raft_proto/gpu_support.hpp> | ||
|
|
||
| #include <type_traits> | ||
|
|
||
| namespace raft_proto { | ||
| template <typename T, typename U> | ||
| HOST DEVICE auto constexpr ceildiv(T dividend, U divisor) | ||
| { | ||
| return (dividend + divisor - T{1}) / divisor; | ||
| static_assert(std::is_integral_v<T> && std::is_integral_v<U>, "Arguments must be integers"); | ||
| return dividend / divisor + (dividend % divisor != 0); | ||
|
dantegd marked this conversation as resolved.
|
||
| } | ||
| } // namespace raft_proto | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is additional out-of-bound access for the unused bit-wise boolean operations. I will remove those in a follow-up.