-
Notifications
You must be signed in to change notification settings - Fork 8
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
MendSlice *might* be UB. #25
Comments
Good point, makes sense |
Note that I'm not sure, hoping @RalfJung can answer, or bring up in "unsafe code guidelines" discussions. |
Context link to the docs https://docs.rs/odds/0.3.1/odds/slice/struct.MendSlices.html |
What exactly is EDIT: Found https://docs.rs/odds/0.3.1/odds/slice/trait.SliceIterExt.html#method.mend_slices but I still didn't find the actual code that @eddyb asks whether it is UB. |
Oh I think I found it at https://docs.rs/odds/0.3.1/src/odds/slice/mod.rs.html#309 Yeah I think this doesn't work. So you are comparing the end address of one slice with the beginning of another. That's an extremely gray area of memory models -- an area where C and C++ differ and we have no idea what the heck LLVM implements. But LLVM is pretty clear that you can do The funny thing is that this will actually work reliably on miri because that assumes allocations never sit right next to each other. ;) |
It's very old code, don't think it is used. It can be removed if this library is updated, because I think mend_slices was more of a cool trick and not a utility. Is the whole stack an allocation? I assume we are not allowed to "mend" together slices of two adjacent arrays on the stack. |
No. Every local variable is its own allocation. |
The code here unfortunately does not only exhibit UB for the cases of mending separate allocations. It also does not ensure that the new slice with length |
Also see oberien/str-concat#8 |
IIUC, it's fine within a memory allocation (what C calls "an object"), but across them, you're usually not allowed to do a lot. cc @RalfJung @solson
The text was updated successfully, but these errors were encountered: