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.

Sign up for GitHub

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

Jump to bottom

[onnx] Do not deref nullptr in scalar type analysis #50237

Closed
wants to merge 1 commit into from
Closed

[onnx] Do not deref nullptr in scalar type analysis #50237

wants to merge 1 commit into from

Conversation

malfet
Copy link
Contributor

@malfet malfet commented Jan 7, 2021

Apply a little bit of defensive programming: type->cast<TensorType>() returns an optional pointer so dereferencing it can lead to a hard crash.

Fixes SIGSEGV reported in #49959

@malfet malfet requested review from SplitInfinity, BowenBao and a team January 7, 2021 22:52
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jan 7, 2021 •
edited

💊 CI failures summary and remediations

As of commit 5e29382 (more details on the Dr. CI page):

  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

This comment has been revised 32 times.

@facebook-github-bot facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Jan 7, 2021
facebook-github-bot
facebook-github-bot reviewed Jan 7, 2021
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

BowenBao
BowenBao reviewed Jan 7, 2021
View reviewed changes
Copy link
Collaborator

@BowenBao BowenBao left a comment

Choose a reason for hiding this comment

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

Thanks for working on a fix for this! I think this PR includes similar fix https://github.com/pytorch/pytorch/pull/50163/files#diff-26fb8ea5176375fe4e4a7522807968d4ef715717a890d1c4860686f2080ab1f2

torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp Outdated Show resolved Hide resolved
@SplitInfinity SplitInfinity mentioned this pull request Jan 7, 2021
Closed
@malfet
[onnx] Do not deref nullptr in scalar type analysis
5e29382 
Apply a little bit of defensive programming: `type->cast<TensorType>()` returns an optional pointer so dereferencing it can lead to a hard crash.

Fixes SIGSEGV reported in pytorch#49959
@malfet malfet force-pushed the malfet/onnx-do-not-deref-nullptr branch from e3ef8c0 to 5e29382 Compare January 7, 2021 23:49
facebook-github-bot
facebook-github-bot reviewed Jan 7, 2021
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@codecov
Copy link

codecov bot commented Jan 8, 2021

Codecov Report

Merging #50237 (5e29382) into master (160b4be) will increase coverage by 0.00%.
The diff coverage is 87.50%.

@@           Coverage Diff           @@
##           master   #50237   +/-   ##
=======================================
  Coverage   80.68%   80.68%           
=======================================
  Files        1902     1902           
  Lines      206348   206354    +6     
=======================================
+ Hits       166485   166491    +6     
  Misses      39863    39863

@facebook-github-bot
Copy link
Contributor

@malfet merged this pull request in 81778e2.

@t-vi
Copy link
Collaborator

t-vi commented Jan 9, 2021 •
edited

Apply a little bit of defensive programming: type->cast() returns an optional pointer so dereferencing it can lead to a hard crash.

You probably know, but you can also use type->expect<TensorType>() to get a pointer safe for dereferencing or an exception.

@malfet malfet deleted the malfet/onnx-do-not-deref-nullptr branch January 12, 2021 02:43
@malfet
Copy link
Contributor Author

malfet commented Jan 12, 2021

@t-vi the design paradigm in that codepath looked like nullptr should be silently skipped rather than raising exception on it.

hwangdeyu pushed a commit to hwangdeyu/pytorch that referenced this pull request Jan 14, 2021
@malfet
[onnx] Do not deref nullptr in scalar type analysis (pytorch#50237)
8797b37 
Summary:
Apply a little bit of defensive programming: `type->cast<TensorType>()` returns an optional pointer so dereferencing it can lead to a hard crash.

Fixes SIGSEGV reported in pytorch#49959

Pull Request resolved: pytorch#50237

Reviewed By: walterddr

Differential Revision: D25839675

Pulled By: malfet

fbshipit-source-id: 403d6df5e2392dd6adc308b1de48057f2f9d77ab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@BowenBao BowenBao BowenBao left review comments

@walterddr walterddr walterddr approved these changes

@SplitInfinity SplitInfinity Awaiting requested review from SplitInfinity

Assignees
No one assigned
Labels
cla signed Merged oncall: jit Add this issue/PR to JIT oncall triage queue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@malfet @facebook-github-bot @t-vi @walterddr @BowenBao