From b5deea844249948a1912279112fbec9054f42f54 Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Wed, 17 May 2023 19:58:11 +0000 Subject: [PATCH] style: Reject empty svg path string for basic shape path function Per CSS shape spec, the empty path string is invalid. However, for SVG d property, the EBNF allows the path data string in the d property to be empty. We just disable the rendering of the path. Note: only offset-path and clip-path are affected. Differential Revision: https://phabricator.services.mozilla.com/D178123 --- .../style/values/specified/basic_shape.rs | 12 ++-- components/style/values/specified/motion.rs | 6 +- components/style/values/specified/svg.rs | 2 +- components/style/values/specified/svg_path.rs | 68 +++++++++++++------ 4 files changed, 59 insertions(+), 29 deletions(-) diff --git a/components/style/values/specified/basic_shape.rs b/components/style/values/specified/basic_shape.rs index 3c571ff8e84..c085446c642 100644 --- a/components/style/values/specified/basic_shape.rs +++ b/components/style/values/specified/basic_shape.rs @@ -15,8 +15,7 @@ use crate::values::specified::border::BorderRadius; use crate::values::specified::image::Image; use crate::values::specified::position::{HorizontalPosition, Position, VerticalPosition}; use crate::values::specified::url::SpecifiedUrl; -use crate::values::specified::SVGPathData; -use crate::values::specified::{LengthPercentage, NonNegativeLengthPercentage}; +use crate::values::specified::{LengthPercentage, NonNegativeLengthPercentage, SVGPathData}; use crate::Zero; use cssparser::Parser; use style_traits::{ParseError, StyleParseErrorKind}; @@ -294,20 +293,21 @@ impl Polygon { impl Parse for Path { fn parse<'i, 't>( - context: &ParserContext, + _context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { input.expect_function_matching("path")?; - input.parse_nested_block(|i| Self::parse_function_arguments(context, i)) + input.parse_nested_block(Self::parse_function_arguments) } } impl Path { /// Parse the inner arguments of a `path` function. fn parse_function_arguments<'i, 't>( - context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { + use crate::values::specified::svg_path::AllowEmpty; + let fill = input .try_parse(|i| -> Result<_, ParseError> { let fill = FillRule::parse(i)?; @@ -315,7 +315,7 @@ impl Path { Ok(fill) }) .unwrap_or_default(); - let path = SVGPathData::parse(context, input)?; + let path = SVGPathData::parse(input, AllowEmpty::No)?; Ok(Path { fill, path }) } } diff --git a/components/style/values/specified/motion.rs b/components/style/values/specified/motion.rs index 32f4cf677e4..d2831d27b79 100644 --- a/components/style/values/specified/motion.rs +++ b/components/style/values/specified/motion.rs @@ -78,6 +78,8 @@ impl Parse for OffsetPath { context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { + use crate::values::specified::svg_path::AllowEmpty; + // Parse none. if input.try_parse(|i| i.expect_ident_matching("none")).is_ok() { return Ok(OffsetPath::none()); @@ -90,8 +92,8 @@ impl Parse for OffsetPath { match_ignore_ascii_case! { &function, // Bug 1186329: Implement the parser for , , // and . - "path" => SVGPathData::parse(context, i).map(GenericOffsetPath::Path), - "ray" => RayFunction::parse(context, i).map(GenericOffsetPath::Ray), + "path" => SVGPathData::parse(i, AllowEmpty::No).map(OffsetPath::Path), + "ray" => RayFunction::parse(context, i).map(OffsetPath::Ray), _ => { Err(location.new_custom_error( StyleParseErrorKind::UnexpectedFunction(function.clone()) diff --git a/components/style/values/specified/svg.rs b/components/style/values/specified/svg.rs index 73e0b16ac5c..24b7d3f3aec 100644 --- a/components/style/values/specified/svg.rs +++ b/components/style/values/specified/svg.rs @@ -385,7 +385,7 @@ impl Parse for DProperty { // Parse possible functions. input.expect_function_matching("path")?; - let path_data = input.parse_nested_block(|i| SVGPathData::parse(context, i))?; + let path_data = input.parse_nested_block(|i| Parse::parse(context, i))?; Ok(DProperty::Path(path_data)) } } diff --git a/components/style/values/specified/svg_path.rs b/components/style/values/specified/svg_path.rs index 29cab702c98..e05130fa755 100644 --- a/components/style/values/specified/svg_path.rs +++ b/components/style/values/specified/svg_path.rs @@ -5,7 +5,7 @@ //! Specified types for SVG Path. use crate::parser::{Parse, ParserContext}; -use crate::values::animated::{Animate, Procedure, ToAnimatedZero, lists}; +use crate::values::animated::{lists, Animate, Procedure, ToAnimatedZero}; use crate::values::distance::{ComputeSquaredDistance, SquaredDistance}; use crate::values::CSSFloat; use cssparser::Parser; @@ -15,6 +15,14 @@ use std::slice; use style_traits::values::SequenceWriter; use style_traits::{CssWriter, ParseError, StyleParseErrorKind, ToCss}; +/// Whether to allow empty string in the parser. +#[derive(Clone, Debug, Eq, PartialEq)] +#[allow(missing_docs)] +pub enum AllowEmpty { + Yes, + No, +} + /// The SVG path data. /// /// https://www.w3.org/TR/SVG11/paths.html#PathData @@ -160,6 +168,41 @@ impl SVGPathData { Ok(SVGPathData(crate::ArcSlice::from_iter(result.into_iter()))) } + + /// Parse this SVG path string with the argument that indicates whether we should allow the + /// empty string. + // We cannot use cssparser::Parser to parse a SVG path string because the spec wants to make + // the SVG path string as compact as possible. (i.e. The whitespaces may be dropped.) + // e.g. "M100 200L100 200" is a valid SVG path string. If we use tokenizer, the first ident + // is "M100", instead of "M", and this is not correct. Therefore, we use a Peekable + // str::Char iterator to check each character. + pub fn parse<'i, 't>( + input: &mut Parser<'i, 't>, + allow_empty: AllowEmpty, + ) -> Result> { + let location = input.current_source_location(); + let path_string = input.expect_string()?.as_ref(); + + // Parse the svg path string as multiple sub-paths. + let mut path_parser = PathParser::new(path_string); + while skip_wsp(&mut path_parser.chars) { + if path_parser.parse_subpath().is_err() { + return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError)); + } + } + + // The css-shapes-1 says a path data string that does conform but defines an empty path is + // invalid and causes the entire path() to be invalid, so we use the argement to decide + // whether we should allow the empty string. + // https://drafts.csswg.org/css-shapes-1/#typedef-basic-shape + if matches!(allow_empty, AllowEmpty::No) && path_parser.path.is_empty() { + return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError)); + } + + Ok(SVGPathData(crate::ArcSlice::from_iter( + path_parser.path.into_iter(), + ))) + } } impl ToCss for SVGPathData { @@ -180,29 +223,14 @@ impl ToCss for SVGPathData { } impl Parse for SVGPathData { - // We cannot use cssparser::Parser to parse a SVG path string because the spec wants to make - // the SVG path string as compact as possible. (i.e. The whitespaces may be dropped.) - // e.g. "M100 200L100 200" is a valid SVG path string. If we use tokenizer, the first ident - // is "M100", instead of "M", and this is not correct. Therefore, we use a Peekable - // str::Char iterator to check each character. fn parse<'i, 't>( _context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { - let location = input.current_source_location(); - let path_string = input.expect_string()?.as_ref(); - - // Parse the svg path string as multiple sub-paths. - let mut path_parser = PathParser::new(path_string); - while skip_wsp(&mut path_parser.chars) { - if path_parser.parse_subpath().is_err() { - return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError)); - } - } - - Ok(SVGPathData(crate::ArcSlice::from_iter( - path_parser.path.into_iter(), - ))) + // Note that the EBNF allows the path data string in the d property to be empty, so we + // don't reject empty SVG path data. + // https://svgwg.org/svg2-draft/single-page.html#paths-PathDataBNF + SVGPathData::parse(input, AllowEmpty::Yes) } }