Github Fix soundness issue for `replace_range` and `range` by dylni · Pull Reque...
source link: https://github.com/rust-lang/rust/pull/81169
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
Contributor
dylni commented 12 days ago
Fixes #81138 by only calling start_bound and end_bound once.
I also fixed the same issue for BTreeMap::range and BTreeSet::range.
Collaborator
rust-highfive commented 12 days ago
(rust-highfive has picked a reviewer for you, use r? to override)
Contributor
bors commented 12 days ago
The latest upstream changes (presumably #81159) made this pull request unmergeable. Please resolve the merge conflicts.
@@ -1553,18 +1553,21 @@ impl String {
// Replace_range does not have the memory safety issues of a vector Splice.
// of the vector version. The data is just plain bytes.
match range.start_bound() {
let start = range.start_bound();
bugadani 12 days ago
Contributor
I think comments explaining this change might be beneficial here as well. With or without a big // WARNING: do not inline this variable :)
Included(&n) => assert!(self.is_char_boundary(n + 1)),
Excluded(&n) => assert!(self.is_char_boundary(n)),
Unbounded => {}
};
unsafe { self.as_mut_vec() }.splice(range, replace_with.bytes());
// Using `range` again would be unsound (#81138)
dylni 12 days ago
Author
Contributor
@bugadani Do you think another comment would be helpful? I added one where I thought it would make the most sense, since adding one above let start but not let end is a little weird.
Contributor
KodrAus commented 12 days ago
@@ -25,7 +25,10 @@ where
K: Borrow<Q>,
R: RangeBounds<Q>,
{
match (range.start_bound(), range.end_bound()) {
// Using `range` again would be unsound (#81138)
KodrAus 12 days ago
Contributor
Contributor
KodrAus commented 12 days ago
I've just left a few comments. It would be good to include the EvilRange as a test case, but r=me at any time.
Contributor
Author
dylni commented 12 days ago
Would you like to include the example from #81138 as a test case to make sure we are still maintaining the UTF8 invariant?
Yes, good idea.
Contributor
KodrAus commented 12 days ago
@dylni This looks good to me! Would you like to squash these down then we should be good to go.
Contributor
Author
dylni commented 12 days ago
Thanks @KodrAus! Is this what you had in mind? Squashing is a little awkward with merge commits.
Contributor
KodrAus commented 12 days ago
@bors r+ p=1
Contributor
bors commented 12 days ago
Commit b96063c has been approved by KodrAus
That's it, thanks! A rebase and squash is what I'd meant. We try to avoid merge commits in PRs and leave that to bors to handle.
Thanks for fixing this up 
Contributor
bors commented 12 days ago
Testing commit Contributor
bors commented 12 days ago
Test successful - checks-actions
Approved by: KodrAus
Pushing 7d7b22d to master...
Contributor
Author
dylni commented 11 days ago
Oh, thanks, that makes sense.
Contributor
ssomers commented 11 days ago
I also fixed the same issue for
BTreeMap::rangeandBTreeSet::range.
Useful and inspirational, but I feel the need to point out that you didn't entirely plug that hole, and probably nobody can. Unlike String::replace_range whose range is tied to usize, the caller of BTreeMap::range gets to choose a type and its Ord implementation. I got a little lost trying to brew a really evil example with a panic, but I still believe it can be done. Here on playground is proof that there are 12 integers from 5 to 7, with the help of an evil borrow.
Contributor
Author
dylni commented 9 days ago
@ssomers I think that's ok. If an impl used by BTreeMap misbehaves, BTreeMap is allowed to misbehave too. The only thing it can't do is something unsound. Although the playground example doesn't give the right result for the number values, it gives the right result based on the Borrow impl.
@dylni I agree, but that means there was no unsoundness in BTreeMap's use of start_bound/end_bound, is what I realized this week. How many times these functions are called or what they return doesn't matter for unsoundness. It's what the various invocations of the lookup type's Ord say that might make BTreeMap trip up.
This PR of course still includes a welcome fix to avoid BTreeMap from appearing to misbehave when the range misbehaves. If there was a possibility of unsoundness in BTreeMap, that possibility is still there. And I can't find it.
Contributor
Author
dylni commented 7 days ago
Contributor
ssomers commented 7 days ago
Not sure how you arrived in left_edge. But yes, disappointingly, I don't think there's anything to exploit. Essentially, BTreeMap is already prepared to withstand a malicious implementation of Ord for the key type, so the fact that range smuggles in another Ord for its search can't make things worse. When BTreeMap validates the range and panics, it doesn't do that to protect itself from unsoundness, but just to advise the caller that they messed up the range (with a message assuming they have an honest Ord implementation). Not sure if the original authors of the code realized that, but it's written in the contract that it should panic on an invalid range, so I'd better leave that to be.
Contributor
Author
dylni commented 7 days ago
@ssomers left_edge is called here.
When BTreeMap validates the range and panics, it doesn't do that to protect itself from unsoundness, but just to advise the caller that they messed up the range (with a message assuming they have an honest
Ordimplementation).
I agree, but this was surprising. I think I'll change the comments a little there to make it clearer for people reading it in the future.
None yet
Successfully merging this pull request may close these issues.
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK