qdoc: Fix heap-use-after-free and memory leak issues

Some of the created nodes appear multiple times in QDoc's node tree.
This caused issues with address sanitizer during deletion of the
tree: Nodes were checked for their parent() node, and the parent
node might have been deleted already.

Implement a cleanup function that removes all children that do not
report *this* node as their parent from the list of children -
after this, the tree can be safely deleted by destroying the root
node.

Fix memory leak issues; a couple of potential leaks in
ClangCodeParser caused by not freeing resources in all cases,
and DocBookGenerator leaking a QFile instance per each generated
file.

Fixes: QTBUG-97627
Change-Id: If279b55ee24dc1b7291951ef11b7a26276df167c
Reviewed-by: Luca Di Sera <luca.disera@qt.io>
Reviewed-by: Paul Wicking <paul.wicking@qt.io>
(cherry picked from commit 02057fc029e3d2cc1808fe712fca84ccfc074f99)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

7 files changed, 37 insertions, 57 deletions

diff --git a/src/qdoc/aggregate.cpp b/src/qdoc/aggregate.cpp
index 64ac0223e..45746b195 100644
--- a/src/qdoc/aggregate.cpp
+++ b/src/qdoc/aggregate.cpp
@@ -50,37 +50,36 @@ QT_BEGIN_NAMESPACE
  */
 
 /*!
-  The destructor calls delete for each child of this Aggregate
-  that has this Aggregate as its parent. A child node that has
-  some other Aggregate as its parent is deleted by that
-  Aggregate's destructor. This is a fail-safe test.
-
-  The destructor no longer deletes the collection of children
-  by calling qDeleteAll() because the child list can contain
-  pointers to children that have some other Aggregate as their
-  parent. This is because of how the \e{\\relates} command is
-  processed. An Aggregate can have a pointer to, for example,
-  a FunctionNode in its child list, but that FunctionNode has
-  a differen Aggregate as its parent because a \e{\\relates}
-  command was used to relate it to that parent. In that case,
-  the other Aggregate's destructor must delete that node.
-
-  \note This function is the \b only place where delete is
-  called to delete any subclass of Node.
-
-  \note This strategy depends on the node tree being destroyed
-  by calling delete on the root node of the tree. This happens
-  in the destructor of class Tree.
+  Recursively set all non-related members in the list of children to
+  \nullptr, after which each aggregate can safely delete all children
+  in their list. Aggregate's destructor calls this only on the root
+  namespace node.
+ */
+void Aggregate::dropNonRelatedMembers()
+{
+    for (auto &child : m_children) {
+        if (!child)
+            continue;
+        if (child->parent() != this)
+            child = nullptr;
+        else if (child->isAggregate())
+            static_cast<Aggregate*>(child)->dropNonRelatedMembers();
+    }
+}
+
+/*!
+  Destroys this Aggregate; deletes each child.
  */
 Aggregate::~Aggregate()
 {
+    // If this is the root, clear non-related children first
+    if (isNamespace() && name().isEmpty())
+        dropNonRelatedMembers();
+
     m_enumChildren.clear();
     m_nonfunctionMap.clear();
     m_functionMap.clear();
-    for (const Node *child : m_children) {
-        if ((child != nullptr) && (child->parent() == this))
-            delete child;
-    }
+    qDeleteAll(m_children.begin(), m_children.end());
     m_children.clear();
 }
 
diff --git a/src/qdoc/aggregate.h b/src/qdoc/aggregate.h
index d50250f41..5e7480d9a 100644
--- a/src/qdoc/aggregate.h
+++ b/src/qdoc/aggregate.h
@@ -98,6 +98,7 @@ private:
     void addFunction(FunctionNode *fn);
     void adoptFunction(FunctionNode *fn, Aggregate *firstParent);
     static bool isSameSignature(const FunctionNode *f1, const FunctionNode *f2);
+    void dropNonRelatedMembers();
 
 protected:
     NodeList m_children {};
diff --git a/src/qdoc/clangcodeparser.cpp b/src/qdoc/clangcodeparser.cpp
index dd6aca556..a7aaf4069 100644
--- a/src/qdoc/clangcodeparser.cpp
+++ b/src/qdoc/clangcodeparser.cpp
@@ -1386,11 +1386,11 @@ void ClangCodeParser::buildPCH()
                     visitor.visitChildren(cur);
                     qCDebug(lcQdoc) << "PCH built and visited for" << moduleHeader();
                 }
-                clang_disposeTranslationUnit(tu);
             } else {
                 m_pchFileDir->remove();
                 qCCritical(lcQdoc) << "Could not create PCH file for " << moduleHeader();
             }
+            clang_disposeTranslationUnit(tu);
             m_args.pop_back(); // remove the "-xc++";
         }
     }
@@ -1464,6 +1464,7 @@ void ClangCodeParser::parseSourceFile(const Location & /*location*/, const QStri
 
     if (err || !tu) {
         qWarning() << "(qdoc) Could not parse source file" << filePath << " error code:" << err;
+        clang_disposeTranslationUnit(tu);
         clang_disposeIndex(index_);
         return;
     }
@@ -1709,6 +1710,7 @@ void ClangCodeParser::printDiagnostics(const CXTranslationUnit &translationUnit)
         auto formattedDiagnostic = clang_formatDiagnostic(diagnostic, displayOptions);
         qCDebug(lcQdocClang) << clang_getCString(formattedDiagnostic);
         clang_disposeString(formattedDiagnostic);
+        clang_disposeDiagnostic(diagnostic);
     }
 }
 
diff --git a/src/qdoc/docbookgenerator.cpp b/src/qdoc/docbookgenerator.cpp
index 242338c01..2f1fd27ff 100644
--- a/src/qdoc/docbookgenerator.cpp
+++ b/src/qdoc/docbookgenerator.cpp
@@ -2454,6 +2454,7 @@ void DocBookGenerator::endDocument()
     m_writer->writeEndElement(); // article
     m_writer->writeEndDocument();
     m_writer->device()->close();
+    delete m_writer->device();
     delete m_writer;
     m_writer = nullptr;
 }
diff --git a/src/qdoc/namespacenode.cpp b/src/qdoc/namespacenode.cpp
index d931a543d..4f48cffa5 100644
--- a/src/qdoc/namespacenode.cpp
+++ b/src/qdoc/namespacenode.cpp
@@ -48,24 +48,6 @@ QT_BEGIN_NAMESPACE
  */
 
 /*!
-  The destructor removes all children from the child list that
-  have a parent() that is not this NamespaceNode. This situation
-  can arise because of elements that are related to this namespace
-  using the \c {\\relates} command.
-
-  \note The child nodes remaining in the child list after the ones
-  with a different parent() have been removed are deleted in the
-  destructor of the Aggregate base class.
- */
-NamespaceNode::~NamespaceNode()
-{
-    for (auto &child : m_children) {
-        if (child->parent() != this)
-            child = nullptr;
-    }
-}
-
-/*!
   Returns true if this namespace is to be documented in the
   current module. There can be elements declared in this
   namespace spread over multiple modules. Those elements are
diff --git a/src/qdoc/namespacenode.h b/src/qdoc/namespacenode.h
index 373759968..151a08125 100644
--- a/src/qdoc/namespacenode.h
+++ b/src/qdoc/namespacenode.h
@@ -42,7 +42,7 @@ class NamespaceNode : public Aggregate
 {
 public:
     NamespaceNode(Aggregate *parent, const QString &name) : Aggregate(Namespace, parent, name) {}
-    ~NamespaceNode() override;
+    ~NamespaceNode() override = default;
     [[nodiscard]] Tree *tree() const override { return (parent() ? parent()->tree() : m_tree); }
 
     [[nodiscard]] bool isFirstClassAggregate() const override { return true; }
diff --git a/src/qdoc/tree.cpp b/src/qdoc/tree.cpp
index 5b73bd717..9247718e8 100644
--- a/src/qdoc/tree.cpp
+++ b/src/qdoc/tree.cpp
@@ -80,24 +80,19 @@ Tree::Tree(const QString &camelCaseModuleName, QDocDatabase *qdb)
 }
 
 /*!
-  Destroys the Tree. The root node is a data member
-  of this object, so its destructor is called. The
-  destructor of each child node is called, and these
-  destructors are recursive. Thus the entire tree is
-  destroyed.
+  Destroys the Tree.
 
   There are two maps of targets, keywords, and contents.
-  One map is indexed by ref, the other by title. The ref
-  is just the canonical form of the title. Both maps
+  One map is indexed by ref, the other by title. Both maps
   use the same set of TargetRec objects as the values,
-  so the destructor only deletes the values from one of
-  the maps. Then it clears both maps.
+  so we only need to delete the values from one of them.
+
+  The Node instances themselves are destroyed by the root
+  node's (\c m_root) destructor.
  */
 Tree::~Tree()
 {
-    for (auto i = m_nodesByTargetRef.begin(); i != m_nodesByTargetRef.end(); ++i) {
-        delete i.value();
-    }
+    qDeleteAll(m_nodesByTargetRef);
     m_nodesByTargetRef.clear();
     m_nodesByTargetTitle.clear();
 }