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

fix UB on TypedArray::as_typed_aray #244

Merged
merged 1 commit into from
Jan 8, 2024
Merged

Conversation

rbran
Copy link
Contributor

@rbran rbran commented Jan 6, 2024

tldr: the code inside bool::then_some is always executed, but the code in the closure from bool::then is only executed if is true.

This is UB, because the bool::then_some is a function that takes a bool and a generic value, because of that, the value inside of it is always evaluated, independent if the bool is true/false.

it's clear if the desugarize the syntax, the code bellow is equivalent to the function as_typed_array:

// first we get the true/false for the condition
let cond: bool = self.is_typed_array::<T>();
// then we evaluate the expression to pass the value as parameter
// UB here, the is executed always, independent of the cond being true/false
let value: &TypedArray<T> = unsafe { self.ref_typed_array() };
// then we call the function `bool::then_some`.
bool::then_some(cond, value)

The solution is simply change it to bool::then, because the parameter it's a closure, it's lazy evaluated, and only executed if the condition is true.

As mentioned at https://github.com/rust-lang/rust/blob/5cb2e7dfc362662b0036faad3bab88d73027fd05/src/tools/clippy/clippy_lints/src/transmute/mod.rs#L468-L520

And presented at https://youtu.be/hBjQ3HqCfxs?si=nrIz6ZHnxEHO6Pik&t=53

@DelSkayn
Copy link
Owner

DelSkayn commented Jan 8, 2024

Good catch! Thanks for the PR!

@DelSkayn DelSkayn merged commit 35b32d8 into DelSkayn:master Jan 8, 2024
5 checks passed
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

Successfully merging this pull request may close these issues.

2 participants