Unsoundness in `dispatch2`

[anayw2001]

During a soundness audit for the unsafe code in dispatch2, we noticed a potential soundness issue where dyn Fn() -> u8 was being transmuted to dyn Fn() -> bool here. This is unsound because of the potential for vtable layout differences between the two types of functions, so even though the bits are copied when transmuting, the memory layout could be unexpected and therefore lead to corruption (ABI incompatibility between u8 and bool). Additionally, if calling convention somehow differs between the two types of functions, this could also lead to issues with memory corruption.

[madsmtm]

Thanks for the review!

I know that the signature looks like dyn Fn(), but under the hood it's converted to an extern "C-unwind" fn(), so I don't think there are any real vtable concerns here.

But you are right that bool and u8 aren't guaranteed to be ABI compatible, and e.g. Miri complains on a similar line of code, and the docs on ABI compatibility does point out this difference.

Though I'm fairly sure that it isn't an issue in practice it is on all the architectures that dispatch2 supports (32-bit ARM, Aarch64, x86 and x86_64)? That is, I think u8 and bool are ABI compatible on those architectures?

[madsmtm]

(That's not to say that I don't agree that this is an issue that should be fixed - I'm mostly just discussing the severity of the issue).

[anayw2001]

Wouldn't the usage of dyn necessitate the generation of a vtable? Also, would it be possible to instead return a bool instead of a u8 from the mentioned Block function?

[madsmtm]

The closure is converted to an extern "C-unwind" here. The dyn Fn(...) -> u8 is only to give the signature of said extern "C-unwind".

Also, would it be possible to instead return a bool instead of a u8 from the mentioned Block function?

At some point yes, but not currently because we do not implement objc2::encode::Encode for bool, to allow bool to mean Bool/BOOL when used in Objective-C contexts.

[anayw2001]

Got it, that makes sense. I guess implementing objc2::encode::Encode for bool would be the best path forward to allow us to drop the unsafe code entirely?