Skip to content
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

Off by one error in str::index_mut (specifically) for RangeToInclusive (specifically) #50002

Closed
ExpHP opened this issue Apr 16, 2018 · 4 comments
Labels
C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@ExpHP
Copy link
Contributor

ExpHP commented Apr 16, 2018

Caught while fleshing out a much beefier test suite for SliceIndex, which I plan to submit as a PR. (this Issue is here just in case that PR never happens).

rust/src/libcore/str/mod.rs

Lines 2107 to 2115 in 1ef1563

fn index_mut(self, slice: &mut str) -> &mut Self::Output {
assert!(self.end != usize::max_value(),
"attempted to index str up to maximum usize");
if slice.is_char_boundary(self.end) {
unsafe { self.get_unchecked_mut(slice) }
} else {
super::slice_error_fail(slice, 0, self.end + 1)
}
}

The unicode character boundary check here should be looking at self.end + 1. Better yet, the method could just delegate to RangeTo instead. (note: it can't delegate to get_mut here without NLL, which seems to be a leading factor as to how this function ended up this way)

@kennytm kennytm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Apr 16, 2018
@ExpHP ExpHP changed the title Off by one error in str::get_mut (specifically) for RangeToInclusive (specifically) Off by one error in str::index_mut (specifically) for RangeToInclusive (specifically) Apr 16, 2018
@alexcrichton alexcrichton added this to the 1.26 milestone Apr 21, 2018
@alexcrichton alexcrichton added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Apr 21, 2018
@alexcrichton
Copy link
Member

Not technically a regression but tagging it as such to ensure that we don't forget about it.

bors added a commit that referenced this issue Apr 21, 2018
smaller PR just to fix #50002

I pulled this out of #50010 to make it easier to backport to beta if necessary, considering that inclusive range syntax is stabilizing soon (?).

It fixes a bug in `<str>::index_mut` with `(..=end)` ranges (#50002), which prior to this fix was not only unusable but also UB in the cases where it "worked" (it gave improperly truncated UTF-8).

(not that I can imagine why anybody would *use* `<str>::index_mut`... but I'm not here to judge)
@bors bors closed this as completed in b74d692 Apr 21, 2018
@ExpHP
Copy link
Contributor Author

ExpHP commented Apr 21, 2018

@alexcrichton my commit message accidentally closed this, which might defeat the purpose of the tag you added

(note: I myself can't reopen it because Github attributed the message to Bors)

@alexcrichton
Copy link
Member

Oh that's ok, the tags on the PR take over now for backporting and that's how we'll track this.

alexcrichton pushed a commit to alexcrichton/rust that referenced this issue Apr 23, 2018
@ExpHP
Copy link
Contributor Author

ExpHP commented May 10, 2018

(ahhhh, now I see. The tag is used in automation for a Github Project to track regressions. That's actually pretty cool!)

(...don't mind me, just talking to myself)

alexcrichton added a commit to alexcrichton/rust that referenced this issue May 10, 2018
Give SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check)

So one day I was writing something in my codebase that basically amounted to `impl SliceIndex for (Bound<usize>, Bound<usize>)`, and I said to myself:

*Boy, gee, golly!  I never realized bounds checking was so tricky!*

At some point when I had around 60 lines of tests for it, I decided to go see how the standard library does it to see if I missed any edge cases. ...That's when I discovered that libcore only had about 40 lines of tests for slicing altogether, and none of them even used `..=`.

---

This PR includes:

* **Literally the first appearance of the word `get_unchecked_mut` in any directory named `test` or `tests`.**
* Likewise the first appearance of `get_mut` used with _any type of range argument_ in these directories.
* Tests for the panics on overflow with `..=`.
    * I wanted to test on `[(); usize::MAX]` as well but that takes linear time in debug mode </3
* A horrible and ugly test-generating macro for the `should_panic` tests that increases the DRYness by a single order of magnitude (which IMO wasn't enough, but I didn't want to go any further and risk making the tests inaccessible to next guy).
* Same stuff for str!
    * Actually, the existing `str` tests were pretty good. I just helped filled in the holes.
* [A fix for the bug it caught](rust-lang#50002).  (only one ~~sadly~~)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants