servo: Merge #16833 - Fix unsafe AtomicRefCell<T> transmutes in Layout component (from MortimerGoro:layout_alignment); r=SimonSapin
authorImanol Fernandez <mortimergoro@gmail.com>
Fri, 12 May 2017 20:44:02 -0500
changeset 406281 942071ef8ff7adbc9a2cc3d9d8a06aebfff941d2
parent 406280 edea8382d3f45ed2fdd38c42f210a63c139f3be2
child 406282 d1ed0a4e7d97f808c1caa1ebc5116b51d1986621
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersSimonSapin
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #16833 - Fix unsafe AtomicRefCell<T> transmutes in Layout component (from MortimerGoro:layout_alignment); r=SimonSapin <!-- Please describe your changes on the following line: --> Fixes unsafe transmute between `AtomicRefCell<PersistentLayoutData>` and `AtomicRefCell<PartialPersistentLayoutData>` which have different memory alignment in 32 bit archs leading to SEGV crashes. See https://github.com/servo/servo/issues/16817 and https://github.com/servo/servo/pull/16816 mem::align_of values in 32 bit archs (e.g. Android): ``` PersistentLayoutData 8 PersistentLayoutData 4 AtomicRefCell<PersistentLayoutData> 8 AtomicRefCell<PartialPersistentLayoutData> 4 ``` mem::align_of values in 64 bit archs ``` PersistentLayoutData 8 PersistentLayoutData 8 AtomicRefCell<PersistentLayoutData> 8 AtomicRefCell<PartialPersistentLayoutData> 8 ``` --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #16817 (github issue number if applicable). <!-- Either: --> - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 47e4c48feb12e4189f11fd94631f0abea5827f91
servo/Cargo.lock
servo/components/layout/lib.rs
servo/components/script_layout_interface/lib.rs
servo/tests/unit/layout/Cargo.toml
servo/tests/unit/layout/align_of.rs
servo/tests/unit/layout/lib.rs
--- a/servo/Cargo.lock
+++ b/servo/Cargo.lock
@@ -1398,17 +1398,19 @@ dependencies = [
  "unicode-script 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "webrender_traits 0.36.0 (git+https://github.com/servo/webrender)",
 ]
 
 [[package]]
 name = "layout_tests"
 version = "0.0.1"
 dependencies = [
+ "atomic_refcell 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "layout 0.0.1",
+ "script_layout_interface 0.0.1",
 ]
 
 [[package]]
 name = "layout_thread"
 version = "0.0.1"
 dependencies = [
  "app_units 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "euclid 0.11.3 (registry+https://github.com/rust-lang/crates.io-index)",
--- a/servo/components/layout/lib.rs
+++ b/servo/components/layout/lib.rs
@@ -88,16 +88,17 @@ mod table_wrapper;
 mod text;
 pub mod traversal;
 pub mod webrender_helpers;
 pub mod wrapper;
 
 // For unit tests:
 pub use fragment::Fragment;
 pub use fragment::SpecificFragmentInfo;
+pub use self::data::PersistentLayoutData;
 
 /// Returns whether the two arguments point to the same value.
 ///
 /// FIXME: Remove this and use Arc::ptr_eq once we require Rust 1.17
 #[inline]
 pub fn arc_ptr_eq<T: 'static>(a: &::std::sync::Arc<T>, b: &::std::sync::Arc<T>) -> bool {
     ::style::ptr_eq::<T>(&**a, &**b)
 }
--- a/servo/components/script_layout_interface/lib.rs
+++ b/servo/components/script_layout_interface/lib.rs
@@ -45,32 +45,37 @@ use core::nonzero::NonZero;
 use ipc_channel::ipc::IpcSender;
 use libc::c_void;
 use net_traits::image_cache::PendingImageId;
 use script_traits::UntrustedNodeAddress;
 use servo_url::ServoUrl;
 use std::sync::atomic::AtomicIsize;
 use style::data::ElementData;
 
+#[repr(C)]
 pub struct PartialPersistentLayoutData {
     /// Data that the style system associates with a node. When the
     /// style system is being used standalone, this is all that hangs
     /// off the node. This must be first to permit the various
     /// transmutations between ElementData and PersistentLayoutData.
     pub style_data: ElementData,
 
     /// Information needed during parallel traversals.
     pub parallel: DomParallelInfo,
+
+    // Required alignment for safe transmutes between PersistentLayoutData and PartialPersistentLayoutData.
+    _align: [u64; 0]
 }
 
 impl PartialPersistentLayoutData {
     pub fn new() -> Self {
         PartialPersistentLayoutData {
             style_data: ElementData::new(None),
             parallel: DomParallelInfo::new(),
+            _align: [],
         }
     }
 }
 
 #[derive(Copy, Clone, HeapSizeOf)]
 pub struct OpaqueStyleAndLayoutData {
     #[ignore_heap_size_of = "TODO(#6910) Box value that should be counted but \
                              the type lives in layout"]
--- a/servo/tests/unit/layout/Cargo.toml
+++ b/servo/tests/unit/layout/Cargo.toml
@@ -5,9 +5,11 @@ authors = ["The Servo Project Developers
 license = "MPL-2.0"
 
 [lib]
 name = "layout_tests"
 path = "lib.rs"
 doctest = false
 
 [dependencies]
+atomic_refcell = "0.1"
 layout = {path = "../../../components/layout"}
+script_layout_interface = {path = "../../../components/script_layout_interface"}
new file mode 100644
--- /dev/null
+++ b/servo/tests/unit/layout/align_of.rs
@@ -0,0 +1,26 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+use atomic_refcell::AtomicRefCell;
+use layout::PersistentLayoutData;
+use script_layout_interface::PartialPersistentLayoutData;
+use std::mem::align_of;
+
+fn check_layout_alignment(expected: usize, current: usize) {
+    if current != expected {
+        panic!("Your changes have altered the mem alignment of the PartialPersistentLayoutData \
+                struct to {}, but it must match the {}-alignment of PersistentLayoutData struct. \
+                Please fix alignment in components/script_layout_interface/lib.rs",
+                current, expected);
+    }
+}
+
+#[test]
+fn test_persistent_layout_data_alignment() {
+    check_layout_alignment(align_of::<PersistentLayoutData>(),
+                           align_of::<PartialPersistentLayoutData>());
+
+    check_layout_alignment(align_of::<AtomicRefCell<PersistentLayoutData>>(),
+                           align_of::<AtomicRefCell<PartialPersistentLayoutData>>());
+}
--- a/servo/tests/unit/layout/lib.rs
+++ b/servo/tests/unit/layout/lib.rs
@@ -1,7 +1,10 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+extern crate atomic_refcell;
 extern crate layout;
+extern crate script_layout_interface;
 
+#[cfg(test)] mod align_of;
 #[cfg(all(test, target_pointer_width = "64"))] mod size_of;