From 70959f3a54e940028ce4f9b49e033e3e67db79a8 Mon Sep 17 00:00:00 2001
From: Robert Löhning <robert.loehning@qt.io>
Date: Fri, 12 Sep 2025 21:22:58 +0200
Subject: [PATCH] Don't create group nodes which will be deleted anyway

The old code first created the nodes, then checked whether their parent
element has the right type and deleted them if not. This was wasted
effort and could also lead to dangling pointers.

Instead, first check the parent's type and only create the node if that
matches.

Task-number: QTBUG-139961
Change-Id: Ifa870efbd5f336b34b81aa09b6fe79fb7fc826b9
Reviewed-by: Hatem ElKharashy <hatem.elkharashy@qt.io>
(cherry picked from commit 7e8898903265d931df0aa54b3913f2c49d4d7bf2)
Reviewed-by: Jani Heikkinen <jani.heikkinen@qt.io>
(cherry picked from commit 16a8eb2c88ec1125c897da35b524a18d619eb261)
(cherry picked from commit 6a6273126770006232e805cf1631f93d4919b788)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
---

diff --git a/src/svg/qsvghandler.cpp b/src/svg/qsvghandler.cpp
index 4553c46..0c5d74d 100644
--- a/src/svg/qsvghandler.cpp
+++ b/src/svg/qsvghandler.cpp
@@ -4733,46 +4733,46 @@
 
     if (FactoryMethod method = findGroupFactory(localName, options())) {
         //group
-        node = method(m_doc ? m_nodes.top() : 0, attributes, this);
-
-        if (node) {
-            if (!m_doc) {
+        if (!m_doc) {
+            node = method(nullptr, attributes, this);
+            if (node) {
                 Q_ASSERT(node->type() == QSvgNode::Doc);
                 m_doc = static_cast<QSvgTinyDocument*>(node);
-            } else {
-                switch (m_nodes.top()->type()) {
-                case QSvgNode::Doc:
-                case QSvgNode::Group:
-                case QSvgNode::Defs:
-                case QSvgNode::Switch:
-                case QSvgNode::Mask:
-                case QSvgNode::Symbol:
-                case QSvgNode::Marker:
-                case QSvgNode::Pattern:
-                {
+            }
+        } else {
+            switch (m_nodes.top()->type()) {
+            case QSvgNode::Doc:
+            case QSvgNode::Group:
+            case QSvgNode::Defs:
+            case QSvgNode::Switch:
+            case QSvgNode::Mask:
+            case QSvgNode::Symbol:
+            case QSvgNode::Marker:
+            case QSvgNode::Pattern:
+            {
+                node = method(m_nodes.top(), attributes, this);
+                if (node) {
                     QSvgStructureNode *group =
                         static_cast<QSvgStructureNode*>(m_nodes.top());
                     group->addChild(node, someId(attributes));
                 }
-                    break;
-                default:
-                    const QByteArray msg = QByteArrayLiteral("Could not add child element to parent element because the types are incorrect.");
-                    qCWarning(lcSvgHandler, "%s", prefixMessage(msg, xml).constData());
-                    delete node;
-                    node = 0;
-                    break;
-                }
             }
+                break;
+            default:
+                const QByteArray msg = QByteArrayLiteral("Could not add child element to parent element because the types are incorrect.");
+                qCWarning(lcSvgHandler, "%s", prefixMessage(msg, xml).constData());
+                break;
+            }
+        }
 
-            if (node) {
-                parseCoreNode(node, attributes);
+        if (node) {
+            parseCoreNode(node, attributes);
 #ifndef QT_NO_CSSPARSER
-                cssStyleLookup(node, this, m_selector);
+            cssStyleLookup(node, this, m_selector);
 #endif
-                parseStyle(node, attributes, this);
-                if (node->type() == QSvgNode::Filter)
-                    m_toBeResolved.append(node);
-            }
+            parseStyle(node, attributes, this);
+            if (node->type() == QSvgNode::Filter)
+                m_toBeResolved.append(node);
         }
     } else if (FactoryMethod method = findGraphicsFactory(localName, options())) {
         //rendering element
diff --git a/tests/auto/qsvgrenderer/tst_qsvgrenderer.cpp b/tests/auto/qsvgrenderer/tst_qsvgrenderer.cpp
index dd677ef..f32362c 100644
--- a/tests/auto/qsvgrenderer/tst_qsvgrenderer.cpp
+++ b/tests/auto/qsvgrenderer/tst_qsvgrenderer.cpp
@@ -82,6 +82,7 @@
     void testSymbol();
     void testMarker();
     void testPatternElement();
+    void testMisplacedElement();
     void testCycles();
     void testFeFlood();
     void testFeOffset();
@@ -2091,6 +2092,31 @@
     QCOMPARE(refImage, image);
 }
 
+void tst_QSvgRenderer::testMisplacedElement()
+{
+    // This input caused a QSvgPattern node to be created with a QSvgPatternStyle referencing to it.
+    // The code then detected that the <pattern> element is misplaced in the <text> element and
+    // deleted it. That left behind the QSvgPatternStyle pointing to the deleted QSvgPattern. That
+    // was reported when running the test with ASAN or UBSAN.
+    QByteArray svg(R"(<svg>
+                      <text><pattern id="ptn" width="4" height="4"/></text>
+                      <g fill="url(#ptn) "/>
+                      </svg>)");
+
+    QImage image(20, 20, QImage::Format_ARGB32_Premultiplied);
+    image.fill(Qt::green);
+    QImage refImage = image.copy();
+
+    QTest::ignoreMessage(QtWarningMsg, "<input>:2:68: Could not add child element to parent "
+                                       "element because the types are incorrect.");
+    QTest::ignoreMessage(QtWarningMsg, "<input>:4:28: Could not resolve property: #ptn");
+
+    QSvgRenderer renderer(svg);
+    QPainter painter(&image);
+    renderer.render(&painter);
+    QCOMPARE(image, refImage);
+}
+
 void tst_QSvgRenderer::testCycles()
 {
     QByteArray svgDoc("<svg viewBox=\"0 0 200 200\">"