Skip to content

fix out of boundary vector per class in SVM#27952

Merged
xadupre merged 16 commits into
mainfrom
xadupre/si-svm-111801
Apr 28, 2026
Merged

fix out of boundary vector per class in SVM#27952
xadupre merged 16 commits into
mainfrom
xadupre/si-svm-111801

Conversation

@xadupre

@xadupre xadupre commented Apr 2, 2026

Copy link
Copy Markdown
Member

Description

vector_per_class dimension was not verified, it could lead to illegal memory access

Motivation and Context

security issue

Comment thread onnxruntime/core/providers/cpu/ml/svmclassifier.cc Outdated
tianleiwu
tianleiwu previously approved these changes Apr 3, 2026
@xadupre

xadupre commented Apr 10, 2026

Copy link
Copy Markdown
Member Author

@copilot can you fix the build?

Copilot AI commented Apr 10, 2026

Copy link
Copy Markdown
Contributor

@copilot can you fix the build?

Fixed in commit 2a1d2c6. The build errors were caused by type mismatches after changing *_count_ members to size_t:

  1. narrow<ptrdiff_t>narrow<size_t> when accumulating vector_count_ (was adding signed to unsigned)
  2. Added explicit narrow<size_t> cast for class_j_support_count (implicit int64_tsize_t conversion)
  3. SafeInt<int32_t>SafeInt<size_t> for the final_scores_per_batch subspan offset (consistent with other subspan calls)

Comment thread onnxruntime/core/providers/cpu/ml/svmclassifier.cc

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the CPU ML SVMClassifier implementation against malformed models by adding validation around vectors_per_class sizing (to prevent potential out-of-bounds access), and adds a regression test to ensure the new validation triggers as expected.

Changes:

  • Add a negative test ensuring undersized vectors_per_class fails with a clear error.
  • Tighten SVMClassifier constructor validation and adjust internal indexing types to size_t.
  • Minor refactoring of classifier scoring loops to use size_t indexing.

Reviewed changes

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

File Description
onnxruntime/test/providers/cpu/ml/svmclassifier_test.cc Adds regression coverage for undersized vectors_per_class.
onnxruntime/core/providers/cpu/ml/svmclassifier.h Switches internal count/index member types to size_t.
onnxruntime/core/providers/cpu/ml/svmclassifier.cc Adds dimension validation and updates indexing/types in constructor and compute path.

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

Comment thread onnxruntime/core/providers/cpu/ml/svmclassifier.cc Outdated
Comment thread onnxruntime/core/providers/cpu/ml/svmclassifier.cc Outdated
Comment thread onnxruntime/core/providers/cpu/ml/svmclassifier.cc Outdated
Comment thread onnxruntime/core/providers/cpu/ml/svmclassifier.cc Outdated
Comment thread onnxruntime/test/providers/cpu/ml/svmclassifier_test.cc Outdated
tianleiwu
tianleiwu previously approved these changes Apr 10, 2026

@tianleiwu tianleiwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Solid security hardening for the SVM classifier. The key fix — validating vectors_per_class_.size() == class_count_ in SVC mode — correctly closes the out-of-bounds access from crafted models. The type migration from ptrdiff_t/int64_t to size_t for count/index members is appropriate and removes many redundant narrow<> casts.

Remaining issues are cosmetic. I agree with the open threads from the Copilot reviewer (duplicate proba_ check, grammar in error message, test naming). Those are all minor and can be addressed in a follow-up if desired.

LGTM.

Comment thread onnxruntime/core/providers/cpu/ml/svmclassifier.cc Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
xadupre and others added 2 commits April 22, 2026 10:33
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@xadupre

xadupre commented Apr 22, 2026

Copy link
Copy Markdown
Member Author

@copilot resolve the merge conflicts in this pull request

Co-authored-by: Tianlei Wu <tlwu@microsoft.com>
@xadupre

xadupre commented Apr 22, 2026

Copy link
Copy Markdown
Member Author

@copilot resolve the merge conflicts in this pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants