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

stats: fix UB in from_slice #83

Merged
merged 1 commit into from
Jul 15, 2022
Merged

Conversation

ordian
Copy link
Member

@ordian ordian commented Jul 15, 2022

As documented in https://doc.rust-lang.org/std/mem/union.MaybeUninit.html,

let x: i32 = unsafe { MaybeUninit::uninit().assume_init() }; // undefined behavior! ⚠️

@ordian ordian force-pushed the ao-fix-ub-in-ColumnStats-from_slice branch from b742a26 to 7170c73 Compare July 15, 2022 10:17
for item in &mut data[..] {
item.write(reader(cursor));
}
data.map(|x| unsafe { x.assume_init() })
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this copies the data unfortunately
using transmute currently not possible with const generics: rust-lang/rust#61956
but I can change it to a macro instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, I think it's a noop: https://godbolt.org/z/YcxonWxPa

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@arkpar arkpar Jul 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well get rid of unsafe here.

 let plain: [u32; 1024] = [Default::default(); 1024];
 let mut atomic: [AtomicU32; 1024] = a.map(AtomicU32::new);
 // Read into `atomic` below.

Shoudl be optimized away as well.

Copy link
Member Author

@ordian ordian Jul 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure it's optimized this way though. The produced code seems different: https://godbolt.org/z/GGnfTro49
Also to sanity check, created a bench:

from_slice/Unsafe       time:   [1.0590 µs 1.0613 µs 1.0636 µs]                               
from_slice/Safe         time:   [2.1197 µs 2.1255 µs 2.1326 µs]  

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's leave it as it is then.

@arkpar arkpar merged commit 1e3e4ec into master Jul 15, 2022
@arkpar arkpar deleted the ao-fix-ub-in-ColumnStats-from_slice branch July 15, 2022 11:35
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