Bug 51698 – SVG pathsegs not sanitized before being passed through to platform Path

• Component: SVG
• Status: RESOLVED INVALID

[Stefan Ring]

Created attachment 77606 [details]
Test case

I'm not sure about the version number, as they mean nothing to me. I'm talking about the version included with Qt 4.7.

I have some JavaScript code that creates a curved path element where some coordinates are NaNs (due to a bug in the rest of my code). Apparently, QPainterPath is used to paint SVG paths, and this is where the problems occur. What happens is this:

QPainterPath::cubicTo: Adding point where x or y is NaN, results are undefined
QPainterPath::lineTo: Adding point where x or y is NaN, results are undefined
QPainterPath::cubicTo: Adding point where x or y is NaN, results are undefined
QPainterPath::lineTo: Adding point where x or y is NaN, results are undefined
QPainterPath::cubicTo: Adding point where x or y is NaN, results are undefined
QPainterPath::cubicTo: Adding point where x or y is NaN, results are undefined
QPainterPath::lineTo: Adding point where x or y is NaN, results are undefined
QPainterPath::cubicTo: Adding point where x or y is NaN, results are undefined
QPainterPath::lineTo: Adding point where x or y is NaN, results are undefined
QPainterPath::cubicTo: Adding point where x or y is NaN, results are undefined
QPainterPath::lineTo: Adding point where x or y is NaN, results are undefined
QPainterPath::cubicTo: Adding point where x or y is NaN, results are undefined
QPainterPath::lineTo: Adding point where x or y is NaN, results are undefined
QPainterPath::cubicTo: Adding point where x or y is NaN, results are undefined

and after a few seconds:

Catchpoint 1 (exception thrown), 0x0000003cb5cbcd00 in __cxa_throw () from /usr/lib64/libstdc++.so.6
(gdb) bt
#0 0x0000003cb5cbcd00 in __cxa_throw () from /usr/lib64/libstdc++.so.6
#1 0x00007ffff7b1eb25 in qBadAlloc () at global/qglobal.cpp:1996
#2 0x00007ffff6e317a7 in QVector<QPointF>::realloc (this=0x7fffffff8cb0, asize=67108863, aalloc=134217727) at ../../include/QtCore/../../src/corelib/tools/qvector.h:481
#3 0x00007ffff6e3deee in QVector<QPointF>::append (this=0x7fffffff8cb0, t=...) at ../../include/QtCore/../../src/corelib/tools/qvector.h:548
#4 0x00007ffff6e77514 in QBezier::addToPolygon (this=0x7fffffff8c70, polygon=0x7fffffff8cb0, bezier_flattening_threshold=0.5) at painting/qbezier.cpp:220
#5 0x00007ffff6ee6848 in QPainterPath::toSubpathPolygons (this=0x7fffffff9fe0, matrix=...) at painting/qpainterpath.cpp:1509
#6 0x00007ffff6ee69b4 in QPainterPath::toFillPolygons (this=0x7fffffff9fe0, matrix=...) at painting/qpainterpath.cpp:1560
#7 0x00007ffff6ee6f65 in QPainterPath::toFillPolygons (this=0x7fffffff9fe0, matrix=...) at painting/qpainterpath.cpp:1655
#8 0x00007ffff6fb8347 in QX11PaintEnginePrivate::fillPath (this=0xc60db0, path=..., gc_mode=QX11PaintEnginePrivate::PenGC, transform=true) at painting/qpaintengine_x11.cpp:1752
#9 0x00007ffff6fb8c23 in QX11PaintEngine::drawPath (this=0xc8ff60, path=...) at painting/qpaintengine_x11.cpp:1815
#10 0x00007ffff6ecee45 in QPainter::drawPath (this=0x7fffffffb6b0, path=...) at painting/qpainter.cpp:3381
#11 0x00007ffff6eceac5 in QPainter::strokePath (this=0x7fffffffb6b0, path=..., pen=...) at painting/qpainter.cpp:3293
#12 0x00007ffff578facc in WebCore::GraphicsContext::strokePath() () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#13 0x00007ffff59e912d in WebCore::SVGPaintServer::renderPath(WebCore::GraphicsContext*&, WebCore::RenderObject const*, WebCore::SVGPaintTargetType) const () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#14 0x00007ffff59e9039 in WebCore::SVGPaintServer::draw(WebCore::GraphicsContext*&, WebCore::RenderObject const*, WebCore::SVGPaintTargetType) const () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#15 0x00007ffff59f22f1 in WebCore::fillAndStrokePath(WebCore::Path const&, WebCore::GraphicsContext*, WebCore::RenderStyle*, WebCore::RenderPath*) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#16 0x00007ffff59f2603 in WebCore::RenderPath::paint(WebCore::RenderObject::PaintInfo&, int, int) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#17 0x00007ffff59f3850 in WebCore::RenderSVGContainer::paint(WebCore::RenderObject::PaintInfo&, int, int) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#18 0x00007ffff59f3850 in WebCore::RenderSVGContainer::paint(WebCore::RenderObject::PaintInfo&, int, int) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#19 0x00007ffff569d1aa in WebCore::RenderBox::paint(WebCore::RenderObject::PaintInfo&, int, int) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#20 0x00007ffff59fd101 in WebCore::RenderSVGRoot::paint(WebCore::RenderObject::PaintInfo&, int, int) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#21 0x00007ffff56df20c in WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#22 0x00007ffff56df478 in WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#23 0x00007ffff56df478 in WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#24 0x00007ffff56de34d in WebCore::RenderLayer::paint(WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#25 0x00007ffff5592f26 in WebCore::FrameView::paintContents(WebCore::GraphicsContext*, WebCore::IntRect const&) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#26 0x00007ffff57d296e in QWebFramePrivate::renderRelativeCoords(WebCore::GraphicsContext*, QWebFrame::RenderLayer, QRegion const&) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#27 0x00007ffff57d56ec in QWebFrame::render(QPainter*, QRegion const&) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4
#28 0x00007ffff57f070c in QWebView::paintEvent(QPaintEvent*) () from /home/sr/build/qt47_debug/lib/libQtWebKit.so.4

[Ademar Reis]

Reproduced here. I'll investigate the cause.

[Ademar Reis]

I submited a patch to Qt last week fixing this problem on their side: http://qt.gitorious.org/qt/qt/merge_requests/1026 (merged already).

Although this was fixed in Qt, I believe it should be fixed in WebKit as well. I had to focus on other higher priority tasks for the moment and what I have here is not ready for submission.

The SVG Micro DOM spec (http://www.w3.org/TR/SVGTiny12/svgudom.html) clearly says that a DOMException with error code NOT_SUPPORTED_ERR must be thrown when a NaN/Inf is used, but I couldn't find any refrences to NaN/Inf in the "standard" SVG spec. I had the impression that NaN/Inf values are ignored by WebKit in other places, so maybe a patch just checking for it (similar to my patch in Qt) is enough.

I plan to get back to it, but if someone wants to work on it right now, feel free to do so. :-)

[Ademar Reis]

Created attachment 89597 [details]
patch

This is a patch that ignores calls with NaNs and a simple test for the crash (based on the test-cased provided in this bug).

Maybe the testcase can be reduced, but I lack the javascript/svg knowledge to prepare something better... Hope it's good enough :)

[Andreas Kling]

Comment on attachment 89597 [details]
patch

Change itself is fine, but the test only covers one specific configuration of the curveToCubic() code path. All three functions should be covered, and we should verify that any of the coordinate arguments can be non-finite without causing a crash.

[Ademar Reis]

Created attachment 89604 [details]
patch with improved test

[Ademar Reis]

blocking 2.2, as it's a crash and the Qt fix is not included in Qt-4.7 (only qt-4.8)

[Dirk Schulze]

Are infinite values allowed for SVGPathSegs at all? We may should take care on it in general, not only for creating the platform path. Also, with your patch we would just ignore the certain segment and continue with the next segment. The Spec want us to stop drawing a path on the first invalid segment and drop all following segments. It's just the question if a segment with infinite values is invalid and how other browsers handle infinite values for segments.

[Ademar Reis]

(In reply to comment #7)
> Are infinite values allowed for SVGPathSegs at all? We may should take care on it in general, not only for creating the platform path. Also, with your patch we would just ignore the certain segment and continue with the next segment. The Spec want us to stop drawing a path on the first invalid segment and drop all following segments. It's just the question if a segment with infinite values is invalid and how other browsers handle infinite values for segments.

I couldn't find any mention of non-finite floats nor exceptions to be raised in such case... I also couln't find a clear statement that says we have to stop drawing on the first invalid segment... But again, I'm no SVG expert.

[Dirk Schulze]

(In reply to comment #8)
> (In reply to comment #7)
> > Are infinite values allowed for SVGPathSegs at all? We may should take care on it in general, not only for creating the platform path. Also, with your patch we would just ignore the certain segment and continue with the next segment. The Spec want us to stop drawing a path on the first invalid segment and drop all following segments. It's just the question if a segment with infinite values is invalid and how other browsers handle infinite values for segments.
> 
> I couldn't find any mention of non-finite floats nor exceptions to be raised in such case... I also couln't find a clear statement that says we have to stop drawing on the first invalid segment... But again, I'm no SVG expert.

How do other browsers react? Can you check the displayed path and the SVGPathSegList? Are following valid Segments in the seglist and are they displayed?

[Ademar Reis]

(In reply to comment #9)
> (In reply to comment #8)
> > I couldn't find any mention of non-finite floats nor exceptions to be raised in such case... I also couln't find a clear statement that says we have to stop drawing on the first invalid segment... But again, I'm no SVG expert.
> 
> How do other browsers react? Can you check the displayed path and the SVGPathSegList? Are following valid Segments in the seglist and are they displayed?

Sure. I used the path-nans-crash.html file from my patch but only with the invalid calls:

Safari 5.0.3: nothing is displayed
Firefox 3.6.16: nothing is displayed
Opera 11.10 (Linux): nothing
IE8: nothing
QtTestBrowser: with latest Qt (trunk), nothing, with stable Qt (4.7), it crashes
Google-chrome 10.0.648.204 (Linux): the polygon is drawn (not sure how it interprets the NaNs)

So the only browser I could find that does drawn a polygon when the pathseg calls include NaNs is google-chrome.

I'll investigate a bit more, as my tests were blind. I'll try to make sense of the numbers passed to the pathseg calls.

[Ademar Reis]

> So the only browser I could find that does drawn a polygon when the pathseg calls include NaNs is google-chrome.
> 
> I'll investigate a bit more, as my tests were blind. I'll try to make sense of the numbers passed to the pathseg calls.

Simple enough: google-chrome converts NaNs to zeros, all the other browsers I could test just don't display anything.

[Dirk Schulze]

(In reply to comment #11)
> > So the only browser I could find that does drawn a polygon when the pathseg calls include NaNs is google-chrome.
> > 
> > I'll investigate a bit more, as my tests were blind. I'll try to make sense of the numbers passed to the pathseg calls.
> 
> Simple enough: google-chrome converts NaNs to zeros, all the other browsers I could test just don't display anything.

You have not added two or more valid segments without infinite values. :-)

[Ademar Reis]

(In reply to comment #12)
> > Simple enough: google-chrome converts NaNs to zeros, all the other browsers I could test just don't display anything.
> 
> You have not added two or more valid segments without infinite values. :-)

You're right. In this case, safari shows something, while the other browsers ignore everything.

Google-chrome: converts NaNs to zero and draws everything;
Safari: ignores the NaNs calls and draws something;
Firefox an Opera: don't display anything;
QtTestBrowser: ignores the NaN calls and draws the rest;

In the end we have four different behaviors, since the images drawn by chrome, safari and QtTestBrowser are all different (I'll attach an screenshot).

With the patch we'll at least have some consistency, as the calls with NaNs will be ignored.

[Ademar Reis]

Created attachment 89626 [details]
screenshot of the different results on chrome, safari and qttestbrowser

[Ademar Reis]

Created attachment 89628 [details]
html file used for the rendering tests

[Dirk Schulze]

(In reply to comment #13)
> (In reply to comment #12)
> > > Simple enough: google-chrome converts NaNs to zeros, all the other browsers I could test just don't display anything.
> > 
> > You have not added two or more valid segments without infinite values. :-)
> 
> You're right. In this case, safari shows something, while the other browsers ignore everything.
> 
> Google-chrome: converts NaNs to zero and draws everything;
> Safari: ignores the NaNs calls and draws something;
> Firefox an Opera: don't display anything;
> QtTestBrowser: ignores the NaN calls and draws the rest;
> 
> In the end we have four different behaviors, since the images drawn by chrome, safari and QtTestBrowser are all different (I'll attach an screenshot).
> 
> With the patch we'll at least have some consistency, as the calls with NaNs will be ignored.

To have a consistant behavior across webkit implementations is one point, but we should also take care to be consistent with other browsers, namely: IE9, Firefox and Opera. And it seems all browsers raise a DOMException for NaN values.

I greped in the history of our IDL files and it looks like we had the following lines commented out: setter raises(DOMException)
Event this line is gone away today. We should add it for every value but I guess we would need more changes.

Niko any idea? Is there a reason why we did not activate this?

[Ademar Reis]

(In reply to comment #16)
> (In reply to comment #13)
> To have a consistant behavior across webkit implementations is one point, but we should also take care to be consistent with other browsers, namely: IE9, Firefox and Opera. And it seems all browsers raise a DOMException for NaN values.
> 
> I greped in the history of our IDL files and it looks like we had the following lines commented out: setter raises(DOMException)
> Event this line is gone away today. We should add it for every value but I guess we would need more changes.
> 
> Niko any idea? Is there a reason why we did not activate this?

All right. Makes sense.

Looking at the code history (thanks for pointing this out), looks like the exception was never implemented (it was introduced as a comment).

I'll prepare a new patch for review: this time introducing the exception. Please let me know if there's any guideline I should follwo regarding this.

[Ademar Reis]

A fix for the crash on QtWebKit has been merged into Qt-4.7 (next minor update will include it) as well as Qt-4.8 (to be released).

I'm afraid I won't be able to work on this bug in the short term, so I'm marking it as unassigned for now.

[Dirk Schulze]

Since this seems to be a Qt only problem and QtWebKit doesn't exist anymore: closing.