-
Notifications
You must be signed in to change notification settings - Fork 21.7k
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
ASAN: fix use-after-free #101064
ASAN: fix use-after-free #101064
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/101064
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 9b0e102: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
30c32e2
to
7db75b1
Compare
@@ -395,7 +396,11 @@ const Tensor& resize_( | |||
SymIntArrayRef size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't the explicit copying just be done for this variable by making it take the SymIntArray by value instead of by reference? That would make an explicit copy and allow it take to ownership if it's an rvalue? Or is the issue that there is no implicit conversion to convert the reference to a value? See comments below, we have a method to do this explicit copy for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a class like SymIntArray? No, issue is not in conversion, issue is in ownership.
auto org_size = self.sym_sizes(); | ||
std::vector<c10::SymInt> org_size_copy{org_size.begin(), org_size.end()}; | ||
std::vector<c10::SymInt> size_copy{size.begin(), size.end()}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto org_size = self.sym_sizes(); | |
std::vector<c10::SymInt> org_size_copy{org_size.begin(), org_size.end()}; | |
std::vector<c10::SymInt> size_copy{size.begin(), size.end()}; | |
auto orig_size_copy = self.sym_sizes().vec(); | |
auto size_copy = size.vec(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, somehow I've missed this function. Will use it.
auto org_size = self.sym_sizes(); | ||
auto template_size = the_template.sym_sizes(); | ||
std::vector<c10::SymInt> org_size_copy{org_size.begin(), org_size.end()}; | ||
std::vector<c10::SymInt> template_size_copy{ | ||
template_size.begin(), template_size.end()}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto org_size = self.sym_sizes(); | |
auto template_size = the_template.sym_sizes(); | |
std::vector<c10::SymInt> org_size_copy{org_size.begin(), org_size.end()}; | |
std::vector<c10::SymInt> template_size_copy{ | |
template_size.begin(), template_size.end()}; | |
auto org_size_copy = self.sym_sizes().vec(); | |
auto template_size_copy = the_template.sym_sizes().vec(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the .vec()
helper method instead for cleaner, more concise copies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
Using .vec() should make the fix simpler indeed!
7db75b1
to
6ad246e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Note that github actions are really sad right now. But once github status stabilizes, we should get green CI and we can merge!
6ad246e
to
d300a9c
Compare
Can add a test covering this case? |
I have found it while running: Sorry for not mentioning it. Does it need a separate test? |
Should we enable ASAN and UBSAN for a larger range of tests? |
I'm sure the answer for "should we" is yes. But can we do it? I'm not sure, that might be a good thing to open an issue to discuss this in more details with the Infra team! |
A simple option is to run tests on a separate ASAN build. However, that may be time consuming. More discussions are encouraged. |
@pytorchbot merge -r |
@pytorchbot successfully started a rebase job. Check the current status here |
When tensor is resized, reference array to it's sizes may become invalid. Make a copy in advance.
Successfully rebased |
d300a9c
to
9b0e102
Compare
Merge failedReason: This PR needs a label If not, please add the To add a label, you can comment to pytorchbot, for example For more information, see Details for Dev Infra teamRaised by workflow job |
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
When tensor is resized, reference array to it's sizes may become invalid. Make a copy in advance.
ASAN report
Additional backtraces (not full)
Memory deallocation:
Memory access: