1595212 - Could not compile 'nsstring' with rust 1.39.0 - use `MaybeUninit<T>` instead

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Ferdinand]

Attachment: rust error output

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

[Mike Hommey [:glandium]]

You're building with --enable-warnings-as-errors is why.

Comment 2

[Ferdinand]

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

[Chris Manchester (limited bugmail, email directly)]

Nathan, can you help find someone to look into this?

Comment 4

[Nathan Froyd [:froydnj]]

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

[Nathan Froyd [:froydnj]]

I guess we could just use MaybeUninit::zeroed, as that would be safe for our string classes or their counterparts in Rust.

Comment 7

[Simon Sapin (:SimonSapin)]

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.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Yeah, I was writing a fix while simon replied lol.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1595212 - Use MaybeUninit in nsString tests. r=SimonSapin

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c17276cc50c4
Use MaybeUninit in nsString tests. r=SimonSapin

Comment 11

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/c17276cc50c4