From 44dcaf01fd73256ae2d826fc3124d41650b81d9f Mon Sep 17 00:00:00 2001
From: Emmanuel Lepage Vallee <elv1313@gmail.com>
Date: Thu, 20 Apr 2017 20:04:13 -0400
Subject: [PATCH] certificatemodel: Make the CertificateModel reentrant
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
First of all, [begin/end][Insert/Remove][Rows/Columns] is a
flat transaction and can only exist once per model. The code
was calling begin in other begin section due to some
lazy-loading code. This could result in SIGSEGV if the model
had active proxies.
More importantly, this code is called from the collections.
Some collections run in different threads. The class had a
mutex, but only used it in half the places it should have
been used. This caused a crash on the macOS and KDE client.
I could not manage to crash the Gnome client, but it is
probably possible. ASAN also got asserted due to this even
when it would have otherwise not crashed.
The commit alse fix a memory leak when the ID was duplicated
by the race condition. This is a band-aid on the problem: IDs
potentially not unique. However a band-aid is better than
acrash and the patch to fix this would be too invasive to get
past Gerrit.
Change-Id: I92e313d0243659b6af374a0cbf8bab18c3f6ba76
Reviewed-by: Nicolas Jäger <nicolas.jager@savoirfairelinux.com>
---
src/certificatemodel.cpp | 78 ++++++++++++++++++++++++++++------------
1 file changed, 55 insertions(+), 23 deletions(-)
diff --git a/src/certificatemodel.cpp b/src/certificatemodel.cpp
index a2ce65b7..9eac4930 100644
--- a/src/certificatemodel.cpp
+++ b/src/certificatemodel.cpp
@@ -237,8 +237,18 @@ void CertificateNode::setStrings(const QString& col1, const QVariant& col2, cons
CertificateNode* CertificateModelPrivate::createCategory(const QString& name, const QString& col2, const QString& tooltip )
{
+ QMutexLocker l(&m_CertLoader);
const int idx = m_lTopLevelNodes.size();
+ // This should be avoided whenever possible. Having a duplicate would be
+ // both a memory leak and a potential collision attack (far fetched).
+ // This code avoids leak, but has side effects.
+ if (m_hStrToCat.contains(name)) {
+ qWarning() << "Trying to create a certificate node with an already used id."
+ " This can have unforneen consequences";
+ return m_hStrToCat[name];
+ }
+
CertificateNode* n = new CertificateNode(idx, CertificateModel::NodeType::CATEGORY, nullptr, nullptr);
n->setStrings(name,col2,tooltip);
@@ -297,7 +307,7 @@ void CertificateModelPrivate::removeFromTree(Certificate* c, CertificateNode* ca
CertificateNode* CertificateModelPrivate::getCategory(const Account* a)
{
- CertificateNode* cat = m_hAccToCat[a];
+ CertificateNode* cat = m_hAccToCat.value(a);
if (!cat) {
cat = createCategory(a->alias(),QString(),QString());
@@ -321,7 +331,7 @@ CertificateNode* CertificateModelPrivate::addToTree(Certificate* cert, Account*
void CertificateModelPrivate::regenChecks(Certificate* cert)
{
- CertificateNode* n = m_hNodes[cert];
+ CertificateNode* n = m_hNodes.value(cert);
if (!n)
return;
@@ -334,6 +344,7 @@ void CertificateModelPrivate::regenChecks(Certificate* cert)
//[Re]generate the checks
void CertificateModelPrivate::loadChecks(CertificateNode* checks, Certificate* cert)
{
+ QMutexLocker locker(&m_CertLoader);
const QModelIndex checksI(q_ptr->createIndex(checks->m_Index,static_cast<int>(CertificateModel::Columns::NAME ),checks));
//Clear the existing nodes
@@ -368,13 +379,14 @@ void CertificateModelPrivate::loadChecks(CertificateNode* checks, Certificate* c
CertificateNode* CertificateModelPrivate::addToTree(Certificate* cert, CertificateNode* category)
{
- QMutexLocker locker(&m_CertLoader);
-
if (!category)
category = defaultCategory();
+ // defaultCategory(); can request the mutex
+ QMutexLocker locker(&m_CertLoader);
+
//Do not add it twice
- CertificateNode* node = CertificateModel::instance().d_ptr->m_hNodes[cert];
+ CertificateNode* node = CertificateModel::instance().d_ptr->m_hNodes.value(cert);
if (isPartOf(node, category))
return node;
@@ -391,24 +403,41 @@ CertificateNode* CertificateModelPrivate::addToTree(Certificate* cert, Certifica
//Lazy loaded function to reduce the overhead of this (mostly hidden) model
node->m_fLoader = [this,node,cert]() {
+ CertificateNode* checks = nullptr;
+ { // mutex
node->m_Col1 = cert->detailResult(Certificate::Details::PUBLIC_KEY_ID).toString();
node->m_IsLoaded = true;
const QModelIndex index = q_ptr->createIndex(node->m_Index,static_cast<int>(CertificateModel::Columns::NAME ),node);
+ CertificateNode* details = nullptr;
+
//Insert the check and details categories
+ { // mutex
+ QMutexLocker locker(&m_CertLoader);
q_ptr->beginInsertRows(index, 0, 1);
- CertificateNode* details = new CertificateNode(static_cast<int>(CertificateModel::Columns::NAME ), CertificateModel::NodeType::DETAILS_CATEGORY, node, nullptr);
- CertificateNode* checks = new CertificateNode(static_cast<int>(CertificateModel::Columns::VALUE), CertificateModel::NodeType::DETAILS_CATEGORY, node, nullptr);
+ details = new CertificateNode(static_cast<int>(CertificateModel::Columns::NAME ), CertificateModel::NodeType::DETAILS_CATEGORY, node, nullptr);
+ checks = new CertificateNode(static_cast<int>(CertificateModel::Columns::VALUE), CertificateModel::NodeType::DETAILS_CATEGORY, node, nullptr);
details->setStrings(QObject::tr("Details"),QString(),QObject::tr("The content of the certificate") );
checks ->setStrings(QObject::tr("Checks") ,QString(),QObject::tr("Various security related information") );
node->m_lChildren << details;
node->m_lChildren << checks;
q_ptr->endInsertRows();
+ } // mutex
static const int detailsC(enum_class_size<Certificate::Details>());
- //Insert the details
const QModelIndex detailsI(q_ptr->createIndex(details->m_Index,static_cast<int>(CertificateModel::Columns::NAME ),details));
+
+ // Make sure the lazy-loaded details have been created (otherwise it will deadlock)
+ for (const Certificate::Details detail : EnumIterator<Certificate::Details>()) {
+ cert->getName (detail);
+ cert->detailResult (detail);
+ cert->getDescription(detail);
+ }
+
+ //Insert the details
+ { // mutex
+ QMutexLocker locker(&m_CertLoader);
q_ptr->beginInsertRows(detailsI, 0, detailsC - 1);
for (const Certificate::Details detail : EnumIterator<Certificate::Details>()) {
CertificateNode* d = new CertificateNode(details->m_lChildren.size(), CertificateModel::NodeType::DETAILS, details, nullptr);
@@ -419,6 +448,9 @@ CertificateNode* CertificateModelPrivate::addToTree(Certificate* cert, Certifica
details->m_lChildren << d;
}
q_ptr->endInsertRows();
+ } // mutex
+
+ } // mutex
//Insert the checks
this->loadChecks(checks, cert);
@@ -592,7 +624,7 @@ Certificate* CertificateModel::getCertificateFromPath(const QString& path, Accou
CertificateNode* cat = d_ptr->getCategory(a);
- Certificate* cert = d_ptr->m_hCertificates[path];
+ Certificate* cert = d_ptr->m_hCertificates.value(path);
if (!cert) {
cert = new Certificate(path, Certificate::Type::NONE);
@@ -603,7 +635,7 @@ Certificate* CertificateModel::getCertificateFromPath(const QString& path, Accou
d_ptr->addToTree(cert,a);
}
- CertificateNode* node = d_ptr->m_hNodes[cert];
+ CertificateNode* node = d_ptr->m_hNodes.value(cert);
if (node) {
if (node->m_pParent != cat) {
@@ -618,7 +650,7 @@ Certificate* CertificateModel::getCertificateFromPath(const QString& path, Certi
{
const QString id = path;
- Certificate* cert = d_ptr->m_hCertificates[id];
+ Certificate* cert = d_ptr->m_hCertificates.value(id);
//The certificate is not loaded yet
if (!cert) {
@@ -635,7 +667,7 @@ Certificate* CertificateModel::getCertificateFromPath(const QString& path, Certi
Certificate* CertificateModel::getCertificateFromId(const QString& id, Account* a, const QString& category)
{
- Certificate* cert = d_ptr->m_hCertificates[id];
+ Certificate* cert = d_ptr->m_hCertificates.value(id);
//The certificate is not loaded yet
if (!cert) {
@@ -643,7 +675,7 @@ Certificate* CertificateModel::getCertificateFromId(const QString& id, Account*
d_ptr->m_hCertificates[id.toLatin1()] = cert;
if ((!a) && (!category.isEmpty())) {
- CertificateNode* cat = d_ptr->m_hStrToCat[category];
+ CertificateNode* cat = d_ptr->m_hStrToCat.value(category);
if (!cat) {
cat = d_ptr->createCategory(category, a?QString("%1 certificates").arg(a->alias()):QString(), QString());
@@ -760,7 +792,7 @@ QAbstractItemModel* CertificateModelPrivate::model(const Certificate* cert) cons
{
if (!cert)
return nullptr;
- return const_cast<CertificateModelPrivate*>(this)->getModelCommon(m_hNodes[cert]);
+ return const_cast<CertificateModelPrivate*>(this)->getModelCommon(m_hNodes.value(cert));
}
/**
@@ -771,7 +803,7 @@ QAbstractItemModel* CertificateModelPrivate::checksModel(const Certificate* cert
if (!cert)
return nullptr;
- CertificateNode* node = m_hNodes[cert];
+ CertificateNode* node = m_hNodes.value(cert);
if (!node)
return nullptr;
@@ -827,7 +859,7 @@ QAbstractItemModel* CertificateModelPrivate::createKnownList(const Account* a) c
QAbstractItemModel* CertificateModelPrivate::createBannedList(const Account* a) const
{
- QAbstractItemModel* m = m_hAccBan[a];
+ QAbstractItemModel* m = m_hAccBan.value(a);
if (m)
return m;
@@ -844,7 +876,7 @@ QAbstractItemModel* CertificateModelPrivate::createBannedList(const Account* a)
QAbstractItemModel* CertificateModelPrivate::createAllowedList(const Account* a) const
{
- QAbstractItemModel* m = m_hAccAllow[a];
+ QAbstractItemModel* m = m_hAccAllow.value(a);
if (m)
return m;
@@ -868,9 +900,9 @@ bool CertificateModelPrivate::allowCertificate(Certificate* c, Account* a)
createAllowedList(a);
createBannedList(a);
- CertificateNode* allow = m_hAccAllowCat [ a ];
- CertificateNode* ban = m_hAccBanCat [ a ];
- CertificateNode* sibling = m_hNodes [ c ];
+ CertificateNode* allow = m_hAccAllowCat .value( a );
+ CertificateNode* ban = m_hAccBanCat .value( a );
+ CertificateNode* sibling = m_hNodes .value( c );
//Check if it's already there
if (isPartOf(sibling, allow))
@@ -907,9 +939,9 @@ bool CertificateModelPrivate::banCertificate(Certificate* c, Account* a)
createAllowedList(a);
createBannedList(a);
- CertificateNode* allow = m_hAccAllowCat [ a ];
- CertificateNode* ban = m_hAccBanCat [ a ];
- CertificateNode* sibling = m_hNodes [ c ];
+ CertificateNode* allow = m_hAccAllowCat .value( a );
+ CertificateNode* ban = m_hAccBanCat .value( a );
+ CertificateNode* sibling = m_hNodes .value( c );
//Check if it's already there
if (isPartOf(sibling, ban))
--
GitLab