Skip to content

Commit de6e9c8

Browse files
JimBobSquarePantsJimBobSquarePants
committed
Harden SVG path parser and add tests. Fix #385 
1 parent ebf73b1 commit de6e9c8

2 files changed

Lines changed: 162 additions & 80 deletions

File tree

‎src/ImageSharp.Drawing/Path.cs‎

Lines changed: 149 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -370,8 +370,9 @@ public static bool TryParseSvgPath(ReadOnlySpan<char> svgPath, [NotNullWhen(true
370370
char ch = svgPath[0];
371371
if (char.IsDigit(ch) || ch == '-' || ch == '+' || ch == '.')
372372
{
373-
// Are we are the end of the string or we are at the end of the path?
374-
if (svgPath.Length == 0 || op == 'Z')
373+
// SVG allows repeated operand groups to reuse the previous command.
374+
// A leading number is only valid once a drawable command is active.
375+
if (op is '\0' or 'Z')
375376
{
376377
return false;
377378
}
@@ -393,107 +394,164 @@ public static bool TryParseSvgPath(ReadOnlySpan<char> svgPath, [NotNullWhen(true
393394
svgPath = TrimSeparator(svgPath[1..]);
394395
}
395396

397+
// Read every operand for the command before appending geometry. That keeps
398+
// malformed or truncated data from leaking a partially parsed segment into the path.
396399
switch (op)
397400
{
398401
case 'M':
399-
svgPath = FindPoint(svgPath, relative, c, out point1);
400-
builder.MoveTo(point1);
402+
if (!TryFindPoint(ref svgPath, relative, c, out point1))
403+
{
404+
return false;
405+
}
406+
407+
_ = builder.MoveTo(point1);
401408
previousOp = '\0';
409+
410+
// Extra coordinate pairs after a move command are implicit line commands.
402411
op = 'L';
403412
c = point1;
404413
break;
405414
case 'L':
406-
svgPath = FindPoint(svgPath, relative, c, out point1);
407-
builder.LineTo(point1);
415+
if (!TryFindPoint(ref svgPath, relative, c, out point1))
416+
{
417+
return false;
418+
}
419+
420+
_ = builder.LineTo(point1);
408421
c = point1;
409422
break;
410423
case 'H':
411-
svgPath = FindScaler(svgPath, out float x);
424+
if (!TryFindScaler(ref svgPath, out float x))
425+
{
426+
return false;
427+
}
428+
412429
if (relative)
413430
{
414431
x += c.X;
415432
}
416433

417-
builder.LineTo(x, c.Y);
434+
if (!float.IsFinite(x))
435+
{
436+
return false;
437+
}
438+
439+
_ = builder.LineTo(x, c.Y);
418440
c.X = x;
419441
break;
420442
case 'V':
421-
svgPath = FindScaler(svgPath, out float y);
443+
if (!TryFindScaler(ref svgPath, out float y))
444+
{
445+
return false;
446+
}
447+
422448
if (relative)
423449
{
424450
y += c.Y;
425451
}
426452

427-
builder.LineTo(c.X, y);
453+
if (!float.IsFinite(y))
454+
{
455+
return false;
456+
}
457+
458+
_ = builder.LineTo(c.X, y);
428459
c.Y = y;
429460
break;
430461
case 'C':
431-
svgPath = FindPoint(svgPath, relative, c, out point1);
432-
svgPath = FindPoint(svgPath, relative, c, out point2);
433-
svgPath = FindPoint(svgPath, relative, c, out point3);
434-
builder.CubicBezierTo(point1, point2, point3);
462+
if (!TryFindPoint(ref svgPath, relative, c, out point1)
463+
|| !TryFindPoint(ref svgPath, relative, c, out point2)
464+
|| !TryFindPoint(ref svgPath, relative, c, out point3))
465+
{
466+
return false;
467+
}
468+
469+
_ = builder.CubicBezierTo(point1, point2, point3);
435470
lastc = point2;
436471
c = point3;
437472
break;
438473
case 'S':
439-
svgPath = FindPoint(svgPath, relative, c, out point2);
440-
svgPath = FindPoint(svgPath, relative, c, out point3);
474+
if (!TryFindPoint(ref svgPath, relative, c, out point2)
475+
|| !TryFindPoint(ref svgPath, relative, c, out point3))
476+
{
477+
return false;
478+
}
479+
441480
point1 = c;
442481
if (previousOp is 'C' or 'S')
443482
{
483+
// Smooth cubic curves mirror the previous cubic control point.
484+
// Without a preceding cubic command, the current point is the control point.
444485
point1.X -= lastc.X - c.X;
445486
point1.Y -= lastc.Y - c.Y;
446487
}
447488

448-
builder.CubicBezierTo(point1, point2, point3);
489+
_ = builder.CubicBezierTo(point1, point2, point3);
449490
lastc = point2;
450491
c = point3;
451492
break;
452493
case 'Q': // Quadratic Bezier Curve
453-
svgPath = FindPoint(svgPath, relative, c, out point1);
454-
svgPath = FindPoint(svgPath, relative, c, out point2);
455-
builder.QuadraticBezierTo(point1, point2);
494+
if (!TryFindPoint(ref svgPath, relative, c, out point1)
495+
|| !TryFindPoint(ref svgPath, relative, c, out point2))
496+
{
497+
return false;
498+
}
499+
500+
_ = builder.QuadraticBezierTo(point1, point2);
456501
lastc = point1;
457502
c = point2;
458503
break;
459504
case 'T':
460-
svgPath = FindPoint(svgPath, relative, c, out point2);
505+
if (!TryFindPoint(ref svgPath, relative, c, out point2))
506+
{
507+
return false;
508+
}
509+
461510
point1 = c;
462511
if (previousOp is 'Q' or 'T')
463512
{
513+
// Smooth quadratic curves mirror the previous quadratic control point.
514+
// Without a preceding quadratic command, the current point is the control point.
464515
point1.X -= lastc.X - c.X;
465516
point1.Y -= lastc.Y - c.Y;
466517
}
467518

468-
builder.QuadraticBezierTo(point1, point2);
519+
_ = builder.QuadraticBezierTo(point1, point2);
469520
lastc = point1;
470521
c = point2;
471522
break;
472523
case 'A':
473-
if (TryFindScaler(ref svgPath, out float radiiX)
474-
&& TryTrimSeparator(ref svgPath)
475-
&& TryFindScaler(ref svgPath, out float radiiY)
476-
&& TryTrimSeparator(ref svgPath)
477-
&& TryFindScaler(ref svgPath, out float angle)
478-
&& TryTrimSeparator(ref svgPath)
479-
&& TryFindScaler(ref svgPath, out float largeArc)
480-
&& TryTrimSeparator(ref svgPath)
481-
&& TryFindScaler(ref svgPath, out float sweep)
482-
&& TryFindPoint(ref svgPath, relative, c, out PointF point))
524+
// Arc flags are single SVG grammar tokens, not numbers. Reading them as
525+
// scalars would accept malformed flag/end-point boundaries such as "04445".
526+
if (!TryFindScaler(ref svgPath, out float radiiX)
527+
|| !TryTrimSeparator(ref svgPath)
528+
|| !TryFindScaler(ref svgPath, out float radiiY)
529+
|| !TryTrimSeparator(ref svgPath)
530+
|| !TryFindScaler(ref svgPath, out float angle)
531+
|| !TryTrimSeparator(ref svgPath)
532+
|| !TryFindFlag(ref svgPath, out bool largeArc)
533+
|| !TryTrimSeparator(ref svgPath)
534+
|| !TryFindFlag(ref svgPath, out bool sweep)
535+
|| !TryFindPoint(ref svgPath, relative, c, out PointF point))
483536
{
484-
builder.ArcTo(radiiX, radiiY, angle, largeArc == 1, sweep == 1, point);
485-
c = point;
537+
return false;
486538
}
487539

540+
_ = builder.ArcTo(radiiX, radiiY, angle, largeArc, sweep, point);
541+
c = point;
488542
break;
489543
case 'Z':
490-
builder.CloseFigure();
544+
_ = builder.CloseFigure();
491545
c = first;
492546
break;
493547
case '~':
494-
svgPath = FindPoint(svgPath, relative, c, out point1);
495-
svgPath = FindPoint(svgPath, relative, c, out point2);
496-
builder.MoveTo(point1).LineTo(point2);
548+
if (!TryFindPoint(ref svgPath, relative, c, out point1)
549+
|| !TryFindPoint(ref svgPath, relative, c, out point2))
550+
{
551+
return false;
552+
}
553+
554+
_ = builder.MoveTo(point1).LineTo(point2);
497555
break;
498556
default:
499557
return false;
@@ -511,8 +569,27 @@ public static bool TryParseSvgPath(ReadOnlySpan<char> svgPath, [NotNullWhen(true
511569
return true;
512570
}
513571

572+
private static bool TryFindFlag(ref ReadOnlySpan<char> str, out bool value)
573+
{
574+
str = TrimSeparator(str);
575+
576+
// https://www.w3.org/TR/SVG11/paths.html#PathDataBNF
577+
// flag: "0" | "1"
578+
if (str.Length == 0 || (str[0] is not '0' and not '1'))
579+
{
580+
value = default;
581+
return false;
582+
}
583+
584+
value = str[0] == '1';
585+
str = str[1..];
586+
return true;
587+
}
588+
514589
private static bool TryTrimSeparator(ref ReadOnlySpan<char> str)
515590
{
591+
// SVG separators are optional in places where the next token can be
592+
// recognized unambiguously. Keep this chainable with the operand readers.
516593
ReadOnlySpan<char> result = TrimSeparator(str);
517594
if (str[^result.Length..].StartsWith(result))
518595
{
@@ -525,11 +602,10 @@ private static bool TryTrimSeparator(ref ReadOnlySpan<char> str)
525602

526603
private static bool TryFindScaler(ref ReadOnlySpan<char> str, out float value)
527604
{
528-
ReadOnlySpan<char> result = FindScaler(str, out float valueInner);
529-
if (str[^result.Length..].StartsWith(result))
605+
ReadOnlySpan<char> source = TrimSeparator(str);
606+
if (TryReadScalar(source, out value, out int length))
530607
{
531-
value = valueInner;
532-
str = result;
608+
str = source[length..];
533609
return true;
534610
}
535611

@@ -539,55 +615,52 @@ private static bool TryFindScaler(ref ReadOnlySpan<char> str, out float value)
539615

540616
private static bool TryFindPoint(ref ReadOnlySpan<char> str, bool relative, PointF current, out PointF value)
541617
{
542-
ReadOnlySpan<char> result = FindPoint(str, relative, current, out PointF valueInner);
543-
if (str[^result.Length..].StartsWith(result))
618+
if (TryFindScaler(ref str, out float x) && TryFindScaler(ref str, out float y))
544619
{
545-
value = valueInner;
546-
str = result;
620+
// Relative operands can overflow after adding the current point even when
621+
// each parsed scalar is finite, so validate the absolute result as well.
622+
if (relative)
623+
{
624+
x += current.X;
625+
y += current.Y;
626+
}
627+
628+
if (!float.IsFinite(x) || !float.IsFinite(y))
629+
{
630+
value = default;
631+
return false;
632+
}
633+
634+
value = new PointF(x, y);
547635
return true;
548636
}
549637

550638
value = default;
551639
return false;
552640
}
553641

554-
private static ReadOnlySpan<char> FindPoint(ReadOnlySpan<char> str, bool isRelative, PointF relative, out PointF value)
555-
{
556-
str = FindScaler(str, out float x);
557-
str = FindScaler(str, out float y);
558-
if (isRelative)
559-
{
560-
x += relative.X;
561-
y += relative.Y;
562-
}
563-
564-
value = new PointF(x, y);
565-
return str;
566-
}
567-
568-
private static ReadOnlySpan<char> FindScaler(ReadOnlySpan<char> str, out float scaler)
642+
private static bool TryReadScalar(ReadOnlySpan<char> str, out float scaler, out int length)
569643
{
570-
str = TrimSeparator(str);
571-
scaler = 0;
572-
644+
// SVG path numbers can be tightly packed: "10-20" is two numbers, as is
645+
// "0.5.6". Stop at the first character that belongs to the next token.
573646
bool hasDot = false;
574647
for (int i = 0; i < str.Length; i++)
575648
{
576649
char ch = str[i];
577650

578651
if (IsSeparator(ch))
579652
{
580-
scaler = ParseFloat(str[..i]);
581-
return str[i..];
653+
length = i;
654+
return TryParseFloat(str[..length], out scaler);
582655
}
583656

584657
if (ch == '.')
585658
{
586659
if (hasDot)
587660
{
588661
// Second decimal point starts a new number.
589-
scaler = ParseFloat(str[..i]);
590-
return str[i..];
662+
length = i;
663+
return TryParseFloat(str[..length], out scaler);
591664
}
592665

593666
hasDot = true;
@@ -599,24 +672,20 @@ private static ReadOnlySpan<char> FindScaler(ReadOnlySpan<char> str, out float s
599672
char prev = str[i - 1];
600673
if (prev is not 'e' and not 'E')
601674
{
602-
scaler = ParseFloat(str[..i]);
603-
return str[i..];
675+
length = i;
676+
return TryParseFloat(str[..length], out scaler);
604677
}
605678
}
606679
else if (char.IsLetter(ch))
607680
{
608681
// Hit a command letter; end this number.
609-
scaler = ParseFloat(str[..i]);
610-
return str[i..];
682+
length = i;
683+
return TryParseFloat(str[..length], out scaler);
611684
}
612685
}
613686

614-
if (str.Length > 0)
615-
{
616-
scaler = ParseFloat(str);
617-
}
618-
619-
return [];
687+
length = str.Length;
688+
return TryParseFloat(str, out scaler);
620689
}
621690

622691
private static bool IsSeparator(char ch)
@@ -641,6 +710,6 @@ private static ReadOnlySpan<char> TrimSeparator(ReadOnlySpan<char> data)
641710
return data[idx..];
642711
}
643712

644-
private static float ParseFloat(ReadOnlySpan<char> str)
645-
=> float.Parse(str, provider: CultureInfo.InvariantCulture);
713+
private static bool TryParseFloat(ReadOnlySpan<char> str, out float value)
714+
=> float.TryParse(str, CultureInfo.InvariantCulture, out value) && float.IsFinite(value);
646715
}

‎tests/ImageSharp.Drawing.Tests/Issues/Issue_385.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// Copyright (c) Six Labors.
2+
// Licensed under the Six Labors Split License.
3+
4+
namespace SixLabors.ImageSharp.Drawing.Tests.Issues;
5+
6+
public class Issue_385
7+
{
8+
[Theory]
9+
[InlineData("M 10 80 A 4444444444444444444444444444444444444445 45 0 04445 45 0 0 0 125 125 L 125 80 Z")]
10+
[InlineData("M 10 80 A 45 455555555555555555555555 55")]
11+
public void TryParseSvgPath_ReturnsFalseForMalformedArcData(string svgPath)
12+
=> Assert.False(Path.TryParseSvgPath(svgPath, out _));
13+
}

0 commit comments

Comments
 (0)