Could not compile 'nsstring' with rust 1.39.0 - use `MaybeUninit<T>` instead
Categories
(Core :: XPCOM, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox72 | --- | fixed |
People
(Reporter: ferdinandw+bmo, Assigned: emilio)
References
Details
Attachments
(2 files)
I'm puzzled why I seem to be alone in hitting this error.
Building mozilla-central with rust 1.39.0 fails, it was fine with 1.38.0.
I'm compiling from a completely clean tree.
xpcom/rust/nsstring/src/lib.rs causes 24 errors of this kind:
error: the type nsCStr<'static>
does not permit zero-initialization
this code causes undefined behavior when executed
help: use MaybeUninit<T>
instead
I'll attach the error output, it gets mangled if when pasting here.
Comment 1•5 years ago
|
||
You're building with --enable-warnings-as-errors is why.
You're right, I had been staring at my mozconfig for a cause, but overlooked that one.
Probably because it never had any effect before. Thanks Mike.
Comment 3•5 years ago
|
||
Nathan, can you help find someone to look into this?
Comment 4•5 years ago
|
||
Simon, can you provide some suggestions as to what the mem::zeroed
bits here:
https://searchfox.org/mozilla-central/source/xpcom/rust/nsstring/src/lib.rs#1398-1423
can be replaced with? I assume that we want something around MaybeUninit
, but it's not clear to me that we can use MaybeUninit::as_ptr
to calculate all the same values. I guess we should also delete the macro_rules
case at:
https://searchfox.org/mozilla-central/source/xpcom/rust/nsstring/src/lib.rs#1373-1391
since it doesn't seem to be called, and would be subject to the same warnings if it was...
Comment 5•5 years ago
|
||
I guess we could just use MaybeUninit::zeroed
, as that would be safe for our string classes or their counterparts in Rust.
Updated•5 years ago
|
Comment 7•5 years ago
|
||
TL;DR: I think something like this is close to the best we can do today:
macro_rules! field_layout {
($field: ident in $Struct: path) => {{
let uninit = std::mem::MaybeUninit::<$Struct>::uninit();
// FIXME: use raw pointer projection when it becomes available:
// https://github.com/rust-lang/rust/issues/64490
let &$Struct { $field: ref field, .. } = unsafe { &*uninit.as_ptr() };
let size = std::mem::size_of_val(field);
let align = std::mem::align_of_val(field);
let offset =
(field as *const _ as usize) -
(uninit.as_ptr() as usize);
(size, align, offset)
}}
}
(Extracting it to a separate macro avoids repeating it three times.)
Having a zero-initialized nsStringRepr
on the stack is UB because zero/null violates the validity invariant of its ptr::NonNull<$char_type>
field. Having MaybeUninit<nsStringRepr>
on the stack instead fixes that immediate problem. This also makes mem::forget
unnecessary, since MaybeUninit
uses ManuallyDrop
internally.
For what this macro is doing I believe there is no difference between zeroed and uninitialized, so it might as well not spend time zeroing.
It is sound to manipulate the value inside a MaybeUninit
through raw pointers only (for example, using std::ptr::write
to initialize it). However the precise Rust language rules around valid uses of unsafe
are not yet fully settled, and it’s possible that we’ll want to make it so that validity invariants must holds for values behind a &T
reference. This means that &(*ptr).$member as *const _
would be UB, since it creates a reference. New syntax is proposed to take a raw pointer to a struct field without creating a reference, which would fix this potential UB, but it’s not implemented yet. Similarly for size_of_val
and align_of_val
for raw pointers. So creating a temporary reference is the best we can do today.
https://internals.rust-lang.org/t/discussion-on-offset-of/7440/2 also mentions using a struct pattern to protect against taking a field of the deref target type in case the struct implements Deref
.
There’s also a discussion of adding an offset_of!
macro or offsetof
keyword to the standard library or language, but nothing concrete so far as far as I know.
Assignee | ||
Comment 8•5 years ago
|
||
Yeah, I was writing a fix while simon replied lol.
Assignee | ||
Comment 9•5 years ago
|
||
Comment 10•5 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c17276cc50c4 Use MaybeUninit in nsString tests. r=SimonSapin
Comment 11•5 years ago
|
||
bugherder |
Description
•