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

libafl_bolts tuples relies on standard library implementation details #2648

Open
Noratrieb opened this issue Nov 1, 2024 · 3 comments
Open

Comments

@Noratrieb
Copy link

/// Returns if the type `T` is equal to `U`, ignoring lifetimes.
#[inline] // this entire call gets optimized away :)
#[must_use]
pub fn type_eq<T: ?Sized, U: ?Sized>() -> bool {
// decider struct: hold a cell (which we will update if the types are unequal) and some
// phantom data using a function pointer to allow for Copy to be implemented
struct W<'a, T: ?Sized, U: ?Sized>(&'a Cell<bool>, PhantomData<fn() -> (&'a T, &'a U)>);
// default implementation: if the types are unequal, we will use the clone implementation
impl<T: ?Sized, U: ?Sized> Clone for W<'_, T, U> {
#[inline]
fn clone(&self) -> Self {
// indicate that the types are unequal
// unfortunately, use of interior mutability (Cell) makes this not const-compatible
// not really possible to get around at this time
self.0.set(false);
W(self.0, self.1)
}
}
// specialized implementation: Copy is only implemented if the types are the same
#[allow(clippy::mismatching_type_param_order)]
impl<T: ?Sized> Copy for W<'_, T, T> {}
let detected = Cell::new(true);
// [].clone() is *specialized* in core.
// Types which implement copy will have their copy implementations used, falling back to clone.
// If the types are the same, then our clone implementation (which sets our Cell to false)
// will never be called, meaning that our Cell's content remains true.
let res = [W::<T, U>(&detected, PhantomData)].clone();
res[0].0.get()
}

this code here relies on the (unsound) specialization of Copy for cloning of arrays. I hope I don't have to explain why that's a problem^^. this will probably break in the future (since it's unsound), either with the specialization being removed, the lifetime-dependent Copy implementation being an error or both.

how else should it be implemented then? not. this operation cannot be implemented correctly and a different way to do the stuff that uses it today needs to be found.

@domenukk
Copy link
Member

domenukk commented Nov 1, 2024

Feel free to propose a better / sound way. I sadly don't know of any.

FWIW we do run tests in CI and would catch future breakage:

fn test_type_eq() {

I agree it's not a good solution, but it'll be pretty hard to come up with a better one..

@Noratrieb
Copy link
Author

There is indeed not really a better way. As I said, this is not an operation that can be implemented correctly. I recommend just deleting the functions. This repo doesn't seem to use it, and users of the crate need to find a different way.

@domenukk
Copy link
Member

domenukk commented Nov 2, 2024

It's used here:

fn match_name<T>(&self, name: &str) -> Option<&T> {
if type_eq::<Head, T>() && name == self.0.name() {
unsafe { (addr_of!(self.0) as *const T).as_ref() }
} else {
self.1.match_name::<T>(name)
}
}

And that's a pretty important piece, if we just leave it out we can run into type confusions everywhere..

I don't think there is much we can do until/if ever min_specialization lands, sadly..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants