-
Notifications
You must be signed in to change notification settings - Fork 590
Avoid use-after-return caused by double move #11485
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
base: main
Are you sure you want to change the base?
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/11485
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 2c6361e with merge base 611092d ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This pull request was exported from Phabricator. Differential Revision: D76271359 |
@pytorchbot label "release notes: none" |
Summary: As described. Differential Revision: D76271359
This pull request was exported from Phabricator. Differential Revision: D76271359 |
This pull request was exported from Phabricator. Differential Revision: D76271359 |
This pull request was exported from Phabricator. Differential Revision: D76271359 |
This pull request was exported from Phabricator. Differential Revision: D76271359 |
Summary: Previously, `Result<T>`'s constructor used `std::move(val)` to initialize the value. This resulted in an unnecessary extra move and destructor call on a moved-from stack object, triggering an HWASAN stack tag mismatch when the moved-from object was later destructed. Replacing `std::move(val)` with `std::forward<T>(val)` avoids the extra move while preserving correct semantics. This ensures only one move occurs and avoids lifetime violations that can lead to tag mismatches under HWASAN. Reviewed By: StefanBossbaly Differential Revision: D76271359
This pull request was exported from Phabricator. Differential Revision: D76271359 |
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 for the catch!
/* implicit */ Result(T&& val) : value_(std::move(val)), hasValue_(true) {} | ||
/// Value forwarding constructor. | ||
/* implicit */ Result(T&& val) | ||
: value_(std::forward<T>(val)), hasValue_(true) {} |
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.
Can you add a test that wouldve caught the previous bad behavior.
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.
Sure. Where are the tests for this type?
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.
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.
looks great, needs test, happy to accept with the test
I tried writing a test, but I have failed to induce the problematic behavior. I think it shows up only in the presence of stack tagging as performed by hwasan. |
/// Value move constructor. | ||
/* implicit */ Result(T&& val) : value_(std::move(val)), hasValue_(true) {} | ||
/// Value forwarding constructor. | ||
/* implicit */ Result(T&& val) | ||
: value_(std::forward<T>(val)), hasValue_(true) {} |
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.
I misread what was going on here originally. This isn't a template function (though it is a member of a template class), so this really is an rvalue reference. I think that the original code is probably correct; std::forward is for arguments to template functions (https://en.cppreference.com/w/cpp/utility/forward.html).
Summary:
Previously,
Result<T>
's constructor usedstd::move(val)
to initialize thevalue. This resulted in an unnecessary extra move and destructor call on a
moved-from stack object, triggering an HWASAN stack tag mismatch when the
moved-from object was later destructed.
Replacing
std::move(val)
withstd::forward<T>(val)
avoids the extra movewhile preserving correct semantics. This ensures only one move occurs and
avoids lifetime violations that can lead to tag mismatches under HWASAN.
Differential Revision: D76271359