Fix stacked borrow violations

[Noratrieb]

There was a Stacked Borrows issue in NodeChildrenInternal::insert and NodeChildrenInternal::remove, where the pointer to src was invalidated by creating the pointer to dst afterwards.

This also generally improves the unsafe code even further by commenting more on the invariants and how they are upheld. I accidentally introduced a UAF bug after the first fix, so I took a real close look at everything before I found it 😅.

Also runs miri in CI to make sure this crate stays sound in the future, but certain tests are ignored in miri because they take too long. Using smaller testdata for miri is something for the future, I tried to do it but it didn't quite work because the tests indexed into it everywhere, so I gave up.

[Noratrieb]

Note: Miri doesn't work on x86 because str-indices uses SIMD there. I cross-ran it on aarch64 locally, we should also do something like this in CI and document it.

[cessen]

Thanks so much for this! I tried to run Ropey through miri before, but wasn't able to get it to run in anything less than what seemed like an infinite amount of time, so I gave up. (IMO the miri docs could use some improvement, there.)

I'll give this a thorough review the next chance I get.

Note: Miri doesn't work on x86 because str-indices uses SIMD there. I cross-ran it on aarch64 locally, we should also do something like this in CI and document it.

I think it would make more sense to add a feature flag to str_indices (and a corresponding flag in Ropey) to disable/enable SIMD, and disable it for the miri run. I don't think that would be too hard, since all the SIMD stuff is very compartmentalized. And we'll need that anyway to keep miri working when we implement SIMD for aarch64.

In any case, we'll either need to deal with that before merging this, or remove the CI stuff from this PR. CI is currently failing for that reason, it appears.

[saethlin]

IMO the miri docs could use some improvement, there.

Do you mean you looked at the rust-lang/miri README? If I wrote a section about this, what keywords would you be looking for?

[cessen]

I've added a "simd" cargo feature flag to str_indices, and a corresponding "simd" feature flag to Ropey that controls it when building Ropey. So you should be able to just disable default features in Ropey to get miri to run now.

@saethlin

Do you mean you looked at the rust-lang/miri README?

Looking over the readme again, it actually looks fine to me now. The thing I missed last time was how to disable tests (e.g. long-running ones) just for miri, but it looks like it's covered. I don't know if it was updated since I last checked it out, or if I just totally missed it (very possible).

[saethlin]

I don't know either :) I'm just looking for feedback on making Miri more accessible

[Noratrieb]

A few rebases later I added the flag to CI and also did the small change in insert/remove that Saethlin proposed above. Everything should be good to go now.

[cessen]

Awesome! It'll probably still be a bit before I have a chance to give this a proper review, but I definitely want to get this merged--it's a great contribution to Ropey's memory safety and very much appreciated. Thanks so much!

[Noratrieb]

Thanks for the quick response and positive feedback as well! I also wanted to add that the unsafe code here was really good and well encapsulated, a really good example

[Noratrieb]

I reverted all the style changes so that the only remaining changes are the actual fixes (the pointer in ptr::copy_nonoverlapping) and additional comments

[cessen]

Awesome, thanks so much for this! And wow... that's a really subtle change. I even more appreciate you undoing the other changes now, because at first glance the fix itself also looks superfluous... but in fact isn't.

Thanks again!