-
Notifications
You must be signed in to change notification settings - Fork 6
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
patch* functions should be marked unsafe #5
Comments
Fair. It'll make it slightly more annoying to use in tests but it's better to call this kind of hankey panky out. I'll accept a PR or I'll get to it in the next couple days. |
@Permutatrix Do you mind writing a small example to demonstrate an invariant being broken I can add to documentation? I can't think of something concrete. |
@mehcode Here's a small example of guerrilla being used to cause memory violations in safe code: extern crate smallvec;
extern crate guerrilla;
use smallvec::SmallVec;
use guerrilla::patch2;
fn main() {
let mut sv = SmallVec::<[u32; 0]>::new();
// spill sv onto the heap.
// we'll trigger a debug assertion without this line, which is
// still bad but perhaps not quite as convincing.
sv.push(0);
let _guard = patch2(SmallVec::<[u32; 0]>::grow, |_self, _new_cap| {
// instead of growing, do nothing.
});
// now loop until we trigger a segmentation fault.
loop {
sv.push(0);
}
} |
Thinking back on this I've changed my mind. Regarding your example, rust |
@mehcode Niko Matsakis wrote a blog post on the composability of safe abstractions that may be of interest: http://smallcultfollowing.com/babysteps/blog/2016/10/02/observational-equivalence-and-unsafe-code/ Regardless of whether you accept his model, there are other things that make these functions unsafe. For instance, multiple threads patching and unpatching the same function can cause race conditions. |
I'm classing that as a bug for now that will be fixed soon enough (tm).
I do agree with most of that. I just have some reservations on making this function unsafe because of its limited intended usage. I'll think on it. I'll leave this issue open to collect feedback for awhile on if we should make this function unsafe. This crate is designed for use in a (currently) single-threaded |
The internal state invariants are the only possible justification for being able to use One possible remedy for |
IMO it should still be |
As a compromise for test UX, what about if we deprecate the functions, rely only on a |
Setting aside the question of whether the patching routine itself should be considered safe, these functions can be used to break invariants, which makes them unsafe.
The text was updated successfully, but these errors were encountered: