-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
RepeatN
: use MaybeUninit
#130145
RepeatN
: use MaybeUninit
#130145
Conversation
rustbot has assigned @Mark-Simulacrum. Use |
Note that this changes the |
|
||
#[test] | ||
fn test_repeat_n_soundness() { | ||
let x = std::iter::repeat_n(String::from("use after free"), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also test clone please 🤔
r=me afterwards
This comment has been minimized.
This comment has been minimized.
@bors r+ rollup=never |
cc @rust-lang/libs |
This comment has been minimized.
This comment has been minimized.
library/core/tests/iter/sources.rs
Outdated
let mut y = std::iter::repeat_n(Box::new(0), 1); | ||
let x = y.next().unwrap(); | ||
let _z = y; | ||
assert_eq(0, *x); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_eq(0, *x); | |
assert_eq!(0, *x); |
oops |
@bors r- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the use of ManuallyDrop
was correct here (ignoring the UAF issue obviously), however before rust-lang/unsafe-code-guidelines#245 is fixed, it makes sense to use ManuallyDrop
instead.
Can you leave a FIXME
to replace it back with ManuallyDrop
, once it doesn't cause issues?
This iterator could be implemented using minimal to no unsafe code (I used none in this example) like this: use core::num::NonZeroUsize;
#[derive(Debug, Clone)]
pub struct RepeatN<T>(Option<(T, NonZeroUsize)>);
impl<T> RepeatN<T> {
pub fn new(value: T, count: usize) -> Self {
Self(NonZeroUsize::new(count).map(|count| (value, count)))
}
// for use in last and maybe other methods
fn take_element(&mut self) -> Option<T> {
self.0.take().map(|(value, _count)| value)
}
}
impl<T: Clone> Iterator for RepeatN<T> {
type Item = T;
fn next(&mut self) -> Option<Self::Item> {
let (value, count) = self.0.as_mut()?;
if let Some(new_count) = NonZeroUsize::new(count.get() - 1) {
*count = new_count;
Some(value.clone())
} else {
// this could be unwrap_unchecked
Some(self.0.take().unwrap().0)
}
}
fn size_hint(&self) -> (usize, Option<usize>) {
let len = self.0.as_ref().map_or(0, |(_value, count)| count.get());
(len, Some(len))
}
...
} |
That also has the advantage of interacting with dropck a bit better(although you could also use fn src() -> String {
// fails today, but shouldn't if you use the option approach
let x = "foo".to_owned();
let mut y = std::iter::repeat_n(&*x, 1);
// use y, but don't consume it entirely
x
} |
Does this also preserve niches from |
#[stable(feature = "iter_repeat_n", since = "1.82.0")] | ||
pub struct RepeatN<A> { | ||
count: usize, | ||
// Invariant: has been dropped iff count == 0. | ||
element: ManuallyDrop<A>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, it's intentional that this was ManuallyDrop
and not MaybeUninit
, because there's no case in which this is constructed without a value, and thus we can preserve niches for it.
Does this actually need to change to MaybeUninit
? Feels to me like the fix is to have Clone
do a bitwise copy if count == 0
-- which will have the nice behaviour of eliding the branch for T: Copy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ManuallyDrop
currently doesn't work here, see #130141 and #130145 (review).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean that ManuallyDrop<Box<_>>
is just never sound? Do we need #[lang = "manually_drop"]
to remove that requirement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean that
ManuallyDrop<Box<_>>
is just never sound?
It's sound if you don't touch it in any way after running Box
's drop I think (i.e. not even move). But generally yes.
To fix this we need to implement MaybeDangling
(#118166) which would be a lang item, yes (and then wrap the only field of ManuallyDrop
with MaybeDangling
).
To @antonilol's point, the only reason this was using unsafe code was to preserve niches, and thus to allow the implementation for If this is going to use |
|
Tbh, it's probably best if the NonZero field is used as the niche for the option, as that allows just loading that field for stuff like use std::num::NonZero;
type T = Option<(String, NonZero<usize>)>;
#[no_mangle]
pub fn src(x: &T) -> usize {
x.as_ref().map_or(0, |a| a.1.get())
}
type U = Option<(NonZero<usize>, String)>;
#[no_mangle]
pub fn dst(x: &U) -> usize {
x.as_ref().map_or(0, |a| a.0.get())
}
#[no_mangle]
pub fn good(x: &Option<(NonZero<usize>, std::os::fd::OwnedFd)>) -> usize {
x.as_ref().map_or(0, |a| a.0.get())
}
#[no_mangle]
pub fn bad(x: &Option<(std::os::fd::OwnedFd, NonZero<usize>)>) -> usize {
x.as_ref().map_or(0, |a| a.1.get())
} src: # @src
# %bb.0:
xor eax, eax
cmp rax, qword ptr [rdi]
jo .LBB0_2
# %bb.1:
mov rax, qword ptr [rdi + 24]
.LBB0_2:
ret
# -- End function
dst: # @dst
# %bb.0:
xor eax, eax
cmp rax, qword ptr [rdi + 8]
jo .LBB1_2
# %bb.1:
mov rax, qword ptr [rdi]
.LBB1_2:
ret
# -- End function
good: # @good
# %bb.0:
mov rax, qword ptr [rdi]
ret
# -- End function
bad: # @bad
# %bb.0:
cmp dword ptr [rdi], -1
je .LBB3_1
# %bb.2:
mov rax, qword ptr [rdi + 8]
ret
.LBB3_1:
xor eax, eax
ret The loss of the other niches mainly hurts the size of it being wrapped in iterator adapters that would wrap it in an option like |
If the compiler doesn't know the original size, that could result in this extra branch, but if it does know, it is smart enough to figure it out (first 3 examples, compiler explorer link) https://rust.godbolt.org/z/W8hqncT81 (How does this even produce 10x more code for 'len_5' compared to 'len_4'? That feels very counterintuitive.) |
I'd like a definite answer on what approach I should take (e.g. fixing the use-after-free first by not changing the field type and writing manual impls but keeping the miri reported UB due to using ManuallyDrop?) |
It's fun to fuss over an unsoundness issue that is also an optimization problem, but it's better to actually ship the fix. This code seems adequate to quell the soundness issue, we can fuss over improvements later. @bors r=lcnr,workingjubilee |
☀️ Test successful - checks-actions |
Finished benchmarking commit (2e367d9): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 759.355s -> 759.624s (0.04%) |
Since this is stabilizing in 1.82: @rustbot label beta-nominated |
[beta] backports - Don't warn empty branches unreachable for now rust-lang#129103 - Win: Add dbghelp to the list of import libraries rust-lang#130047 - `RepeatN`: use MaybeUninit rust-lang#130145 - Update LLVM to 19 327ca6c rust-lang#130212 - Revert rust-lang#129749 to fix segfault in LLVM rust-lang#130477 - Check params for unsafety in THIR rust-lang#130531 r? cuviper
[beta] backports - Don't warn empty branches unreachable for now rust-lang#129103 - Win: Add dbghelp to the list of import libraries rust-lang#130047 - `RepeatN`: use MaybeUninit rust-lang#130145 - Update LLVM to 19 327ca6c rust-lang#130212 - Revert rust-lang#129749 to fix segfault in LLVM rust-lang#130477 - Check params for unsafety in THIR rust-lang#130531 r? cuviper try-job: dist-various-1
[beta] backports - Don't warn empty branches unreachable for now rust-lang#129103 - Win: Add dbghelp to the list of import libraries rust-lang#130047 - `RepeatN`: use MaybeUninit rust-lang#130145 - Update LLVM to 19 327ca6c rust-lang#130212 - Revert rust-lang#129749 to fix segfault in LLVM rust-lang#130477 - Check params for unsafety in THIR rust-lang#130531 r? cuviper try-job: dist-various-1
[beta] backports - Don't warn empty branches unreachable for now rust-lang#129103 - Win: Add dbghelp to the list of import libraries rust-lang#130047 - `RepeatN`: use MaybeUninit rust-lang#130145 - Update LLVM to 19 327ca6c rust-lang#130212 - Revert rust-lang#129749 to fix segfault in LLVM rust-lang#130477 - Check params for unsafety in THIR rust-lang#130531 r? cuviper try-job: dist-various-1
[beta] backports - Don't warn empty branches unreachable for now rust-lang#129103 - Win: Add dbghelp to the list of import libraries rust-lang#130047 - `RepeatN`: use MaybeUninit rust-lang#130145 - Update LLVM to 19 327ca6c rust-lang#130212 - Revert rust-lang#129749 to fix segfault in LLVM rust-lang#130477 - Check params for unsafety in THIR rust-lang#130531 r? cuviper try-job: dist-various-1
[beta] backports - Don't warn empty branches unreachable for now rust-lang#129103 - Win: Add dbghelp to the list of import libraries rust-lang#130047 - `RepeatN`: use MaybeUninit rust-lang#130145 - Update LLVM to 19 327ca6c rust-lang#130212 - Revert rust-lang#129749 to fix segfault in LLVM rust-lang#130477 - Check params for unsafety in THIR rust-lang#130531 r? cuviper try-job: dist-various-1
Closes #130140. Closes #130141.
Use
MaybeUninit
instead ofManuallyDrop
for soundness.