Bug 1510862 - Prevent exponential blowup of custom properties. r=jwatt
☠☠ backed out by e5dd0884d82f ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 29 Nov 2018 10:16:38 +0000 (2018-11-29)
changeset 448721 3e5ea9da2cbbb0e9ac5d27a277f921ca9d470c85
parent 448720 dcec04522fb6d455270c8fe5059660e83574712c
child 448722 04f0bbf40bf36957dc1f72a8aae9916df0e3222f
push id73976
push userealvarez@mozilla.com
push dateThu, 29 Nov 2018 10:17:16 +0000 (2018-11-29)
treeherderautoland@3e5ea9da2cbb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjwatt
bugs1510862
milestone65.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 1510862 - Prevent exponential blowup of custom properties. r=jwatt Put a hard cap on the value length instead of counting substitutions, because it works best, see the comment. Differential Revision: https://phabricator.services.mozilla.com/D13352
servo/components/style/custom_properties.rs
testing/web-platform/tests/css/css-variables/variable-exponential-blowup.html
--- a/servo/components/style/custom_properties.rs
+++ b/servo/components/style/custom_properties.rs
@@ -278,55 +278,80 @@ impl VariableValue {
             css: String::new(),
             last_token_type: TokenSerializationType::nothing(),
             first_token_type: TokenSerializationType::nothing(),
             references: Default::default(),
             references_environment: false,
         }
     }
 
-    fn push(
+    fn push<'i>(
         &mut self,
+        input: &Parser<'i, '_>,
         css: &str,
         css_first_token_type: TokenSerializationType,
         css_last_token_type: TokenSerializationType,
-    ) {
+    ) -> Result<(), ParseError<'i>> {
+        /// Prevent values from getting terribly big since you can use custom
+        /// properties exponentially.
+        ///
+        /// This number (1MB) is somewhat arbitrary, but silly enough that no
+        /// sane page would hit it. We could limit by number of total
+        /// substitutions, but that was very easy to work around in practice
+        /// (just choose a larger initial value and boom).
+        const MAX_VALUE_LENGTH_IN_BYTES: usize = 1024 * 1024;
+
+        if self.css.len() + css.len() > MAX_VALUE_LENGTH_IN_BYTES {
+            return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
+        }
+
         // This happens e.g. between two subsequent var() functions:
         // `var(--a)var(--b)`.
         //
         // In that case, css_*_token_type is nonsensical.
         if css.is_empty() {
-            return;
+            return Ok(());
         }
 
         self.first_token_type.set_if_nothing(css_first_token_type);
         // If self.first_token_type was nothing,
         // self.last_token_type is also nothing and this will be false:
         if self
             .last_token_type
             .needs_separator_when_before(css_first_token_type)
         {
             self.css.push_str("/**/")
         }
         self.css.push_str(css);
-        self.last_token_type = css_last_token_type
+        self.last_token_type = css_last_token_type;
+        Ok(())
     }
 
-    fn push_from(
+    fn push_from<'i>(
         &mut self,
+        input: &Parser<'i, '_>,
         position: (SourcePosition, TokenSerializationType),
-        input: &Parser,
         last_token_type: TokenSerializationType,
-    ) {
-        self.push(input.slice_from(position.0), position.1, last_token_type)
+    ) -> Result<(), ParseError<'i>> {
+        self.push(
+            input,
+            input.slice_from(position.0),
+            position.1,
+            last_token_type,
+        )
     }
 
-    fn push_variable(&mut self, variable: &ComputedValue) {
+    fn push_variable<'i>(
+        &mut self,
+        input: &Parser<'i, '_>,
+        variable: &ComputedValue,
+    ) -> Result<(), ParseError<'i>> {
         debug_assert!(variable.references.is_empty());
         self.push(
+            input,
             &variable.css,
             variable.first_token_type,
             variable.last_token_type,
         )
     }
 
     /// Parse a custom property value.
     pub fn parse<'i, 't>(input: &mut Parser<'i, 't>) -> Result<Arc<Self>, ParseError<'i>> {
@@ -722,30 +747,32 @@ impl<'a> CustomPropertiesBuilder<'a> {
 fn substitute_all(custom_properties_map: &mut CustomPropertiesMap, environment: &CssEnvironment) {
     // The cycle dependencies removal in this function is a variant
     // of Tarjan's algorithm. It is mostly based on the pseudo-code
     // listed in
     // https://en.wikipedia.org/w/index.php?
     // title=Tarjan%27s_strongly_connected_components_algorithm&oldid=801728495
 
     /// Struct recording necessary information for each variable.
+    #[derive(Debug)]
     struct VarInfo {
         /// The name of the variable. It will be taken to save addref
         /// when the corresponding variable is popped from the stack.
         /// This also serves as a mark for whether the variable is
         /// currently in the stack below.
         name: Option<Name>,
         /// If the variable is in a dependency cycle, lowlink represents
         /// a smaller index which corresponds to a variable in the same
         /// strong connected component, which is known to be accessible
         /// from this variable. It is not necessarily the root, though.
         lowlink: usize,
     }
     /// Context struct for traversing the variable graph, so that we can
     /// avoid referencing all the fields multiple times.
+    #[derive(Debug)]
     struct Context<'a> {
         /// Number of variables visited. This is used as the order index
         /// when we visit a new unresolved variable.
         count: usize,
         /// The map from custom property name to its order index.
         index_map: PrecomputedHashMap<Name, usize>,
         /// Information of each variable indexed by the order index.
         var_info: SmallVec<[VarInfo; 5]>,
@@ -936,32 +963,32 @@ fn substitute_references_in_value<'i>(
     let last_token_type = substitute_block(
         &mut input,
         &mut position,
         &mut computed_value,
         custom_properties,
         environment,
     )?;
 
-    computed_value.push_from(position, &input, last_token_type);
+    computed_value.push_from(&input, position, last_token_type)?;
     Ok(computed_value)
 }
 
 /// Replace `var()` functions in an arbitrary bit of input.
 ///
 /// If the variable has its initial value, the callback should return `Err(())`
 /// and leave `partial_computed_value` unchanged.
 ///
 /// Otherwise, it should push the value of the variable (with its own `var()` functions replaced)
 /// to `partial_computed_value` and return `Ok(last_token_type of what was pushed)`
 ///
 /// Return `Err(())` if `input` is invalid at computed-value time.
 /// or `Ok(last_token_type that was pushed to partial_computed_value)` otherwise.
-fn substitute_block<'i, 't>(
-    input: &mut Parser<'i, 't>,
+fn substitute_block<'i>(
+    input: &mut Parser<'i, '_>,
     position: &mut (SourcePosition, TokenSerializationType),
     partial_computed_value: &mut ComputedValue,
     custom_properties: &CustomPropertiesMap,
     env: &CssEnvironment,
 ) -> Result<TokenSerializationType, ParseError<'i>> {
     let mut last_token_type = TokenSerializationType::nothing();
     let mut set_position_at_next_iteration = false;
     loop {
@@ -986,20 +1013,21 @@ fn substitute_block<'i, 't>(
         };
         match token {
             Token::Function(ref name)
                 if name.eq_ignore_ascii_case("var") || name.eq_ignore_ascii_case("env") =>
             {
                 let is_env = name.eq_ignore_ascii_case("env");
 
                 partial_computed_value.push(
+                    input,
                     input.slice(position.0..before_this_token),
                     position.1,
                     last_token_type,
-                );
+                )?;
                 input.parse_nested_block(|input| {
                     // parse_var_function() / parse_env_function() ensure neither .unwrap() will fail.
                     let name = {
                         let name = input.expect_ident().unwrap();
                         if is_env {
                             Atom::from(&**name)
                         } else {
                             Atom::from(parse_name(&name).unwrap())
@@ -1009,17 +1037,17 @@ fn substitute_block<'i, 't>(
                     let value = if is_env {
                         env.get(&name)
                     } else {
                         custom_properties.get(&name).map(|v| &**v)
                     };
 
                     if let Some(v) = value {
                         last_token_type = v.last_token_type;
-                        partial_computed_value.push_variable(v);
+                        partial_computed_value.push_variable(input, v)?;
                         // Skip over the fallback, as `parse_nested_block` would return `Err`
                         // if we don't consume all of `input`.
                         // FIXME: Add a specialized method to cssparser to do this with less work.
                         while input.next().is_ok() {}
                     } else {
                         input.expect_comma()?;
                         let after_comma = input.state();
                         let first_token_type = input
@@ -1031,17 +1059,17 @@ fn substitute_block<'i, 't>(
                         let mut position = (after_comma.position(), first_token_type);
                         last_token_type = substitute_block(
                             input,
                             &mut position,
                             partial_computed_value,
                             custom_properties,
                             env,
                         )?;
-                        partial_computed_value.push_from(position, input, last_token_type);
+                        partial_computed_value.push_from(input, position, last_token_type)?;
                     }
                     Ok(())
                 })?;
                 set_position_at_next_iteration = true
             }
             Token::Function(_) |
             Token::ParenthesisBlock |
             Token::CurlyBracketBlock |
@@ -1091,11 +1119,11 @@ pub fn substitute<'i>(
     };
     let last_token_type = substitute_block(
         &mut input,
         &mut position,
         &mut substituted,
         &custom_properties,
         env,
     )?;
-    substituted.push_from(position, &input, last_token_type);
+    substituted.push_from(&input, position, last_token_type)?;
     Ok(substituted.css)
 }
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-variables/variable-exponential-blowup.html
@@ -0,0 +1,27 @@
+<!doctype html>
+<title>CSS Variables Test: Exponential blowup doesn't crash</title>
+<meta charset="UTF-8">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<link rel="author" href="mailto:emilio@crisal.io" title="Emilio Cobos Álvarez">
+<link rel="author" href="https://mozilla.org" title="Mozilla">
+<script>
+  let css = `
+    --v0: "Something really really really long";
+  `;
+  for (let i = 0; i < 30; ++i)
+    css += `--v${i + 1}: var(--v${i}), var(--v${i});`;
+  let s = document.createElement("style");
+  s.innerHTML = `
+    :root { ${css}; }
+    :root::before { content: var(--v31); }
+  `;
+  document.head.appendChild(s);
+</script>
+PASS if doesn't crash
+<script>
+  test(function() {
+    getComputedStyle(document.documentElement, "::before").content;
+    assert_true(true, "Didn't crash");
+  });
+</script>