layout: Don't leak FlowRefs to the flexbox layout code.

This can cause memory safety problems. Comments have been added to
`flow_list.rs` to prevent this from happening again.

components/layout/flex.rs

@@ -15,7 +15,6 @@ use floats::FloatKind;
 use flow;
 use flow::{Flow, FlowClass, ImmutableFlowUtils, OpaqueFlow};
 use flow::{INLINE_POSITION_IS_STATIC, IS_ABSOLUTELY_POSITIONED};
-use flow_ref::{self, FlowRef};
 use fragment::{Fragment, FragmentBorderBoxIterator, Overflow};
 use gfx::display_list::StackingContext;
 use layout_debug;
@@ -113,10 +112,8 @@ struct FlexItem {
     /// The maximal main size. If this property is not actually set by style
     /// It will be the largest size available for code reuse.
     pub max_size: Au,
-    /// Reference to the actual flow.
+    /// The index of the actual flow in our child list.
-    pub flow: FlowRef,
+    pub index: usize,
-    /// Style of the child flow, stored here to reduce overhead.
-    pub style: Arc<ServoComputedValues>,
     /// The 'flex-grow' property of this item.
     pub flex_grow: f32,
     /// The 'flex-shrink' property of this item.
@@ -130,8 +127,8 @@ struct FlexItem {
 }
 
 impl FlexItem {
-    pub fn new(flow: FlowRef) -> FlexItem {
+    pub fn new(index: usize, flow: &Flow) -> FlexItem {
-        let style = flow.as_block().fragment.style.clone();
+        let style = &flow.as_block().fragment.style;
         let flex_grow = style.get_position().flex_grow;
         let flex_shrink = style.get_position().flex_shrink;
         let order = style.get_position().order;
@@ -142,8 +139,7 @@ impl FlexItem {
             base_size: Au(0),
             min_size: Au(0),
             max_size: MAX_AU,
-            flow: flow,
+            index: index,
-            style: style,
             flex_grow: flex_grow,
             flex_shrink: flex_shrink,
             order: order,
@@ -155,15 +151,15 @@ impl FlexItem {
     /// Initialize the used flex base size, minimal main size and maximal main size.
     /// For block mode container this method should be called in assign_block_size()
     /// pass so that the item has already been layouted.
-    pub fn init_sizes(&mut self, containing_length: Au, direction: Direction) {
+    pub fn init_sizes(&mut self, flow: &mut Flow, containing_length: Au, direction: Direction) {
-        let block = flow_ref::deref_mut(&mut self.flow).as_mut_block();
+        let block = flow.as_mut_block();
         match direction {
             // TODO(stshine): the definition of min-{width, height} in style component
             // should change to LengthOrPercentageOrAuto for automatic implied minimal size.
             // https://drafts.csswg.org/css-flexbox-1/#min-size-auto
             Direction::Inline => {
-                let basis = from_flex_basis(self.style.get_position().flex_basis,
+                let basis = from_flex_basis(block.fragment.style.get_position().flex_basis,
-                                            self.style.content_inline_size(),
+                                            block.fragment.style.content_inline_size(),
                                             Some(containing_length));
 
                 // These methods compute auto margins to zero length, which is exactly what we want.
@@ -178,29 +174,31 @@ impl FlexItem {
                     - margin
                     + block.fragment.box_sizing_boundary(direction);
                 self.base_size = basis.specified_or_default(content_size);
-                self.max_size = specified_or_none(self.style.max_inline_size(), containing_length)
+                self.max_size = specified_or_none(block.fragment.style.max_inline_size(),
-                    .unwrap_or(MAX_AU);
+                                                  containing_length).unwrap_or(MAX_AU);
-                self.min_size = specified(self.style.min_inline_size(), containing_length);
+                self.min_size = specified(block.fragment.style.min_inline_size(),
+                                          containing_length);
             }
             Direction::Block => {
-                let basis = from_flex_basis(self.style.get_position().flex_basis,
+                let basis = from_flex_basis(block.fragment.style.get_position().flex_basis,
-                                            self.style.content_block_size(),
+                                            block.fragment.style.content_block_size(),
                                             Some(containing_length));
                 let content_size = block.fragment.border_box.size.block
                     - block.fragment.border_padding.block_start_end()
                     + block.fragment.box_sizing_boundary(direction);
                 self.base_size = basis.specified_or_default(content_size);
-                self.max_size = specified_or_none(self.style.max_block_size(), containing_length)
+                self.max_size = specified_or_none(block.fragment.style.max_block_size(),
-                    .unwrap_or(MAX_AU);
+                                                  containing_length).unwrap_or(MAX_AU);
-                self.min_size = specified(self.style.min_block_size(), containing_length);
+                self.min_size = specified(block.fragment.style.min_block_size(),
+                                          containing_length);
             }
         }
     }
 
     /// Returns the outer main size of the item, including paddings and margins,
     /// clamped by max and min size.
-    pub fn outer_main_size(&self, direction: Direction) -> Au {
+    pub fn outer_main_size(&self, flow: &Flow, direction: Direction) -> Au {
-        let ref fragment = self.flow.as_block().fragment;
+        let ref fragment = flow.as_block().fragment;
         let outer_width = match direction {
             Direction::Inline => {
                 fragment.border_padding.inline_start_end() + fragment.margin.inline_start_end()
@@ -214,8 +212,8 @@ impl FlexItem {
     }
 
     /// Returns the number of auto margins in given direction.
-    pub fn auto_margin_count(&self, direction: Direction) -> i32 {
+    pub fn auto_margin_count(&self, flow: &Flow, direction: Direction) -> i32 {
-        let margin = self.style.logical_margin();
+        let margin = flow.as_block().fragment.style.logical_margin();
         let mut margin_count = 0;
         match direction {
             Direction::Inline => {
@@ -414,12 +412,13 @@ impl FlexFlow {
 
         let items = &mut self.items[start..];
         for mut item in items {
-            item.init_sizes(container_size, self.main_mode);
+            let kid = &mut self.block_flow.base.children[item.index];
-            let outer_main_size = item.outer_main_size(self.main_mode);
+            item.init_sizes(kid, container_size, self.main_mode);
+            let outer_main_size = item.outer_main_size(kid, self.main_mode);
             if total_line_size + outer_main_size > container_size && end != start && self.is_wrappable {
                 break;
             }
-            margin_count += item.auto_margin_count(self.main_mode);
+            margin_count += item.auto_margin_count(kid, self.main_mode);
             total_line_size += outer_main_size;
             end += 1;
         }
@@ -439,8 +438,8 @@ impl FlexFlow {
 
         let mut computation = self.block_flow.fragment.compute_intrinsic_inline_sizes();
         if !fixed_width {
-            for kid in &mut self.items {
+            for kid in self.block_flow.base.children.iter_mut() {
-                let base = flow::mut_base(flow_ref::deref_mut(&mut kid.flow));
+                let base = flow::mut_base(kid);
                 let is_absolutely_positioned = base.flags.contains(IS_ABSOLUTELY_POSITIONED);
                 if !is_absolutely_positioned {
                     let flex_item_inline_sizes = IntrinsicISizes {
@@ -465,8 +464,8 @@ impl FlexFlow {
 
         let mut computation = self.block_flow.fragment.compute_intrinsic_inline_sizes();
         if !fixed_width {
-            for kid in &mut self.items {
+            for kid in self.block_flow.base.children.iter_mut() {
-                let base = flow::mut_base(flow_ref::deref_mut(&mut kid.flow));
+                let base = flow::mut_base(kid);
                 let is_absolutely_positioned = base.flags.contains(IS_ABSOLUTELY_POSITIONED);
                 if !is_absolutely_positioned {
                     computation.content_intrinsic_sizes.minimum_inline_size =
@@ -506,24 +505,22 @@ impl FlexFlow {
             AxisSize::Infinite => content_inline_size
         };
         for kid in &mut self.items {
-            {
+            let kid_base = flow::mut_base(&mut self.block_flow.base.children[kid.index]);
-                let kid_base = flow::mut_base(flow_ref::deref_mut(&mut kid.flow));
+            kid_base.block_container_explicit_block_size = container_block_size;
-                kid_base.block_container_explicit_block_size = container_block_size;
+            if kid_base.flags.contains(INLINE_POSITION_IS_STATIC) {
-                if kid_base.flags.contains(INLINE_POSITION_IS_STATIC) {
+                // The inline-start margin edge of the child flow is at our inline-start content
-                    // The inline-start margin edge of the child flow is at our inline-start content edge,
+                // edge, and its inline-size is our content inline-size.
-                    // and its inline-size is our content inline-size.
+                kid_base.position.start.i =
-                    kid_base.position.start.i =
+                    if kid_base.writing_mode.is_bidi_ltr() == containing_block_mode.is_bidi_ltr() {
-                        if kid_base.writing_mode.is_bidi_ltr() == containing_block_mode.is_bidi_ltr() {
+                        inline_start_content_edge
-                            inline_start_content_edge
+                    } else {
-                        } else {
+                        // The kid's inline 'start' is at the parent's 'end'
-                            // The kid's inline 'start' is at the parent's 'end'
+                        inline_end_content_edge
-                            inline_end_content_edge
+                    };
-                        };
-                }
-                kid_base.block_container_inline_size = container_inline_size;
-                kid_base.block_container_writing_mode = containing_block_mode;
-                kid_base.position.start.i = inline_start_content_edge;
             }
+            kid_base.block_container_inline_size = container_inline_size;
+            kid_base.block_container_writing_mode = containing_block_mode;
+            kid_base.position.start.i = inline_start_content_edge;
         }
     }
 
@@ -602,7 +599,7 @@ impl FlexFlow {
             }
 
             for item in items.iter_mut() {
-                let mut block = flow_ref::deref_mut(&mut item.flow).as_mut_block();
+                let mut block = self.block_flow.base.children[item.index].as_mut_block();
 
                 block.base.block_container_writing_mode = container_mode;
                 block.base.block_container_inline_size = inline_size;
@@ -653,8 +650,8 @@ impl FlexFlow {
         } else {
             self.block_flow.fragment.border_box.size.block
         };
-        for kid in &mut self.items {
+        for item in &mut self.items {
-            let base = flow::mut_base(flow_ref::deref_mut(&mut kid.flow));
+            let mut base = flow::mut_base(&mut self.block_flow.base.children[item.index]);
             if !self.main_reverse {
                 base.position.start.b = cur_b;
                 cur_b = cur_b + base.position.size.block;
@@ -677,9 +674,10 @@ impl FlexFlow {
 
         for line in self.lines.iter_mut() {
             for item in &self.items[line.range.clone()] {
-                let ref fragment = item.flow.as_block().fragment;
+                let fragment = &self.block_flow.base.children[item.index].as_block().fragment;
                 line.cross_size = max(line.cross_size,
-                                      fragment.border_box.size.block + fragment.margin.block_start_end());
+                                      fragment.border_box.size.block +
+                                      fragment.margin.block_start_end());
             }
             total_cross_size += line.cross_size;
         }
@@ -729,9 +727,9 @@ impl FlexFlow {
         }
 
         for line in &self.lines {
-            for mut item in self.items[line.range.clone()].iter_mut() {
+            for item in self.items[line.range.clone()].iter_mut() {
-                let auto_margin_count = item.auto_margin_count(Direction::Block);
+                let block = self.block_flow.base.children[item.index].as_mut_block();
-                let mut block = flow_ref::deref_mut(&mut item.flow).as_mut_block();
+                let auto_margin_count = item.auto_margin_count(block, Direction::Block);
                 let margin = block.fragment.style().logical_margin();
 
                 let mut margin_block_start = block.fragment.margin.block_start;
@@ -823,10 +821,17 @@ impl Flow for FlexFlow {
 
         // Flexbox Section 9.1: Re-order flex items according to their order.
         // FIXME(stshine): This should be done during flow construction.
-        let mut items = self.block_flow.base.children.iter_flow_ref_mut()
+        let mut items: Vec<FlexItem> =
-            .filter(|flow| !flow.as_block().base.flags.contains(IS_ABSOLUTELY_POSITIONED))
+            self.block_flow
-            .map(|flow| FlexItem::new(flow.clone()))
+                .base
-            .collect::<Vec<FlexItem>>();
+                .children
+                .iter()
+                .enumerate()
+                .filter(|&(_, flow)| {
+                    !flow.as_block().base.flags.contains(IS_ABSOLUTELY_POSITIONED)
+                })
+                .map(|(index, flow)| FlexItem::new(index, flow))
+                .collect();
 
         items.sort_by_key(|item| item.order);
         self.items = items;
@@ -914,10 +919,8 @@ impl Flow for FlexFlow {
     fn assign_block_size<'a>(&mut self, layout_context: &'a LayoutContext<'a>) {
         self.block_flow.assign_block_size(layout_context);
         match self.main_mode {
-            Direction::Inline =>
+            Direction::Inline => self.inline_mode_assign_block_size(layout_context),
-                self.inline_mode_assign_block_size(layout_context),
+            Direction::Block => self.block_mode_assign_block_size(),
-            Direction::Block  =>
-                self.block_mode_assign_block_size(layout_context)
         }
     }
 

components/layout/flow_list.rs

@@ -5,10 +5,17 @@
 use flow::Flow;
 use flow_ref::{self, FlowRef};
 use std::collections::{LinkedList, linked_list};
+use std::ops::{Index, IndexMut};
 
-// This needs to be reworked now that we have dynamically-sized types in Rust.
+/// This needs to be reworked now that we have dynamically-sized types in Rust.
-// Until then, it's just a wrapper around LinkedList.
+/// Until then, it's just a wrapper around LinkedList.
-
+///
+/// SECURITY-NOTE(pcwalton): It is very important that `FlowRef` values not leak directly to
+/// layout. Layout code must only interact with `&Flow` or `&mut Flow` values. Otherwise, layout
+/// could stash `FlowRef` values in random places unknown to the system and thereby cause data
+/// races. Those data races can lead to memory safety problems, potentially including arbitrary
+/// remote code execution! In general, do not add new methods to this file (e.g. new ways of
+/// iterating over flows) unless you are *very* sure of what you are doing.
 pub struct FlowList {
     flows: LinkedList<FlowRef>,
 }
@@ -52,13 +59,19 @@ impl FlowList {
         }
     }
 
-    /// Provide a forward iterator
+    /// Provide a forward iterator.
+    ///
+    /// SECURITY-NOTE(pcwalton): This does not hand out `FlowRef`s by design. Do not add a method
+    /// to do so! See the comment above in `FlowList`.
     #[inline]
     pub fn iter<'a>(&'a self) -> impl DoubleEndedIterator<Item = &'a Flow> {
         self.flows.iter().map(|flow| &**flow)
     }
 
     /// Provide a forward iterator with mutable references
+    ///
+    /// SECURITY-NOTE(pcwalton): This does not hand out `FlowRef`s by design. Do not add a method
+    /// to do so! See the comment above in `FlowList`.
     #[inline]
     pub fn iter_mut(&mut self) -> MutFlowListIterator {
         MutFlowListIterator {
@@ -66,13 +79,6 @@ impl FlowList {
         }
     }
 
-    /// Provide a forward iterator with FlowRef items
-    #[inline]
-    pub fn iter_flow_ref_mut<'a>(&'a mut self)
-                                 -> impl DoubleEndedIterator<Item = &'a mut FlowRef> {
-        self.flows.iter_mut()
-    }
-
     /// O(1)
     #[inline]
     pub fn is_empty(&self) -> bool {
@@ -93,6 +99,21 @@ impl FlowList {
     }
 }
 
+impl Index<usize> for FlowList {
+    /// FIXME(pcwalton): O(n)!
+    type Output = Flow;
+    fn index(&self, index: usize) -> &Flow {
+        &**self.flows.iter().nth(index).unwrap()
+    }
+}
+
+impl IndexMut<usize> for FlowList {
+    /// FIXME(pcwalton): O(n)!
+    fn index_mut(&mut self, index: usize) -> &mut Flow {
+        self.iter_mut().nth(index).unwrap()
+    }
+}
+
 impl<'a> DoubleEndedIterator for MutFlowListIterator<'a> {
     fn next_back(&mut self) -> Option<&'a mut Flow> {
         self.it.next_back().map(flow_ref::deref_mut)