diff --git a/Tests/LibWeb/Ref/reference/svg-invalid-number-arguments-ref.html b/Tests/LibWeb/Ref/reference/svg-invalid-number-arguments-ref.html new file mode 100644 index 0000000000..3f3050b304 --- /dev/null +++ b/Tests/LibWeb/Ref/reference/svg-invalid-number-arguments-ref.html @@ -0,0 +1,28 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/Tests/LibWeb/Ref/reference/svg-path-incomplete-args-ref.html b/Tests/LibWeb/Ref/reference/svg-path-incomplete-args-ref.html new file mode 100644 index 0000000000..fb6f8b45a5 --- /dev/null +++ b/Tests/LibWeb/Ref/reference/svg-path-incomplete-args-ref.html @@ -0,0 +1,30 @@ + +
+ + This should be drawn + +
+
+ + + + + + +
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/Tests/LibWeb/Ref/reference/svg-polygon-with-incomplete-points-ref.html b/Tests/LibWeb/Ref/reference/svg-polygon-with-incomplete-points-ref.html new file mode 100644 index 0000000000..ab577425a3 --- /dev/null +++ b/Tests/LibWeb/Ref/reference/svg-polygon-with-incomplete-points-ref.html @@ -0,0 +1,18 @@ + + + + + + + + + + + + + + + + + + diff --git a/Tests/LibWeb/Ref/svg-invalid-number-arguments.html b/Tests/LibWeb/Ref/svg-invalid-number-arguments.html new file mode 100644 index 0000000000..4c1b83ad0f --- /dev/null +++ b/Tests/LibWeb/Ref/svg-invalid-number-arguments.html @@ -0,0 +1,29 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/Tests/LibWeb/Ref/svg-path-incomplete-args.html b/Tests/LibWeb/Ref/svg-path-incomplete-args.html new file mode 100644 index 0000000000..772481be3b --- /dev/null +++ b/Tests/LibWeb/Ref/svg-path-incomplete-args.html @@ -0,0 +1,99 @@ + + +
+ + + + + This should be drawn + +
+
+ + + + + + +
+
+ + + + +
+
+ + + +
+
+ + + +
+
+ + + +
+
+ + + +
+
+ + + +
+
+ + + +
+
+ + + +
+
+ + + +
+
+ + + +
+
+ + + +
+
+ + + +
+
+ + + +
+
+ + + +
+
+ + + +
+
+ + + +
diff --git a/Tests/LibWeb/Ref/svg-polygon-with-incomplete-points.html b/Tests/LibWeb/Ref/svg-polygon-with-incomplete-points.html new file mode 100644 index 0000000000..6a3e90cb16 --- /dev/null +++ b/Tests/LibWeb/Ref/svg-polygon-with-incomplete-points.html @@ -0,0 +1,29 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/Userland/Libraries/LibWeb/SVG/AttributeParser.cpp b/Userland/Libraries/LibWeb/SVG/AttributeParser.cpp index 2f471300c7..140b4cf43a 100644 --- a/Userland/Libraries/LibWeb/SVG/AttributeParser.cpp +++ b/Userland/Libraries/LibWeb/SVG/AttributeParser.cpp @@ -2,6 +2,7 @@ * Copyright (c) 2020, Matthew Olsson * Copyright (c) 2022, Sam Atkins * Copyright (c) 2023, MacDue + * Copyright (c) 2024, Tim Ledbetter * * SPDX-License-Identifier: BSD-2-Clause */ @@ -29,8 +30,11 @@ Vector AttributeParser::parse_path_data(StringView input) { AttributeParser parser { input }; parser.parse_whitespace(); - while (!parser.done()) - parser.parse_drawto(); + while (!parser.done()) { + auto maybe_error = parser.parse_drawto(); + if (maybe_error.is_error()) + break; + } if (!parser.m_instructions.is_empty() && parser.m_instructions[0].type != PathInstructionType::Move) { // Invalid. "A path data segment (if there is one) must begin with a "moveto" command." return {}; @@ -42,13 +46,12 @@ Optional AttributeParser::parse_coordinate(StringView input) { AttributeParser parser { input }; parser.parse_whitespace(); - if (parser.match_coordinate()) { - float result = parser.parse_coordinate(); - parser.parse_whitespace(); - if (parser.done()) - return result; - } - + auto result_or_error = parser.parse_coordinate(); + if (result_or_error.is_error()) + return {}; + parser.parse_whitespace(); + if (parser.done()) + return result_or_error.value(); return {}; } @@ -56,12 +59,12 @@ Optional AttributeParser::parse_length(StringView input) { AttributeParser parser { input }; parser.parse_whitespace(); - if (parser.match_coordinate()) { - float result = parser.parse_length(); - parser.parse_whitespace(); - if (parser.done()) - return result; - } + auto result_or_error = parser.parse_length(); + if (result_or_error.is_error()) + return {}; + parser.parse_whitespace(); + if (parser.done()) + return result_or_error.value(); return {}; } @@ -77,15 +80,16 @@ Optional AttributeParser::parse_number_percentage(StringView i { AttributeParser parser { input }; parser.parse_whitespace(); - if (parser.match_number()) { - float number = parser.parse_number(); - bool is_percentage = parser.match('%'); - if (is_percentage) - parser.consume(); - parser.parse_whitespace(); - if (parser.done()) - return NumberPercentage(number, is_percentage); - } + + auto number_or_error = parser.parse_number(); + if (number_or_error.is_error()) + return {}; + bool is_percentage = parser.match('%'); + if (is_percentage) + parser.consume(); + parser.parse_whitespace(); + if (parser.done()) + return NumberPercentage(number_or_error.value(), is_percentage); return {}; } @@ -106,16 +110,12 @@ Vector AttributeParser::parse_points(StringView input) parser.parse_whitespace(); - // FIXME: "If an odd number of coordinates is provided, then the element is in error, with the same user agent behavior - // as occurs with an incorrectly specified ‘path’ element. In such error cases the user agent will drop the last, - // odd coordinate and otherwise render the shape." - // The parser currently doesn't notice that there is a missing coordinate, so make it notice! - auto coordinate_pair_sequence = parser.parse_coordinate_pair_sequence(); - - parser.parse_whitespace(); - if (!parser.done()) + auto coordinate_pair_sequence_or_error = parser.parse_coordinate_pair_sequence(); + if (coordinate_pair_sequence_or_error.is_error()) return {}; + auto coordinate_pair_sequence = coordinate_pair_sequence_or_error.release_value(); + // FIXME: This is awkward. Can we return Gfx::FloatPoints from some of these parsing methods instead of Vector? Vector points; points.ensure_capacity(coordinate_pair_sequence.size()); @@ -126,47 +126,50 @@ Vector AttributeParser::parse_points(StringView input) return points; } -void AttributeParser::parse_drawto() +ErrorOr AttributeParser::parse_drawto() { if (match('M') || match('m')) { - parse_moveto(); + return parse_moveto(); } else if (match('Z') || match('z')) { parse_closepath(); + return {}; } else if (match('L') || match('l')) { - parse_lineto(); + return parse_lineto(); } else if (match('H') || match('h')) { - parse_horizontal_lineto(); + return parse_horizontal_lineto(); } else if (match('V') || match('v')) { - parse_vertical_lineto(); + return parse_vertical_lineto(); } else if (match('C') || match('c')) { - parse_curveto(); + return parse_curveto(); } else if (match('S') || match('s')) { - parse_smooth_curveto(); + return parse_smooth_curveto(); } else if (match('Q') || match('q')) { - parse_quadratic_bezier_curveto(); + return parse_quadratic_bezier_curveto(); } else if (match('T') || match('t')) { - parse_smooth_quadratic_bezier_curveto(); + return parse_smooth_quadratic_bezier_curveto(); } else if (match('A') || match('a')) { - parse_elliptical_arc(); - } else { - dbgln("AttributeParser::parse_drawto failed to match: '{}'", ch()); - TODO(); + return parse_elliptical_arc(); } + + dbgln("AttributeParser::parse_drawto failed to match: '{}'", ch()); + return Error::from_string_literal("Invalid drawto command"); } // https://www.w3.org/TR/SVG2/paths.html#PathDataMovetoCommands -void AttributeParser::parse_moveto() +ErrorOr AttributeParser::parse_moveto() { bool absolute = consume() == 'M'; parse_whitespace(); bool is_first = true; - for (auto& pair : parse_coordinate_pair_sequence()) { + for (auto& pair : TRY(parse_coordinate_pair_sequence())) { // NOTE: "M 1 2 3 4" is equivalent to "M 1 2 L 3 4". auto type = is_first ? PathInstructionType::Move : PathInstructionType::Line; m_instructions.append({ type, absolute, pair }); is_first = false; } + + return {}; } void AttributeParser::parse_closepath() @@ -176,128 +179,152 @@ void AttributeParser::parse_closepath() m_instructions.append({ PathInstructionType::ClosePath, absolute, {} }); } -void AttributeParser::parse_lineto() +ErrorOr AttributeParser::parse_lineto() { bool absolute = consume() == 'L'; parse_whitespace(); - for (auto& pair : parse_coordinate_pair_sequence()) + for (auto& pair : TRY(parse_coordinate_pair_sequence())) m_instructions.append({ PathInstructionType::Line, absolute, pair }); + + return {}; } -void AttributeParser::parse_horizontal_lineto() +ErrorOr AttributeParser::parse_horizontal_lineto() { bool absolute = consume() == 'H'; parse_whitespace(); - for (auto coordinate : parse_coordinate_sequence()) + for (auto coordinate : TRY(parse_coordinate_sequence())) m_instructions.append({ PathInstructionType::HorizontalLine, absolute, { coordinate } }); + + return {}; } -void AttributeParser::parse_vertical_lineto() +ErrorOr AttributeParser::parse_vertical_lineto() { bool absolute = consume() == 'V'; parse_whitespace(); - for (auto coordinate : parse_coordinate_sequence()) + for (auto coordinate : TRY(parse_coordinate_sequence())) m_instructions.append({ PathInstructionType::VerticalLine, absolute, { coordinate } }); + + return {}; } -void AttributeParser::parse_curveto() +ErrorOr AttributeParser::parse_curveto() { bool absolute = consume() == 'C'; parse_whitespace(); while (true) { - m_instructions.append({ PathInstructionType::Curve, absolute, parse_coordinate_pair_triplet() }); + m_instructions.append({ PathInstructionType::Curve, absolute, TRY(parse_coordinate_pair_triplet()) }); if (match_comma_whitespace()) parse_comma_whitespace(); if (!match_coordinate()) break; } + + return {}; } -void AttributeParser::parse_smooth_curveto() +ErrorOr AttributeParser::parse_smooth_curveto() { bool absolute = consume() == 'S'; parse_whitespace(); while (true) { - m_instructions.append({ PathInstructionType::SmoothCurve, absolute, parse_coordinate_pair_double() }); + m_instructions.append({ PathInstructionType::SmoothCurve, absolute, TRY(parse_coordinate_pair_double()) }); if (match_comma_whitespace()) parse_comma_whitespace(); if (!match_coordinate()) break; } + + return {}; } -void AttributeParser::parse_quadratic_bezier_curveto() +ErrorOr AttributeParser::parse_quadratic_bezier_curveto() { bool absolute = consume() == 'Q'; parse_whitespace(); while (true) { - m_instructions.append({ PathInstructionType::QuadraticBezierCurve, absolute, parse_coordinate_pair_double() }); + m_instructions.append({ PathInstructionType::QuadraticBezierCurve, absolute, TRY(parse_coordinate_pair_double()) }); if (match_comma_whitespace()) parse_comma_whitespace(); if (!match_coordinate()) break; } + + return {}; } -void AttributeParser::parse_smooth_quadratic_bezier_curveto() +ErrorOr AttributeParser::parse_smooth_quadratic_bezier_curveto() { bool absolute = consume() == 'T'; parse_whitespace(); while (true) { - m_instructions.append({ PathInstructionType::SmoothQuadraticBezierCurve, absolute, parse_coordinate_pair() }); + m_instructions.append({ PathInstructionType::SmoothQuadraticBezierCurve, absolute, TRY(parse_coordinate_pair()) }); if (match_comma_whitespace()) parse_comma_whitespace(); if (!match_coordinate()) break; } + + return {}; } -void AttributeParser::parse_elliptical_arc() +ErrorOr AttributeParser::parse_elliptical_arc() { bool absolute = consume() == 'A'; parse_whitespace(); while (true) { - m_instructions.append({ PathInstructionType::EllipticalArc, absolute, parse_elliptical_arc_argument() }); + auto argument = TRY(parse_elliptical_arc_argument()); + m_instructions.append({ PathInstructionType::EllipticalArc, absolute, move(argument) }); if (match_comma_whitespace()) parse_comma_whitespace(); if (!match_coordinate()) break; } + + return {}; } -float AttributeParser::parse_length() +ErrorOr AttributeParser::parse_length() { // https://www.w3.org/TR/SVG11/types.html#DataTypeLength return parse_number(); } -float AttributeParser::parse_coordinate() +ErrorOr AttributeParser::parse_coordinate() { // https://www.w3.org/TR/SVG11/types.html#DataTypeCoordinate // coordinate ::= length return parse_length(); } -Vector AttributeParser::parse_coordinate_pair() +ErrorOr> AttributeParser::parse_coordinate_pair() { Vector coordinates; - coordinates.append(parse_coordinate()); + coordinates.append(TRY(parse_coordinate())); if (match_comma_whitespace()) parse_comma_whitespace(); - coordinates.append(parse_coordinate()); + coordinates.append(TRY(parse_coordinate())); return coordinates; } -Vector AttributeParser::parse_coordinate_sequence() +ErrorOr> AttributeParser::parse_coordinate_sequence() { Vector sequence; + bool is_first = true; while (true) { - sequence.append(parse_coordinate()); + auto coordinate_or_error = parse_coordinate(); + if (coordinate_or_error.is_error()) { + if (is_first) + return Error::from_string_literal("Expected coordinate sequence"); + } + is_first = false; + sequence.append(coordinate_or_error.release_value()); if (match_comma_whitespace()) parse_comma_whitespace(); if (!match_comma_whitespace() && !match_coordinate()) @@ -306,11 +333,19 @@ Vector AttributeParser::parse_coordinate_sequence() return sequence; } -Vector> AttributeParser::parse_coordinate_pair_sequence() +ErrorOr>> AttributeParser::parse_coordinate_pair_sequence() { Vector> sequence; + bool is_first = true; while (true) { - sequence.append(parse_coordinate_pair()); + auto coordinate_pair_or_error = parse_coordinate_pair(); + if (coordinate_pair_or_error.is_error()) { + if (is_first) + return Error::from_string_literal("Expected coordinate pair sequence"); + break; + } + is_first = false; + sequence.append(coordinate_pair_or_error.release_value()); if (match_comma_whitespace()) parse_comma_whitespace(); if (!match_comma_whitespace() && !match_coordinate()) @@ -319,47 +354,48 @@ Vector> AttributeParser::parse_coordinate_pair_sequence() return sequence; } -Vector AttributeParser::parse_coordinate_pair_double() +ErrorOr> AttributeParser::parse_coordinate_pair_double() { Vector coordinates; - coordinates.extend(parse_coordinate_pair()); + coordinates.extend(TRY(parse_coordinate_pair())); if (match_comma_whitespace()) parse_comma_whitespace(); - coordinates.extend(parse_coordinate_pair()); + coordinates.extend(TRY(parse_coordinate_pair())); return coordinates; } -Vector AttributeParser::parse_coordinate_pair_triplet() +ErrorOr> AttributeParser::parse_coordinate_pair_triplet() { Vector coordinates; - coordinates.extend(parse_coordinate_pair()); + coordinates.extend(TRY(parse_coordinate_pair())); if (match_comma_whitespace()) parse_comma_whitespace(); - coordinates.extend(parse_coordinate_pair()); + coordinates.extend(TRY(parse_coordinate_pair())); if (match_comma_whitespace()) parse_comma_whitespace(); - coordinates.extend(parse_coordinate_pair()); + coordinates.extend(TRY(parse_coordinate_pair())); return coordinates; } -Vector AttributeParser::parse_elliptical_arc_argument() +ErrorOr> AttributeParser::parse_elliptical_arc_argument() { Vector numbers; - numbers.append(parse_number()); + numbers.append(TRY(parse_number())); if (match_comma_whitespace()) parse_comma_whitespace(); - numbers.append(parse_number()); + numbers.append(TRY(parse_number())); if (match_comma_whitespace()) parse_comma_whitespace(); - numbers.append(parse_number()); - parse_comma_whitespace(); - numbers.append(parse_flag()); + numbers.append(TRY(parse_number())); if (match_comma_whitespace()) parse_comma_whitespace(); - numbers.append(parse_flag()); + numbers.append(TRY(parse_flag())); if (match_comma_whitespace()) parse_comma_whitespace(); - numbers.extend(parse_coordinate_pair()); + numbers.append(TRY(parse_flag())); + if (match_comma_whitespace()) + parse_comma_whitespace(); + numbers.extend(TRY(parse_coordinate_pair())); return numbers; } @@ -389,18 +425,19 @@ void AttributeParser::parse_comma_whitespace() } // https://www.w3.org/TR/SVG11/types.html#DataTypeNumber -float AttributeParser::parse_number() +ErrorOr AttributeParser::parse_number() { auto sign = parse_sign(); - return sign * parse_nonnegative_number(); + return sign * TRY(parse_nonnegative_number()); } // https://www.w3.org/TR/SVG11/paths.html#PathDataBNF -float AttributeParser::parse_nonnegative_number() +ErrorOr AttributeParser::parse_nonnegative_number() { // NOTE: The grammar is almost a floating point except we cannot have a sign // at the start. That condition should have been checked by the caller. - VERIFY(!match('+') && !match('-')); + if (match('+') || match('-') || !match_number()) + return Error::from_string_literal("Expected number"); auto remaining_source_text = m_lexer.remaining(); char const* start = remaining_source_text.characters_without_null_termination(); @@ -412,10 +449,10 @@ float AttributeParser::parse_nonnegative_number() return maybe_float.value; } -float AttributeParser::parse_flag() +ErrorOr AttributeParser::parse_flag() { if (!match('0') && !match('1')) - VERIFY_NOT_REACHED(); + return Error::from_string_literal("Expected flag"); return consume() - '0'; } @@ -542,15 +579,17 @@ Optional> AttributeParser::parse_transform() // FIXME: This parsing is quite lenient, so will accept (with default values) some transforms that should be rejected. auto parse_optional_number = [&](float default_value = 0.0f) { consume_comma_whitespace(); - if (match_number()) - return parse_number(); - return default_value; + auto number_or_error = parse_number(); + if (number_or_error.is_error()) + return default_value; + return number_or_error.value(); }; auto try_parse_number = [&]() -> Optional { - if (match_number()) - return parse_number(); - return {}; + auto number_or_error = parse_number(); + if (number_or_error.is_error()) + return {}; + return number_or_error.value(); }; auto parse_function = [&](auto body) -> Optional { @@ -691,7 +730,17 @@ bool AttributeParser::match_number() const bool AttributeParser::match_length() const { - return !done() && (isdigit(ch()) || ch() == '-' || ch() == '+' || ch() == '.'); + if (done()) + return false; + + size_t offset = 0; + if (ch() == '-' || ch() == '+') + offset++; + + if (ch(offset) == '.') + offset++; + + return !done() && isdigit(ch(offset)); } } diff --git a/Userland/Libraries/LibWeb/SVG/AttributeParser.h b/Userland/Libraries/LibWeb/SVG/AttributeParser.h index d7152bb18a..deccb356de 100644 --- a/Userland/Libraries/LibWeb/SVG/AttributeParser.h +++ b/Userland/Libraries/LibWeb/SVG/AttributeParser.h @@ -1,6 +1,7 @@ /* * Copyright (c) 2020, Matthew Olsson * Copyright (c) 2022, Sam Atkins + * Copyright (c) 2024, Tim Ledbetter * * SPDX-License-Identifier: BSD-2-Clause */ @@ -159,33 +160,33 @@ public: private: AttributeParser(StringView source); - void parse_drawto(); - void parse_moveto(); + ErrorOr parse_drawto(); + ErrorOr parse_moveto(); void parse_closepath(); - void parse_lineto(); - void parse_horizontal_lineto(); - void parse_vertical_lineto(); - void parse_curveto(); - void parse_smooth_curveto(); - void parse_quadratic_bezier_curveto(); - void parse_smooth_quadratic_bezier_curveto(); - void parse_elliptical_arc(); + ErrorOr parse_lineto(); + ErrorOr parse_horizontal_lineto(); + ErrorOr parse_vertical_lineto(); + ErrorOr parse_curveto(); + ErrorOr parse_smooth_curveto(); + ErrorOr parse_quadratic_bezier_curveto(); + ErrorOr parse_smooth_quadratic_bezier_curveto(); + ErrorOr parse_elliptical_arc(); Optional> parse_transform(); - float parse_length(); - float parse_coordinate(); - Vector parse_coordinate_pair(); - Vector parse_coordinate_sequence(); - Vector> parse_coordinate_pair_sequence(); - Vector parse_coordinate_pair_double(); - Vector parse_coordinate_pair_triplet(); - Vector parse_elliptical_arc_argument(); + ErrorOr parse_length(); + ErrorOr parse_coordinate(); + ErrorOr> parse_coordinate_pair(); + ErrorOr> parse_coordinate_sequence(); + ErrorOr>> parse_coordinate_pair_sequence(); + ErrorOr> parse_coordinate_pair_double(); + ErrorOr> parse_coordinate_pair_triplet(); + ErrorOr> parse_elliptical_arc_argument(); void parse_whitespace(bool must_match_once = false); void parse_comma_whitespace(); - float parse_number(); - float parse_nonnegative_number(); - float parse_flag(); + ErrorOr parse_number(); + ErrorOr parse_nonnegative_number(); + ErrorOr parse_flag(); // -1 if negative, +1 otherwise int parse_sign(); @@ -197,7 +198,7 @@ private: bool match(char c) const { return !done() && ch() == c; } bool done() const { return m_lexer.is_eof(); } - char ch() const { return m_lexer.peek(); } + char ch(size_t offset = 0) const { return m_lexer.peek(offset); } char consume() { return m_lexer.consume(); } GenericLexer m_lexer;