diff options
author | Topi Reinio <topi.reinio@qt.io> | 2021-11-05 12:30:37 +0100 |
---|---|---|
committer | Qt Cherry-pick Bot <cherrypick_bot@qt-project.org> | 2021-11-06 10:50:24 +0000 |
commit | 40785d2b4107b7f86e9464f4696189c30cf5f740 (patch) | |
tree | a916e00fe19ef8db8b8adda3570b5e00a5ce68ca | |
parent | 27de6e52e5104d97a214bfa2ea8982f23d182fe4 (diff) | |
download | qttools-40785d2b4107b7f86e9464f4696189c30cf5f740.tar.gz |
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>
-rw-r--r-- | src/qdoc/aggregate.cpp | 49 | ||||
-rw-r--r-- | src/qdoc/aggregate.h | 1 | ||||
-rw-r--r-- | src/qdoc/clangcodeparser.cpp | 4 | ||||
-rw-r--r-- | src/qdoc/docbookgenerator.cpp | 1 | ||||
-rw-r--r-- | src/qdoc/namespacenode.cpp | 18 | ||||
-rw-r--r-- | src/qdoc/namespacenode.h | 2 | ||||
-rw-r--r-- | src/qdoc/tree.cpp | 19 |
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(); } |