Closed Bug 1595212 Opened 3 months ago Closed 3 months ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: ferdinandw+bmo, Assigned: emilio)

References

Details

Attachments

(2 files)

Attached file 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.

Attachment #9107619 - Attachment mime type: application/octet-stream → text/plain

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.

Blocks: 1595218

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

Flags: needinfo?(nfroyd)

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...

Flags: needinfo?(nfroyd) → needinfo?(simon.sapin)

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

Duplicate of this bug: 1596035
Summary: error: could not compile 'nsstring' with rust 1.39.0 → Could not compile 'nsstring' with rust 1.39.0 - use `MaybeUninit<T>` instead

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.

Flags: needinfo?(simon.sapin)

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

Assignee: nobody → emilio
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c17276cc50c4
Use MaybeUninit in nsString tests. r=SimonSapin
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
See Also: → 1596311
You need to log in before you can comment on or make changes to this bug.