From de6e9c89808ff8e25439bf1af2430d3261f7fd6b Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Sun, 10 May 2026 23:21:35 +1000 Subject: [PATCH 1/2] Harden SVG path parser and add tests. Fix #385 --- src/ImageSharp.Drawing/Path.cs | 229 ++++++++++++------ .../Issues/Issue_385.cs | 13 + 2 files changed, 162 insertions(+), 80 deletions(-) create mode 100644 tests/ImageSharp.Drawing.Tests/Issues/Issue_385.cs diff --git a/src/ImageSharp.Drawing/Path.cs b/src/ImageSharp.Drawing/Path.cs index 8092883c..263fa060 100644 --- a/src/ImageSharp.Drawing/Path.cs +++ b/src/ImageSharp.Drawing/Path.cs @@ -370,8 +370,9 @@ public static bool TryParseSvgPath(ReadOnlySpan svgPath, [NotNullWhen(true char ch = svgPath[0]; if (char.IsDigit(ch) || ch == '-' || ch == '+' || ch == '.') { - // Are we are the end of the string or we are at the end of the path? - if (svgPath.Length == 0 || op == 'Z') + // SVG allows repeated operand groups to reuse the previous command. + // A leading number is only valid once a drawable command is active. + if (op is '\0' or 'Z') { return false; } @@ -393,107 +394,164 @@ public static bool TryParseSvgPath(ReadOnlySpan svgPath, [NotNullWhen(true svgPath = TrimSeparator(svgPath[1..]); } + // Read every operand for the command before appending geometry. That keeps + // malformed or truncated data from leaking a partially parsed segment into the path. switch (op) { case 'M': - svgPath = FindPoint(svgPath, relative, c, out point1); - builder.MoveTo(point1); + if (!TryFindPoint(ref svgPath, relative, c, out point1)) + { + return false; + } + + _ = builder.MoveTo(point1); previousOp = '\0'; + + // Extra coordinate pairs after a move command are implicit line commands. op = 'L'; c = point1; break; case 'L': - svgPath = FindPoint(svgPath, relative, c, out point1); - builder.LineTo(point1); + if (!TryFindPoint(ref svgPath, relative, c, out point1)) + { + return false; + } + + _ = builder.LineTo(point1); c = point1; break; case 'H': - svgPath = FindScaler(svgPath, out float x); + if (!TryFindScaler(ref svgPath, out float x)) + { + return false; + } + if (relative) { x += c.X; } - builder.LineTo(x, c.Y); + if (!float.IsFinite(x)) + { + return false; + } + + _ = builder.LineTo(x, c.Y); c.X = x; break; case 'V': - svgPath = FindScaler(svgPath, out float y); + if (!TryFindScaler(ref svgPath, out float y)) + { + return false; + } + if (relative) { y += c.Y; } - builder.LineTo(c.X, y); + if (!float.IsFinite(y)) + { + return false; + } + + _ = builder.LineTo(c.X, y); c.Y = y; break; case 'C': - svgPath = FindPoint(svgPath, relative, c, out point1); - svgPath = FindPoint(svgPath, relative, c, out point2); - svgPath = FindPoint(svgPath, relative, c, out point3); - builder.CubicBezierTo(point1, point2, point3); + if (!TryFindPoint(ref svgPath, relative, c, out point1) + || !TryFindPoint(ref svgPath, relative, c, out point2) + || !TryFindPoint(ref svgPath, relative, c, out point3)) + { + return false; + } + + _ = builder.CubicBezierTo(point1, point2, point3); lastc = point2; c = point3; break; case 'S': - svgPath = FindPoint(svgPath, relative, c, out point2); - svgPath = FindPoint(svgPath, relative, c, out point3); + if (!TryFindPoint(ref svgPath, relative, c, out point2) + || !TryFindPoint(ref svgPath, relative, c, out point3)) + { + return false; + } + point1 = c; if (previousOp is 'C' or 'S') { + // Smooth cubic curves mirror the previous cubic control point. + // Without a preceding cubic command, the current point is the control point. point1.X -= lastc.X - c.X; point1.Y -= lastc.Y - c.Y; } - builder.CubicBezierTo(point1, point2, point3); + _ = builder.CubicBezierTo(point1, point2, point3); lastc = point2; c = point3; break; case 'Q': // Quadratic Bezier Curve - svgPath = FindPoint(svgPath, relative, c, out point1); - svgPath = FindPoint(svgPath, relative, c, out point2); - builder.QuadraticBezierTo(point1, point2); + if (!TryFindPoint(ref svgPath, relative, c, out point1) + || !TryFindPoint(ref svgPath, relative, c, out point2)) + { + return false; + } + + _ = builder.QuadraticBezierTo(point1, point2); lastc = point1; c = point2; break; case 'T': - svgPath = FindPoint(svgPath, relative, c, out point2); + if (!TryFindPoint(ref svgPath, relative, c, out point2)) + { + return false; + } + point1 = c; if (previousOp is 'Q' or 'T') { + // Smooth quadratic curves mirror the previous quadratic control point. + // Without a preceding quadratic command, the current point is the control point. point1.X -= lastc.X - c.X; point1.Y -= lastc.Y - c.Y; } - builder.QuadraticBezierTo(point1, point2); + _ = builder.QuadraticBezierTo(point1, point2); lastc = point1; c = point2; break; case 'A': - if (TryFindScaler(ref svgPath, out float radiiX) - && TryTrimSeparator(ref svgPath) - && TryFindScaler(ref svgPath, out float radiiY) - && TryTrimSeparator(ref svgPath) - && TryFindScaler(ref svgPath, out float angle) - && TryTrimSeparator(ref svgPath) - && TryFindScaler(ref svgPath, out float largeArc) - && TryTrimSeparator(ref svgPath) - && TryFindScaler(ref svgPath, out float sweep) - && TryFindPoint(ref svgPath, relative, c, out PointF point)) + // Arc flags are single SVG grammar tokens, not numbers. Reading them as + // scalars would accept malformed flag/end-point boundaries such as "04445". + if (!TryFindScaler(ref svgPath, out float radiiX) + || !TryTrimSeparator(ref svgPath) + || !TryFindScaler(ref svgPath, out float radiiY) + || !TryTrimSeparator(ref svgPath) + || !TryFindScaler(ref svgPath, out float angle) + || !TryTrimSeparator(ref svgPath) + || !TryFindFlag(ref svgPath, out bool largeArc) + || !TryTrimSeparator(ref svgPath) + || !TryFindFlag(ref svgPath, out bool sweep) + || !TryFindPoint(ref svgPath, relative, c, out PointF point)) { - builder.ArcTo(radiiX, radiiY, angle, largeArc == 1, sweep == 1, point); - c = point; + return false; } + _ = builder.ArcTo(radiiX, radiiY, angle, largeArc, sweep, point); + c = point; break; case 'Z': - builder.CloseFigure(); + _ = builder.CloseFigure(); c = first; break; case '~': - svgPath = FindPoint(svgPath, relative, c, out point1); - svgPath = FindPoint(svgPath, relative, c, out point2); - builder.MoveTo(point1).LineTo(point2); + if (!TryFindPoint(ref svgPath, relative, c, out point1) + || !TryFindPoint(ref svgPath, relative, c, out point2)) + { + return false; + } + + _ = builder.MoveTo(point1).LineTo(point2); break; default: return false; @@ -511,8 +569,27 @@ public static bool TryParseSvgPath(ReadOnlySpan svgPath, [NotNullWhen(true return true; } + private static bool TryFindFlag(ref ReadOnlySpan str, out bool value) + { + str = TrimSeparator(str); + + // https://www.w3.org/TR/SVG11/paths.html#PathDataBNF + // flag: "0" | "1" + if (str.Length == 0 || (str[0] is not '0' and not '1')) + { + value = default; + return false; + } + + value = str[0] == '1'; + str = str[1..]; + return true; + } + private static bool TryTrimSeparator(ref ReadOnlySpan str) { + // SVG separators are optional in places where the next token can be + // recognized unambiguously. Keep this chainable with the operand readers. ReadOnlySpan result = TrimSeparator(str); if (str[^result.Length..].StartsWith(result)) { @@ -525,11 +602,10 @@ private static bool TryTrimSeparator(ref ReadOnlySpan str) private static bool TryFindScaler(ref ReadOnlySpan str, out float value) { - ReadOnlySpan result = FindScaler(str, out float valueInner); - if (str[^result.Length..].StartsWith(result)) + ReadOnlySpan source = TrimSeparator(str); + if (TryReadScalar(source, out value, out int length)) { - value = valueInner; - str = result; + str = source[length..]; return true; } @@ -539,11 +615,23 @@ private static bool TryFindScaler(ref ReadOnlySpan str, out float value) private static bool TryFindPoint(ref ReadOnlySpan str, bool relative, PointF current, out PointF value) { - ReadOnlySpan result = FindPoint(str, relative, current, out PointF valueInner); - if (str[^result.Length..].StartsWith(result)) + if (TryFindScaler(ref str, out float x) && TryFindScaler(ref str, out float y)) { - value = valueInner; - str = result; + // Relative operands can overflow after adding the current point even when + // each parsed scalar is finite, so validate the absolute result as well. + if (relative) + { + x += current.X; + y += current.Y; + } + + if (!float.IsFinite(x) || !float.IsFinite(y)) + { + value = default; + return false; + } + + value = new PointF(x, y); return true; } @@ -551,25 +639,10 @@ private static bool TryFindPoint(ref ReadOnlySpan str, bool relative, Poin return false; } - private static ReadOnlySpan FindPoint(ReadOnlySpan str, bool isRelative, PointF relative, out PointF value) - { - str = FindScaler(str, out float x); - str = FindScaler(str, out float y); - if (isRelative) - { - x += relative.X; - y += relative.Y; - } - - value = new PointF(x, y); - return str; - } - - private static ReadOnlySpan FindScaler(ReadOnlySpan str, out float scaler) + private static bool TryReadScalar(ReadOnlySpan str, out float scaler, out int length) { - str = TrimSeparator(str); - scaler = 0; - + // SVG path numbers can be tightly packed: "10-20" is two numbers, as is + // "0.5.6". Stop at the first character that belongs to the next token. bool hasDot = false; for (int i = 0; i < str.Length; i++) { @@ -577,8 +650,8 @@ private static ReadOnlySpan FindScaler(ReadOnlySpan str, out float s if (IsSeparator(ch)) { - scaler = ParseFloat(str[..i]); - return str[i..]; + length = i; + return TryParseFloat(str[..length], out scaler); } if (ch == '.') @@ -586,8 +659,8 @@ private static ReadOnlySpan FindScaler(ReadOnlySpan str, out float s if (hasDot) { // Second decimal point starts a new number. - scaler = ParseFloat(str[..i]); - return str[i..]; + length = i; + return TryParseFloat(str[..length], out scaler); } hasDot = true; @@ -599,24 +672,20 @@ private static ReadOnlySpan FindScaler(ReadOnlySpan str, out float s char prev = str[i - 1]; if (prev is not 'e' and not 'E') { - scaler = ParseFloat(str[..i]); - return str[i..]; + length = i; + return TryParseFloat(str[..length], out scaler); } } else if (char.IsLetter(ch)) { // Hit a command letter; end this number. - scaler = ParseFloat(str[..i]); - return str[i..]; + length = i; + return TryParseFloat(str[..length], out scaler); } } - if (str.Length > 0) - { - scaler = ParseFloat(str); - } - - return []; + length = str.Length; + return TryParseFloat(str, out scaler); } private static bool IsSeparator(char ch) @@ -641,6 +710,6 @@ private static ReadOnlySpan TrimSeparator(ReadOnlySpan data) return data[idx..]; } - private static float ParseFloat(ReadOnlySpan str) - => float.Parse(str, provider: CultureInfo.InvariantCulture); + private static bool TryParseFloat(ReadOnlySpan str, out float value) + => float.TryParse(str, CultureInfo.InvariantCulture, out value) && float.IsFinite(value); } diff --git a/tests/ImageSharp.Drawing.Tests/Issues/Issue_385.cs b/tests/ImageSharp.Drawing.Tests/Issues/Issue_385.cs new file mode 100644 index 00000000..d820bf4f --- /dev/null +++ b/tests/ImageSharp.Drawing.Tests/Issues/Issue_385.cs @@ -0,0 +1,13 @@ +// Copyright (c) Six Labors. +// Licensed under the Six Labors Split License. + +namespace SixLabors.ImageSharp.Drawing.Tests.Issues; + +public class Issue_385 +{ + [Theory] + [InlineData("M 10 80 A 4444444444444444444444444444444444444445 45 0 04445 45 0 0 0 125 125 L 125 80 Z")] + [InlineData("M 10 80 A 45 455555555555555555555555 55")] + public void TryParseSvgPath_ReturnsFalseForMalformedArcData(string svgPath) + => Assert.False(Path.TryParseSvgPath(svgPath, out _)); +} From 3c926156be3739af333b58b166cfbc648521fb58 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Sun, 10 May 2026 23:37:52 +1000 Subject: [PATCH 2/2] Add test for malformed SVG path parsing --- src/ImageSharp.Drawing/Path.cs | 1 + .../Shapes/PathTests.cs | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/ImageSharp.Drawing/Path.cs b/src/ImageSharp.Drawing/Path.cs index 263fa060..f90a6f3a 100644 --- a/src/ImageSharp.Drawing/Path.cs +++ b/src/ImageSharp.Drawing/Path.cs @@ -575,6 +575,7 @@ private static bool TryFindFlag(ref ReadOnlySpan str, out bool value) // https://www.w3.org/TR/SVG11/paths.html#PathDataBNF // flag: "0" | "1" + // Adjacent flags are valid, so this consumes exactly one character. if (str.Length == 0 || (str[0] is not '0' and not '1')) { value = default; diff --git a/tests/ImageSharp.Drawing.Tests/Shapes/PathTests.cs b/tests/ImageSharp.Drawing.Tests/Shapes/PathTests.cs index db440217..2df21b4c 100644 --- a/tests/ImageSharp.Drawing.Tests/Shapes/PathTests.cs +++ b/tests/ImageSharp.Drawing.Tests/Shapes/PathTests.cs @@ -86,6 +86,34 @@ public void EmptyPath_ToLinearGeometry_ReturnsEmptyGeometry() Assert.False(segments.MoveNext()); } + [Theory] + [InlineData("M")] + [InlineData("M 5")] + [InlineData("V")] + [InlineData("H")] + [InlineData("C 1 2")] + [InlineData("S 6 7")] + [InlineData("Q 3 4 5")] + [InlineData("T")] + [InlineData("A 1 2 3 4 5 6")] + [InlineData("~ 7 6 5")] + public void TryParseSvgPath_ReturnsFalseForMalformedPathData(string svgPath) + => Assert.False(Path.TryParseSvgPath(svgPath, out _)); + + [Theory] + [InlineData("M 10 10 L 1e999 20")] + [InlineData("M 10 10 L NaN 20")] + [InlineData("M 10 10 L Infinity 20")] + [InlineData("M 10 10 h 1e999")] + [InlineData("M 10 10 v 1e999")] + [InlineData("M 10 10 A 25 25 0 2 0 50 50")] + [InlineData("M 10 10 A 25 25 0 0 2 50 50")] + [InlineData("M 10 10 L")] + [InlineData("M 10 10 Q 20 20")] + [InlineData("M 10 10 C 20 20 30 30")] + public void TryParseSvgPath_ReturnsFalseForInvalidPathDataBoundaries(string svgPath) + => Assert.False(Path.TryParseSvgPath(svgPath, out _)); + [Fact] public void PathCollection_EnumerableConstructor_PreservesPaths() {