Skip to content
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

Fix SegFault bug in shape inference #5990

Merged
merged 7 commits into from
Mar 4, 2024
Merged

Conversation

OYCN
Copy link
Contributor

@OYCN OYCN commented Mar 2, 2024

Description

The bug is SegFault during shape inference if the schema not be set inference function.

The schema has CheckInputOutputType for shape inference and Verify for raw node proto. And the behavior is not fully aligned.

Motivation and Context

fixes #5989

Signed-off-by: opluss <opluss@qq.com>
Copy link

codecov bot commented Mar 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.82%. Comparing base (d7e2b81) to head (9cad14b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5990      +/-   ##
==========================================
+ Coverage   56.81%   56.82%   +0.01%     
==========================================
  Files         506      506              
  Lines       30357    30365       +8     
  Branches     4589     4590       +1     
==========================================
+ Hits        17246    17254       +8     
  Misses      12283    12283              
  Partials      828      828              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

OYCN added 4 commits March 2, 2024 23:59
Signed-off-by: opluss <opluss@qq.com>
Signed-off-by: opluss <opluss@qq.com>
Signed-off-by: opluss <opluss@qq.com>
Signed-off-by: opluss <opluss@qq.com>
@OYCN OYCN marked this pull request as ready for review March 3, 2024 04:38
@OYCN OYCN requested review from a team as code owners March 3, 2024 04:38
@justinchuby justinchuby added this to the 1.16 milestone Mar 4, 2024
@justinchuby
Copy link
Contributor

@cjvolzka @gramalingam may be worthy to be included in 1.16 since this is fixing a memory error.

Copy link
Contributor

@gramalingam gramalingam left a comment

Choose a reason for hiding this comment

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

The check can be made more general, as suggested in comments. But it's ok to merge this as is too, if we want to get this into the upcoming release.

@cjvolzka
Copy link
Contributor

cjvolzka commented Mar 4, 2024

@justinchuby I can wait to cut rc2 until this has been cherry-picked into the rel-1.16.0 branch.

onnx/defs/schema.cc Outdated Show resolved Hide resolved
onnx/defs/schema.cc Outdated Show resolved Hide resolved
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
@justinchuby
Copy link
Contributor

I am going to merge as is for us to fix the bug, and then allow the author to create a follow-up.

@justinchuby justinchuby added this pull request to the merge queue Mar 4, 2024
Merged via the queue into onnx:main with commit b590c5f Mar 4, 2024
41 checks passed
cjvolzka pushed a commit that referenced this pull request Mar 4, 2024
### Description
Cherry-pick #5990 into `rel-1.16.0` branch

The bug is SegFault during shape inference if the schema not be set
inference function.

The schema has `CheckInputOutputType` for shape inference and `Verify`
for raw node proto. And the behavior is not fully aligned.

### Motivation and Context

fixes #5989

---------

Signed-off-by: opluss <opluss@qq.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
cjvolzka added a commit that referenced this pull request Mar 5, 2024
### Description
Cherry-Pick #5990 into `rel-1.16.0` branch

The bug is SegFault during shape inference if the schema not be set
inference function.

The schema has `CheckInputOutputType` for shape inference and `Verify`
for raw node proto. And the behavior is not fully aligned.

### Motivation and Context

fixes #5989

Signed-off-by: opluss <opluss@qq.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Co-authored-by: oPluss <opluss@qq.com>
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
@OYCN OYCN deleted the dev-fix-bug branch March 8, 2024 14:40
isdanni pushed a commit to isdanni/onnx that referenced this pull request Mar 18, 2024
### Description

The bug is SegFault during shape inference if the schema not be set
inference function.

The schema has `CheckInputOutputType` for shape inference and `Verify`
for raw node proto. And the behavior is not fully aligned.

### Motivation and Context

fixes onnx#5989

---------

Signed-off-by: opluss <opluss@qq.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: isdanni <leedanni@gmail.com>
github-merge-queue bot pushed a commit that referenced this pull request Mar 19, 2024
### Description

- Add `OpSchema::VerifyInputNum` and `OpSchema::VerifyOutputNum`
methods. The logic has been moved from `OpSchema::Verify`.
- Replaced the input/output number check logic in
`OpSchema::CheckInputOutputType` with calls to the new interfaces.
- Without using the `Node(domain::op_type::version)` string pattern in
`OpSchema::CheckInputOutputType` when calling `fail_check`, as the
`op_type` will be displayed by the shape inference common exception.
Otherwise, it would appear redundant.

### Motivation and Context

follow-up #5990

resolve #5993

---------

Signed-off-by: opluss <opluss@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

SegFault during shape inference if the schema not be set inference function
4 participants