Skip to content

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

Closed
malfet wants to merge 1 commit intopytorch:masterfrom
malfet:malfet/onnx-do-not-deref-nullptr
Closed

[onnx] Do not deref nullptr in scalar type analysis#50237
malfet wants to merge 1 commit intopytorch:masterfrom
malfet:malfet/onnx-do-not-deref-nullptr

Conversation

@malfet
Copy link
Copy Markdown
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 a team, BowenBao and SplitInfinity January 7, 2021 22:52
@facebook-github-bot
Copy link
Copy Markdown
Contributor

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

💊 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
Copy Markdown
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
Copy Markdown
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

Comment thread torch/csrc/jit/passes/onnx/scalar_type_analysis.cpp Outdated
@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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor

facebook-github-bot commented Jan 8, 2021

@malfet merged this pull request in 81778e2.

@t-vi
Copy link
Copy Markdown
Collaborator

t-vi commented Jan 9, 2021 •
edited
Loading

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
Copy Markdown
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

@BowenBao BowenBao BowenBao left review comments

@SplitInfinity SplitInfinity Awaiting requested review from SplitInfinity

+2 more reviewers

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

@walterddr walterddr walterddr approved these changes

Reviewers whose approvals may not affect merge requirements

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.

5 participants

@malfet @facebook-github-bot @t-vi @walterddr @BowenBao