☠☠ backed out by e5dd0884d82f ☠ ☠ | |
author | Emilio 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 id | 73976 |
push user | ealvarez@mozilla.com |
push date | Thu, 29 Nov 2018 10:17:16 +0000 (2018-11-29) |
treeherder | autoland@3e5ea9da2cbb [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | jwatt |
bugs | 1510862 |
milestone | 65.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
|
--- 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>