From ee5c5744242435d10360a23f0c1798a45f38a518 Mon Sep 17 00:00:00 2001 From: Robert Löhning Date: Wed, 18 Jun 2025 16:02:32 +0200 Subject: [PATCH] Replace check for endless recursion when loading The old check parsed the tree of SvgNodes again and again which lead to quadratic complexity. Instead, set and check a bool where the recursion may actually happen which is faster and only has linear complexity. Partially reverts 0332df304f013ded362537c1f61556098b875352 I chose to have the check in QSvgPattern::renderPattern() because: - It not only appears in the recursive backtrace of the stack-overflow which was fixed using the qudratic check, but also in the backtrace of another, still unfixed stack overflow. That way, both can be fixed by the same patch. Credit to OSS-Fuzz for finding them. - The function already had some error checking and returns a default value when it cannot render the content. In the same way, I can return a QImage of the right size but without any content when the endless recursion is about to happen. [ChangeLog] Speed up loading by replacing check for cyclic elements [ChangeLog] Fix stack overflow when an element references its child element using url() Fixes: QTBUG-137553 Change-Id: If011c15fde50dcefeb653d1d5995ff1347e7b5ac Reviewed-by: Hatem ElKharashy (cherry picked from commit 9e5bed9584ab65d56cd5fbac0471e06e37a54412) Reviewed-by: Qt Cherry-pick Bot (cherry picked from commit 90a5331640bb760b0114a7ea4e08b9e42b03e082) (cherry picked from commit ea44b50c6e61104cadd6b7c8ede92a4108634232) --- diff --git a/src/svg/qsvghandler.cpp b/src/svg/qsvghandler.cpp index ad6c0ec..4553c46 100644 --- a/src/svg/qsvghandler.cpp +++ b/src/svg/qsvghandler.cpp @@ -4663,8 +4663,7 @@ // this point is to do what everyone else seems to do and // ignore the reported namespaceUri completely. if (remainingUnfinishedElements - && startElement(xml->name().toString(), xml->attributes()) - && !detectCyclesAndWarn(m_doc)) { + && startElement(xml->name().toString(), xml->attributes())) { --remainingUnfinishedElements; } else { delete m_doc; diff --git a/src/svg/qsvgstructure.cpp b/src/svg/qsvgstructure.cpp index 6533684..6b474b4 100644 --- a/src/svg/qsvgstructure.cpp +++ b/src/svg/qsvgstructure.cpp @@ -802,6 +802,7 @@ m_rect(bounds), m_viewBox(viewBox), m_contentUnits(contentUnits), + m_isRendering(false), m_transform(transform) { @@ -887,6 +888,13 @@ } pattern.fill(Qt::transparent); + if (m_isRendering) { + qCWarning(lcSvgDraw) << "The pattern is trying to render itself recursively. " + "Returning a transparent QImage of the right size."; + return pattern; + } + QScopedValueRollback guard(m_isRendering, true); + // Draw the pattern using our QPainter. QPainter patternPainter(&pattern); QSvgExtraStates patternStates; diff --git a/src/svg/qsvgstructure_p.h b/src/svg/qsvgstructure_p.h index 285244a..d36ca9e 100644 --- a/src/svg/qsvgstructure_p.h +++ b/src/svg/qsvgstructure_p.h @@ -241,6 +241,7 @@ QSvgRectF m_rect; QRectF m_viewBox; QtSvg::UnitTypes m_contentUnits; + mutable bool m_isRendering; QTransform m_transform; }; diff --git a/tests/auto/qsvgrenderer/tst_qsvgrenderer.cpp b/tests/auto/qsvgrenderer/tst_qsvgrenderer.cpp index 64bdcb9..905d18f 100644 --- a/tests/auto/qsvgrenderer/tst_qsvgrenderer.cpp +++ b/tests/auto/qsvgrenderer/tst_qsvgrenderer.cpp @@ -69,6 +69,7 @@ void oss_fuzz_24738(); void oss_fuzz_61586(); void oss_fuzz_42532991(); + void oss_fuzz_390467765(); void oss_fuzz_399769595(); void imageRendering(); void imageMalformedDataUrl(); @@ -1769,6 +1770,12 @@ QSvgRenderer().load(QByteArray("")); } +void tst_QSvgRenderer::oss_fuzz_390467765() +{ + // resulted in stack overflow + QSvgRenderer().load(QByteArray("")); +} + void tst_QSvgRenderer::oss_fuzz_399769595() { // resulted in null pointer deref