1888581 - MOZ_CRASH(unsafe precondition(s) violated: slice::from_raw_parts requires the pointer to be aligned and non-null

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

[Brad Werth [:bradwerth] (out until March 10)]

On my macOS local Nightly build, I get this crash when viewing most any web content. Debugging indicates that this is crashing here in FontInstanceKey::add_key when the instance has no variations. In such a case, the instance variations Vec has a raw pointer that is not memory-aligned. This Vec is created from WrVecU8::convert_into_vec which has the possibility of creating non-aligned empty Vecs.

I have no idea why this isn't a bigger deal, hitting other users, nor why it just started now for me. It is, however, easy enough to fix.

Comment 1

[Brad Werth [:bradwerth] (out until March 10)]

Attachment: Bug 1888581 Part 2: Make WrVecU8::convert_into_vec return memory-aligned in all cases.

Comment 2

[Brad Werth [:bradwerth] (out until March 10)]

This is probably happening to me because I ran a rustup update before my most recent build, and I'm on the nightly branch. Even if that's the reason, and I could avoid this problem by going back to stable, this is a future problem that needs solving.

Comment 3

[Erich Gubler [:ErichDonGubler] (he/him)]

:bradwerth: Perhaps propagating the assertion from std to constructors of WrVecU8 could be helpful in tracking down culprit code?

Comment 4

[Brad Werth [:bradwerth] (out until March 10)]

(In reply to Erich Gubler [:ErichDonGubler] from comment #3)

:bradwerth: Perhaps propagating the assertion from std to constructors of WrVecU8 could be helpful in tracking down culprit code?

Maybe... but that would require constructors to specify the alignment value we intend to convert to -- even if only to check against it. And once we've provided it, we might as well construct our u8 data on the alignment boundary, but then we're just solving the problem a different way that is less semantically clear.

Comment 5

[Markus Stange [:mstange]]

(In reply to Erich Gubler [:ErichDonGubler] from comment #3)

Perhaps propagating the assertion from std to constructors of WrVecU8 could be helpful in tracking down culprit code?

There's only one caller of convert_into_vec<T>, namely wr_resource_updates_add_font_instance. And there's no checking to be done in the constructor of WrVecU8 because WrVecU8 contains u8s which have an alignment of 1. It's only the convert_into_vec<T> method that needs to ensure alignment.

Another way to fix this bug would be to only allow convert_into_vec<T> to be called with types T which have an alignment of 1. We can turn FontVariation into such a type by replacing its u32 and f32 members with [u8; 4] arrays and by adding accessor functions which do the conversion by calling u32::from_ne_bytes and f32::from_ne_bytes.

Comment 6

[Brad Werth [:bradwerth] (out until March 10)]

Attachment: Bug 1888581 Part 1: Make C-side WrVecU8 callers responsible for aligning buffer to intended type.

This ensures that the eventual call in Rust to WrVecU8::convert_into_vec
will succeed, as long as no intervening calls to ::reserve or
::push_bytes cause the Vec to be reallocated. In an ideal world, we
would be able to templatize the FFI functions that call those methods,
but that requires name mangling, making the functions unusable for FFI.

Comment 7

[Markus Stange [:mstange]]

I'm going to attach two alternative patches. The first avoids the memcpy, the second makes us always memcpy FontVariations.
How big are the lists of FontVariations usually? I'm suspecting it won't be in the megabytes range.

Comment 8

[Markus Stange [:mstange]]

Attachment: Bug 1888581 - Make FontVariation unaligned and fix undefined behavior in convert_into_vec. r=jrmuizel

Comment 9

[Markus Stange [:mstange]]

Attachment: Bug 1888581 - Copy FontVariation data instead of doing an unsound cast. r=jrmuizel

Comment 10

[Mike Hommey [:glandium]]

This is now blocking the update to rustc 1.78.

Comment 11

[Brad Werth [:bradwerth] (out until March 10)]

Jeff, time to pick the approach you'd prefer to solve this Bug.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

I've been meaning to talk this over with mstange.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8bf7ce5ab91
Copy FontVariation data instead of doing an unsound cast. r=jrmuizel

Comment 14

[Serban Stanca [:SerbanS]]

Backed out for causing webrender related build bustages.

• Backout link
• Push with failures
• Failure Log
• Failure line: error[E0599]: no method named convert_into_vec found for mutable reference &mut WrVecU8 in the current scope

Comment 15

[Markus Stange [:mstange]]

Oh hello!

#[cfg(target_os = "windows")]
fn read_font_descriptor(bytes: &mut WrVecU8, index: u32) -> NativeFontHandle {
    let wchars = bytes.convert_into_vec::<u16>();

Somehow I missed this caller of convert_into_vec in all my searches.

Comment 16

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&revision=9d2d17f34eac805125be1783c5f41fb386f95011

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a33f8c53d37c
Copy FontVariation data instead of doing an unsound cast. r=jrmuizel

Comment 18

[Norisz Fay [:noriszfay]]

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