-
Notifications
You must be signed in to change notification settings - Fork 386
Add miri trophy for LazyArray::swap (ink! PR) #1368
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
Add miri trophy for LazyArray::swap (ink! PR) #1368
Conversation
Details to the found in rust-lang#1364. Note that this was not a found in a `master` or production release of ink!, however without analysing the code via `miri` this could have potentially happened.
Thanks! |
📌 Commit 72442ac has been approved by |
…alfJung Add miri trophy for LazyArray::swap (ink! PR) Details to the found in #1364. Note that this was not a found in a `master` or production release of ink!, however without analysing the code via `miri` this could have potentially happened.
README.md
Outdated
@@ -281,6 +281,7 @@ Violations of [Stacked Borrows] found that are likely bugs (but Stacked Borrows | |||
* [Aliasing mutable references in `sized-chunks`](https://github.com/bodil/sized-chunks/issues/8) | |||
* [`String::push_str` invalidating existing references into the string](https://github.com/rust-lang/rust/issues/70301) | |||
* [`ryu` using raw pointers outside their valid memory area](https://github.com/dtolnay/ryu/issues/24) | |||
* [`LazyArray::swap` method creating overlapping mutable references (ink! PR)](https://github.com/rust-lang/miri/issues/1364) |
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 am confused by the ink! PR
part here, is there a PR we could link to?
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.
The PR where this bug was caught is this:
use-ink/ink#311
However, I am not sure it makes sense to link the PR since the PR is already linked via the linked miri issue that adds a lot more context to the underlying issue than the PR.
I should note that the ink! PR is kind of important in that it represents the next generation of a very important module within the ink! framework.
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.
Ah yes, and that's not a PR for just a fix, it's a more general thing.
But then the link text is confusing, because it sounds like it links to an ink! PR.
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.
Shall I remove the "(ink! PR)" parentheses?
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.
That would be good I think -- but ink!
should appear somehow. Maybe just replace LazyArray::swap
by it? Like,
ink!
creating overlapping mutable references
(or maybe without backticks)
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.
done
@bors r- retry |
☀️ Try build successful - checks-travis, status-appveyor |
Looking good. :) |
📌 Commit 3f43305 has been approved by |
☀️ Test successful - checks-travis, status-appveyor |
Details to the found in #1364.
Note that this was not a found in a
master
or production release of ink!, however without analysing the code viamiri
this could have potentially happened.