Bug 1549969 - Don't panic when creating empty ThinArcs. r=bholley
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 08 May 2019 17:54:25 +0000
changeset 531918 82ae6ce8f3a8e18ab661b8d85fc96649a28694b5
parent 531917 89ab5386f54bc1f39b5ea368cc3e6f0d2952e410
child 531919 0ba7b9f6fdafcd2a2ba6a13706a8d419d7d64f5b
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs1549969
milestone68.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
Bug 1549969 - Don't panic when creating empty ThinArcs. r=bholley The change from [T; 1] to [T; 0] shouldn't change behavior since they have the right alignment and we never poke at that particular array, but feels more correct to avoid creating types that point to uninitialized data or outside of their allocation, now that we allow empty slices. Differential Revision: https://phabricator.services.mozilla.com/D30352
servo/components/servo_arc/lib.rs
--- a/servo/components/servo_arc/lib.rs
+++ b/servo/components/servo_arc/lib.rs
@@ -163,17 +163,17 @@ unsafe impl<T: ?Sized + Sync + Send> Sen
 unsafe impl<T: ?Sized + Sync + Send> Sync for ArcInner<T> {}
 
 impl<T> Arc<T> {
     /// Construct an `Arc<T>`
     #[inline]
     pub fn new(data: T) -> Self {
         let x = Box::new(ArcInner {
             count: atomic::AtomicUsize::new(1),
-            data: data,
+            data,
         });
         unsafe {
             Arc {
                 p: ptr::NonNull::new_unchecked(Box::into_raw(x)),
                 phantom: PhantomData,
             }
         }
     }
@@ -605,17 +605,17 @@ impl<H, T> Arc<HeaderSlice<H, [T]>> {
         use std::mem::size_of;
         assert_ne!(size_of::<T>(), 0, "Need to think about ZST");
 
         // Compute the required size for the allocation.
         let num_items = items.len();
         let size = {
             // First, determine the alignment of a hypothetical pointer to a
             // HeaderSlice.
-            let fake_slice_ptr_align: usize = mem::align_of::<ArcInner<HeaderSlice<H, [T; 1]>>>();
+            let fake_slice_ptr_align: usize = mem::align_of::<ArcInner<HeaderSlice<H, [T; 0]>>>();
 
             // Next, synthesize a totally garbage (but properly aligned) pointer
             // to a sequence of T.
             let fake_slice_ptr = fake_slice_ptr_align as *const T;
 
             // Convert that sequence to a fat pointer. The address component of
             // the fat pointer will be garbage, but the length will be correct.
             let fake_slice = unsafe { slice::from_raw_parts(fake_slice_ptr, num_items) };
@@ -662,33 +662,34 @@ impl<H, T> Arc<HeaderSlice<H, [T]>> {
             // we'll just leak the uninitialized memory.
             let count = if is_static {
                 atomic::AtomicUsize::new(STATIC_REFCOUNT)
             } else {
                 atomic::AtomicUsize::new(1)
             };
             ptr::write(&mut ((*ptr).count), count);
             ptr::write(&mut ((*ptr).data.header), header);
-            let mut current: *mut T = &mut (*ptr).data.slice[0];
-            for _ in 0..num_items {
-                ptr::write(
-                    current,
-                    items
-                        .next()
-                        .expect("ExactSizeIterator over-reported length"),
-                );
-                current = current.offset(1);
+            if num_items != 0 {
+                let mut current: *mut T = &mut (*ptr).data.slice[0];
+                for _ in 0..num_items {
+                    ptr::write(
+                        current,
+                        items
+                            .next()
+                            .expect("ExactSizeIterator over-reported length"),
+                    );
+                    current = current.offset(1);
+                }
+                // We should have consumed the buffer exactly.
+                debug_assert_eq!(current as *mut u8, buffer.offset(size as isize));
             }
             assert!(
                 items.next().is_none(),
                 "ExactSizeIterator under-reported length"
             );
-
-            // We should have consumed the buffer exactly.
-            debug_assert_eq!(current as *mut u8, buffer.offset(size as isize));
         }
 
         // Return the fat Arc.
         assert_eq!(
             size_of::<Self>(),
             size_of::<usize>() * 2,
             "The Arc will be fat"
         );
@@ -767,32 +768,34 @@ type HeaderSliceWithLength<H, T> = Heade
 ///
 /// When you create an `Arc` containing a dynamically sized type
 /// like `HeaderSlice<H, [T]>`, the `Arc` is represented on the stack
 /// as a "fat pointer", where the length of the slice is stored
 /// alongside the `Arc`'s pointer. In some situations you may wish to
 /// have a thin pointer instead, perhaps for FFI compatibility
 /// or space efficiency.
 ///
+/// Note that we use `[T; 0]` in order to have the right alignment for `T`.
+///
 /// `ThinArc` solves this by storing the length in the allocation itself,
 /// via `HeaderSliceWithLength`.
 #[repr(C)]
 pub struct ThinArc<H, T> {
-    ptr: ptr::NonNull<ArcInner<HeaderSliceWithLength<H, [T; 1]>>>,
+    ptr: ptr::NonNull<ArcInner<HeaderSliceWithLength<H, [T; 0]>>>,
     phantom: PhantomData<(H, T)>,
 }
 
 unsafe impl<H: Sync + Send, T: Sync + Send> Send for ThinArc<H, T> {}
 unsafe impl<H: Sync + Send, T: Sync + Send> Sync for ThinArc<H, T> {}
 
 // Synthesize a fat pointer from a thin pointer.
 //
 // See the comment around the analogous operation in from_header_and_iter.
 fn thin_to_thick<H, T>(
-    thin: *mut ArcInner<HeaderSliceWithLength<H, [T; 1]>>,
+    thin: *mut ArcInner<HeaderSliceWithLength<H, [T; 0]>>,
 ) -> *mut ArcInner<HeaderSliceWithLength<H, [T]>> {
     let len = unsafe { (*thin).data.header.length };
     let fake_slice: *mut [T] = unsafe { slice::from_raw_parts_mut(thin as *mut T, len) };
 
     fake_slice as *mut ArcInner<HeaderSliceWithLength<H, [T]>>
 }
 
 impl<H, T> ThinArc<H, T> {
@@ -900,17 +903,17 @@ impl<H, T> Arc<HeaderSliceWithLength<H, 
             a.slice.len(),
             "Length needs to be correct for ThinArc to work"
         );
         let fat_ptr: *mut ArcInner<HeaderSliceWithLength<H, [T]>> = a.ptr();
         mem::forget(a);
         let thin_ptr = fat_ptr as *mut [usize] as *mut usize;
         ThinArc {
             ptr: unsafe {
-                ptr::NonNull::new_unchecked(thin_ptr as *mut ArcInner<HeaderSliceWithLength<H, [T; 1]>>)
+                ptr::NonNull::new_unchecked(thin_ptr as *mut ArcInner<HeaderSliceWithLength<H, [T; 0]>>)
             },
             phantom: PhantomData,
         }
     }
 
     /// Converts a `ThinArc` into an `Arc`. This consumes the `ThinArc`, so the refcount
     /// is not modified.
     #[inline]
@@ -1316,16 +1319,27 @@ mod tests {
         fn drop(&mut self) {
             unsafe {
                 (*self.0).fetch_add(1, SeqCst);
             }
         }
     }
 
     #[test]
+    fn empty_thin() {
+        let header = HeaderWithLength::new(100u32, 0);
+        let x = Arc::from_header_and_iter(header, std::iter::empty::<i32>());
+        let y = Arc::into_thin(x.clone());
+        assert_eq!(y.header.header, 100);
+        assert!(y.slice.is_empty());
+        assert_eq!(x.header.header, 100);
+        assert!(x.slice.is_empty());
+    }
+
+    #[test]
     fn slices_and_thin() {
         let mut canary = atomic::AtomicUsize::new(0);
         let c = Canary(&mut canary as *mut atomic::AtomicUsize);
         let v = vec![5, 6];
         let header = HeaderWithLength::new(c, v.len());
         {
             let x = Arc::into_thin(Arc::from_header_and_iter(header, v.into_iter()));
             let y = ThinArc::with_arc(&x, |q| q.clone());