add more ORT_ENFORCE when building the tree structure of TreeEnsemble operators by xadupre · Pull Request #27677 · microsoft/onnxruntime

xadupre · 2026-03-16T15:14:37Z

Description

The building of the tree was implemented assuming the onnx models has been checked before.
This PR assumes it is not and verifies some indices are not outside boundaries.

The following validations were added or improved:

Bounds and range checks for target_ids/weights mapping when building leaf weight structures, with actionable diagnostic messages including index and container sizes.
Array size relationships (target_class_ids, target_class_nodeids, target_class_treeids, weights) validated once before iteration rather than per-loop.
Non-negative nodes_featureids enforced for non-leaf nodes.
base_values and base_values_as_tensor sizes validated against n_targets_or_classes for both regressors and classifiers, with correct attribute names in error messages.
Unit tests added to cover the new ORT_ENFORCE checks: out-of-range target_ids, negative featureids, and wrong-sized base_values for regressors and classifiers (binary and multi-class).

Motivation and Context

Security issues.

… operators

Copilot

Pull request overview

This PR hardens TreeEnsemble initialization by adding ORT_ENFORCE bounds checks around target/weight indexing to prevent out-of-bounds access when models have not been pre-validated, and adjusts a couple of ML operator tests accordingly.

Changes:

Added additional ORT_ENFORCE checks in TreeEnsembleCommon::Init to validate indices and target id ranges before dereferencing.
Updated TreeRegressor/TreeEnsembler CPU tests to use in-range target_ids / leaf_targetids.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
onnxruntime/core/providers/cpu/ml/tree_ensemble_common.h	Adds extra index/range enforcement when mapping leaf weights to targets/classes.
onnxruntime/test/providers/cpu/ml/treeregressor_test.cc	Adjusts test data to keep `target_ids` within the allowed target range.
onnxruntime/test/providers/cpu/ml/tree_ensembler_test.cc	Adjusts test data to keep `leaf_targetids` within the allowed target range.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

onnxruntime/core/providers/cpu/ml/tree_ensemble_common.h

onnxruntime/test/providers/cpu/ml/treeregressor_test.cc

onnxruntime/test/providers/cpu/ml/tree_ensembler_test.cc

Copilot · 2026-03-16T16:22:42Z

@xadupre I've opened a new pull request, #27678, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2026-03-16T16:22:50Z

@xadupre I've opened a new pull request, #27679, to work on those changes. Once the pull request is ready, I'll request review from you.

github-actions

You can commit the suggested changes from lintrunner.

onnxruntime/core/providers/cpu/ml/tree_ensemble_common.h

onnxruntime/test/providers/cpu/ml/treeregressor_test.cc

yuslepukhin · 2026-03-16T17:44:57Z

There are suggestions to consider and CI to fix.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

xadupre · 2026-03-17T14:29:26Z

There are suggestions to consider and CI to fix.

done

yuslepukhin

Please, resolve the ICM ticket accordingly.

revoking review

yuslepukhin · 2026-03-19T20:56:42Z

Please, resolve comments for tests

…xadupre/tree110413

github-actions

You can commit the suggested changes from lintrunner.

onnxruntime/core/providers/cpu/ml/tree_ensemble_attribute.h

Copilot

Pull request overview

This PR hardens CPU ML TreeEnsemble tree-building against malformed ONNX model attributes by adding additional runtime validations (primarily via ORT_ENFORCE) and adjusting existing tests to comply with the stricter checks.

Changes:

Add bounds/range validation for target_ids/weights mapping when building leaf weight structures.
Enforce non-negative nodes_featureids for non-leaf nodes (while normalizing leaf feature ids).
Update/add unit tests to reflect and validate the new failure behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
onnxruntime/test/providers/cpu/ml/treeregressor_test.cc	Fixes an existing test’s invalid `target_ids` for `n_targets=1` and adds new negative tests covering out-of-range `target_ids` and negative `featureids`.
onnxruntime/test/providers/cpu/ml/tree_ensembler_test.cc	Fixes `leaf_targetids` in `n_targets=1` coverage to conform to new validations.
onnxruntime/core/providers/cpu/ml/tree_ensemble_common.h	Adds new `ORT_ENFORCE` validations during tree construction (target id range, feature id validity) and moves `n_targets_or_classes_` initialization earlier.
onnxruntime/core/providers/cpu/ml/tree_ensemble_attribute.h	Adds validation of `base_values` / `base_values_as_tensor` sizes vs `n_targets_or_classes` for regressor/classifier attribute parsing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

onnxruntime/core/providers/cpu/ml/tree_ensemble_common.h

onnxruntime/core/providers/cpu/ml/tree_ensemble_attribute.h

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

xadupre · 2026-03-26T13:46:22Z

@copilot open a new pull request to apply changes based on the comments in this thread

github-actions

You can commit the suggested changes from lintrunner.

onnxruntime/core/providers/cpu/ml/tree_ensemble_attribute.h

…perators (#27678) ### Description Adds informative error messages to the three `ORT_ENFORCE` index-bounds checks in `tree_ensemble_common.h` that validate `target_class_ids`, `target_class_weights`, and `target_class_weights_as_tensor` accesses. Each message now surfaces the failing index `i` and the container size, e.g.: ``` Index i=5 is out of bounds for target_class_ids (size=3) Index i=5 is out of bounds for target_class_weights (size=3) Index i=5 is out of bounds for target_class_weights_as_tensor (size=3) ``` ### Motivation and Context The original bare `ORT_ENFORCE(i < ...)` calls produced opaque failures with no indication of which attribute was malformed or what the actual index/size values were. This made it difficult to diagnose invalid or adversarial models. Addresses feedback from [#27677](#27677 (comment)).  --- 🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. [Learn more about Advanced Security.](https://gh.io/cca-advanced-security) --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: xadupre <22452781+xadupre@users.noreply.github.com>

…teration (#27679) ### Description Addresses review feedback on #27677. The three per-iteration `ORT_ENFORCE` bounds checks inside the weight assignment loop in `tree_ensemble_common.h` have been moved to a single pre-loop validation block. **Before:** on every iteration, the loop checked `i < target_class_ids.size()`, `i < target_class_weights.size()`, and `i < target_class_weights_as_tensor.size()`. **After:** since `indices` is built from `target_class_nodeids` with consecutive indices `[0, limit)`, `i = indices[indi].second` is always in range — so size relationships are validated once upfront with `== limit` equality checks: ```cpp limit = attributes.target_class_nodeids.size(); ORT_ENFORCE(attributes.target_class_ids.size() == limit, "target_class_ids size (", attributes.target_class_ids.size(), ") must match target_class_nodeids size (", limit, ")"); ORT_ENFORCE(attributes.target_class_weights_as_tensor.empty() || attributes.target_class_weights_as_tensor.size() == limit, ...); ORT_ENFORCE(!attributes.target_class_weights_as_tensor.empty() || attributes.target_class_weights.size() == limit, ...); for (indi = 0; indi < limit; ++indi) { // no per-element bounds checks needed } ``` ### Motivation and Context The per-iteration checks made the control flow harder to reason about and repeated redundant work on every loop iteration. Validating the authoritative array lengths once — before the loop — makes it clear which arrays must be parallel and eliminates repeated checks. Original PR: #27677  --- ✨ Let Copilot coding agent [set things up for you](https://github.com/microsoft/onnxruntime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: xadupre <22452781+xadupre@users.noreply.github.com> Co-authored-by: Xavier Dupré <xadupre@users.noreply.github.com>

…xadupre/tree110413

…rators Agent-Logs-Url: https://github.com/microsoft/onnxruntime/sessions/3d879480-fc50-47de-806f-0cb316b902f5 Co-authored-by: xadupre <22452781+xadupre@users.noreply.github.com>

…xruntime into xadupre/tree110413

add more ORT_ENFORCE when building the tree structure of TreeEnsemble…

7014730

… operators

xadupre requested a review from Copilot March 16, 2026 15:14

Copilot AI reviewed Mar 16, 2026

View reviewed changes

add test to check error messages are raised

afb141a

This was referenced Mar 16, 2026

Add diagnostic context to ORT_ENFORCE bounds checks in TreeEnsemble operators #27678

Merged

Refactor TreeEnsemble weight loop: validate array sizes once before iteration #27679

Merged

github-actions bot reviewed Mar 16, 2026

View reviewed changes

onnxruntime/core/providers/cpu/ml/tree_ensemble_common.h Outdated Show resolved Hide resolved

onnxruntime/test/providers/cpu/ml/treeregressor_test.cc Outdated Show resolved Hide resolved

xadupre and others added 2 commits March 17, 2026 14:21

Update onnxruntime/core/providers/cpu/ml/tree_ensemble_common.h

0d538f0

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Update onnxruntime/test/providers/cpu/ml/treeregressor_test.cc

9442414

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

yuslepukhin previously approved these changes Mar 18, 2026

View reviewed changes

xadupre added 4 commits March 20, 2026 15:40

Merge branch 'main' of https://github.com/microsoft/onnxruntime into …

2fa5b6d

…xadupre/tree110413

Merge branch 'main' of https://github.com/microsoft/onnxruntime into …

ae09adc

…xadupre/tree110413

Merge branch 'main' of https://github.com/microsoft/onnxruntime into …

b06f7c3

…xadupre/tree110413

checks base_values.size()

c05acab

github-actions bot reviewed Mar 23, 2026

View reviewed changes

onnxruntime/core/providers/cpu/ml/tree_ensemble_attribute.h Outdated Show resolved Hide resolved

onnxruntime/core/providers/cpu/ml/tree_ensemble_attribute.h Outdated Show resolved Hide resolved

onnxruntime/core/providers/cpu/ml/tree_ensemble_attribute.h Show resolved Hide resolved

xadupre requested a review from Copilot March 26, 2026 11:44

Copilot started reviewing on behalf of xadupre March 26, 2026 11:45 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

onnxruntime/core/providers/cpu/ml/tree_ensemble_common.h Outdated Show resolved Hide resolved

onnxruntime/core/providers/cpu/ml/tree_ensemble_attribute.h Outdated Show resolved Hide resolved

Update onnxruntime/core/providers/cpu/ml/tree_ensemble_attribute.h

0f98f3b

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

github-actions bot reviewed Mar 26, 2026

View reviewed changes

onnxruntime/core/providers/cpu/ml/tree_ensemble_attribute.h Outdated Show resolved Hide resolved

onnxruntime/core/providers/cpu/ml/tree_ensemble_attribute.h Outdated Show resolved Hide resolved

onnxruntime/core/providers/cpu/ml/tree_ensemble_attribute.h Show resolved Hide resolved

fix use of a negative index in TreeEnsemble

e331d9e

xadupre commented Mar 26, 2026

View reviewed changes

onnxruntime/core/providers/cpu/ml/tree_ensemble_attribute.h Show resolved Hide resolved

Copilot AI and others added 2 commits March 26, 2026 17:22

fix one warning

b966029

Copilot started work on behalf of xadupre April 2, 2026 11:27 View session

xadupre and others added 2 commits April 2, 2026 11:31

Merge branch 'main' of https://github.com/microsoft/onnxruntime into …

c2b4bee

…xadupre/tree110413

Add unit tests for base_values ORT_ENFORCE checks in TreeEnsemble ope…

0a78a0b

…rators Agent-Logs-Url: https://github.com/microsoft/onnxruntime/sessions/3d879480-fc50-47de-806f-0cb316b902f5 Co-authored-by: xadupre <22452781+xadupre@users.noreply.github.com>

Copilot finished work on behalf of xadupre April 2, 2026 11:35

xadupre added 2 commits April 2, 2026 12:33

Merge branch 'xadupre/tree110413' of https://github.com/microsoft/onn…

6d3c046

…xruntime into xadupre/tree110413

add one more tests

db4a2de

Conversation

xadupre commented Mar 16, 2026 • edited by Copilot AI Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Mar 16, 2026

Uh oh!

Copilot AI commented Mar 16, 2026

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yuslepukhin commented Mar 16, 2026

Uh oh!

xadupre commented Mar 17, 2026

Uh oh!

yuslepukhin left a comment

Choose a reason for hiding this comment

Uh oh!

yuslepukhin commented Mar 19, 2026

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

xadupre commented Mar 26, 2026

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xadupre commented Mar 16, 2026 •

edited by Copilot AI

Loading