fix: avoid reentrant deadlock in DSysInfo::ensureOsVersion logging#570
Open
yixinshark wants to merge 1 commit into
Open
fix: avoid reentrant deadlock in DSysInfo::ensureOsVersion logging#570yixinshark wants to merge 1 commit into
yixinshark wants to merge 1 commit into
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yixinshark The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuideRefactors DSysInfoPrivate::ensureOsVersion to avoid holding the internal mutex while emitting qWarning messages by deferring construction of warning text and moving logging outside the critical section, and adjusts related helpers and callers accordingly. Sequence diagram for updated ensureOsVersion logging without holding mutexsequenceDiagram
participant Client
participant DSysInfoPrivate
participant QMutex
participant DDesktopEntry
participant QtMessageHandler
Client->>DSysInfoPrivate: ensureOsVersion()
DSysInfoPrivate->>QMutex: QMutexLocker(mutex)
DSysInfoPrivate->>DSysInfoPrivate: check osBuild.A and inTest()
alt [osBuild.A > 0 and not inTest]
QMutex-->>DSysInfoPrivate: unlocked
DSysInfoPrivate-->>Client: return true
else [need to read OS_VERSION_FILE]
QMutex-->>DSysInfoPrivate: unlocked
DSysInfoPrivate->>DDesktopEntry: DDesktopEntry(OS_VERSION_FILE)
DSysInfoPrivate->>QMutex: QMutexLocker(mutex)
DSysInfoPrivate->>DDesktopEntry: status()
alt [entry.status() != DDesktopEntry::NoError]
DSysInfoPrivate->>DSysInfoPrivate: warningMessage = "entry status: %1"
QMutex-->>DSysInfoPrivate: unlocked
DSysInfoPrivate->>QtMessageHandler: qWarning() << "ensureOsVersion" << warningMessage
DSysInfoPrivate-->>Client: return false
else [parse version]
DSysInfoPrivate->>DSysInfoPrivate: parse OsBuild and minorVersion
DSysInfoPrivate->>DSysInfoPrivate: splitA_BC_DMode(&warningMessage)
QMutex-->>DSysInfoPrivate: unlocked
alt [warningMessage not empty]
DSysInfoPrivate->>QtMessageHandler: qWarning() << "ensureOsVersion" << warningMessage
end
DSysInfoPrivate-->>Client: return result
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/dsysinfo.cpp" line_range="253" />
<code_context>
bool DSysInfoPrivate::ensureOsVersion()
{
- QMutexLocker locker(&mutex);
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying `ensureOsVersion` by using a single mutex lock and linear control flow with one exit point for logging instead of a lambda and duplicated fast‑path logic.
You can keep the new warning behavior while simplifying `ensureOsVersion` by removing the lambda, the duplicated fast‑path, and the double locking. A single lock with a linear flow + one exit point for logging is enough.
For example:
```cpp
bool DSysInfoPrivate::ensureOsVersion()
{
QString warningMessage;
bool ok = true;
QMutexLocker locker(&mutex);
#ifndef OS_VERSION_TEST_FILE // Always re-read the file when testing!
if (osBuild.A > 0 && !inTest())
goto end;
#endif
DDesktopEntry entry(OS_VERSION_FILE);
if (entry.status() != DDesktopEntry::NoError) {
warningMessage = QStringLiteral("entry status: %1").arg(entry.status());
ok = false;
goto end;
}
// 先获取版本信息
// ABCDE.xyz.abc
const QString osb = entry.stringValue("OsBuild", "Version");
const QStringList osbs = osb.split('.');
ok = (osbs.size() >= 2 && osbs.value(0).size() == 5);
if (!ok) {
warningMessage = QStringLiteral("OsBuild version invalid!");
goto end;
}
#if QT_VERSION >= QT_VERSION_CHECK(5, 15, 0)
const QStringList &left = osbs.value(0).split(QString(), Qt::SkipEmptyParts);
#else
const QStringList &left = osbs.value(0).split(QString(), QString::SkipEmptyParts);
#endif
if (left.size() != 5) {
warningMessage = QStringLiteral("OsBuild version(ls) invalid!");
ok = false;
goto end;
}
int idx = 0;
auto parseDigitOrAZ = [&](const QString &s, bool *outOk) -> uint {
if (s.isEmpty()) { if (outOk) *outOk = false; return 0u; }
const QChar ch = s.at(0);
if (ch.isDigit()) { if (outOk) *outOk = true; return static_cast<uint>(ch.unicode() - '0'); }
if (ch >= 'A' && ch <= 'Z') { if (outOk) *outOk = true; return static_cast<uint>(ch.toLatin1()); }
if (outOk) *outOk = false;
return 0u;
};
osBuild.A = parseDigitOrAZ(left.value(idx++, "0"), &ok);
if (!ok) { warningMessage = QStringLiteral("OsBuild version(A) invalid!"); goto end; }
osBuild.B = parseDigitOrAZ(left.value(idx++, "0"), &ok);
if (!ok) { warningMessage = QStringLiteral("OsBuild version(B) invalid!"); goto end; }
osBuild.C = parseDigitOrAZ(left.value(idx++, "0"), &ok);
if (!ok) { warningMessage = QStringLiteral("OsBuild version(C) invalid!"); goto end; }
osBuild.D = parseDigitOrAZ(left.value(idx++, "0"), &ok);
if (!ok) { warningMessage = QStringLiteral("OsBuild version(D) invalid!"); goto end; }
osBuild.E = parseDigitOrAZ(left.value(idx++, "0"), &ok);
if (!ok) { warningMessage = QStringLiteral("OsBuild version(E) invalid!"); goto end; }
// xyz
osBuild.xyz = osbs.value(1).trimmed().toUInt(&ok);
majorVersion = entry.stringValue("MajorVersion", "Version");
minorVersion = entry.stringValue("MinorVersion", "Version");
switch (osBuild.D) {
case 7: {
const QStringList &versionList = minorVersion.split('.');
if (versionList.isEmpty()) {
warningMessage = QStringLiteral("no minorVersion");
ok = false;
goto end;
} else if (versionList.length() == 2) {
minVersion.X = versionList.first().toUInt();
minVersion.Y = versionList.last().toUInt();
minVersion.Z = 0;
} else if (versionList.length() == 3) {
minVersion.X = versionList.at(0).toUInt();
minVersion.Y = versionList.at(1).toUInt();
minVersion.Z = versionList.at(2).toUInt();
}
minVersion.type = MinVersion::X_Y_Z;
} break;
case 3: {
bool a_bc_dMode = false;
const QStringList &versionList = minorVersion.split('.');
if (versionList.isEmpty()) {
warningMessage = QStringLiteral("no minorVersion");
ok = false;
goto end;
} else if (versionList.length() == 1) {
QString modeVersion = versionList.first();
if (modeVersion.length() == 2) {
minVersion.A = modeVersion.toUInt();
minVersion.B = 0;
minVersion.C = 0;
} else {
// A_BC_D mode
splitA_BC_DMode(&warningMessage);
a_bc_dMode = true;
}
} else if (versionList.length() == 2) {
minVersion.A = versionList.first().toUInt();
minVersion.B = versionList.last().toUInt();
minVersion.C = 0;
} else if (versionList.length() == 3) {
minVersion.A = versionList.at(0).toUInt();
minVersion.B = versionList.at(1).toUInt();
minVersion.C = versionList.at(2).toUInt();
}
if (!a_bc_dMode)
minVersion.type = MinVersion::A_B_C;
} break;
default: {
// A-BC-D
ok = splitA_BC_DMode(&warningMessage);
} break;
}
end:
if (!warningMessage.isEmpty())
qWarning() << "ensureOsVersion" << warningMessage;
return ok;
}
```
This keeps:
- Single `QMutexLocker` and a single fast‑path check.
- Centralized logging via `warningMessage` at one exit point.
- All your new, more descriptive warning messages.
- Existing `splitA_BC_DMode(&warningMessage)` behavior.
But it removes:
- The inline lambda and its extra indentation / scope.
- The duplicated locking and duplicated `osBuild.A > 0 && !inTest()` check.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -251,128 +252,162 @@ void DSysInfoPrivate::ensureDeepinInfo() | |||
|
|
|||
| bool DSysInfoPrivate::ensureOsVersion() | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying ensureOsVersion by using a single mutex lock and linear control flow with one exit point for logging instead of a lambda and duplicated fast‑path logic.
You can keep the new warning behavior while simplifying ensureOsVersion by removing the lambda, the duplicated fast‑path, and the double locking. A single lock with a linear flow + one exit point for logging is enough.
For example:
bool DSysInfoPrivate::ensureOsVersion()
{
QString warningMessage;
bool ok = true;
QMutexLocker locker(&mutex);
#ifndef OS_VERSION_TEST_FILE // Always re-read the file when testing!
if (osBuild.A > 0 && !inTest())
goto end;
#endif
DDesktopEntry entry(OS_VERSION_FILE);
if (entry.status() != DDesktopEntry::NoError) {
warningMessage = QStringLiteral("entry status: %1").arg(entry.status());
ok = false;
goto end;
}
// 先获取版本信息
// ABCDE.xyz.abc
const QString osb = entry.stringValue("OsBuild", "Version");
const QStringList osbs = osb.split('.');
ok = (osbs.size() >= 2 && osbs.value(0).size() == 5);
if (!ok) {
warningMessage = QStringLiteral("OsBuild version invalid!");
goto end;
}
#if QT_VERSION >= QT_VERSION_CHECK(5, 15, 0)
const QStringList &left = osbs.value(0).split(QString(), Qt::SkipEmptyParts);
#else
const QStringList &left = osbs.value(0).split(QString(), QString::SkipEmptyParts);
#endif
if (left.size() != 5) {
warningMessage = QStringLiteral("OsBuild version(ls) invalid!");
ok = false;
goto end;
}
int idx = 0;
auto parseDigitOrAZ = [&](const QString &s, bool *outOk) -> uint {
if (s.isEmpty()) { if (outOk) *outOk = false; return 0u; }
const QChar ch = s.at(0);
if (ch.isDigit()) { if (outOk) *outOk = true; return static_cast<uint>(ch.unicode() - '0'); }
if (ch >= 'A' && ch <= 'Z') { if (outOk) *outOk = true; return static_cast<uint>(ch.toLatin1()); }
if (outOk) *outOk = false;
return 0u;
};
osBuild.A = parseDigitOrAZ(left.value(idx++, "0"), &ok);
if (!ok) { warningMessage = QStringLiteral("OsBuild version(A) invalid!"); goto end; }
osBuild.B = parseDigitOrAZ(left.value(idx++, "0"), &ok);
if (!ok) { warningMessage = QStringLiteral("OsBuild version(B) invalid!"); goto end; }
osBuild.C = parseDigitOrAZ(left.value(idx++, "0"), &ok);
if (!ok) { warningMessage = QStringLiteral("OsBuild version(C) invalid!"); goto end; }
osBuild.D = parseDigitOrAZ(left.value(idx++, "0"), &ok);
if (!ok) { warningMessage = QStringLiteral("OsBuild version(D) invalid!"); goto end; }
osBuild.E = parseDigitOrAZ(left.value(idx++, "0"), &ok);
if (!ok) { warningMessage = QStringLiteral("OsBuild version(E) invalid!"); goto end; }
// xyz
osBuild.xyz = osbs.value(1).trimmed().toUInt(&ok);
majorVersion = entry.stringValue("MajorVersion", "Version");
minorVersion = entry.stringValue("MinorVersion", "Version");
switch (osBuild.D) {
case 7: {
const QStringList &versionList = minorVersion.split('.');
if (versionList.isEmpty()) {
warningMessage = QStringLiteral("no minorVersion");
ok = false;
goto end;
} else if (versionList.length() == 2) {
minVersion.X = versionList.first().toUInt();
minVersion.Y = versionList.last().toUInt();
minVersion.Z = 0;
} else if (versionList.length() == 3) {
minVersion.X = versionList.at(0).toUInt();
minVersion.Y = versionList.at(1).toUInt();
minVersion.Z = versionList.at(2).toUInt();
}
minVersion.type = MinVersion::X_Y_Z;
} break;
case 3: {
bool a_bc_dMode = false;
const QStringList &versionList = minorVersion.split('.');
if (versionList.isEmpty()) {
warningMessage = QStringLiteral("no minorVersion");
ok = false;
goto end;
} else if (versionList.length() == 1) {
QString modeVersion = versionList.first();
if (modeVersion.length() == 2) {
minVersion.A = modeVersion.toUInt();
minVersion.B = 0;
minVersion.C = 0;
} else {
// A_BC_D mode
splitA_BC_DMode(&warningMessage);
a_bc_dMode = true;
}
} else if (versionList.length() == 2) {
minVersion.A = versionList.first().toUInt();
minVersion.B = versionList.last().toUInt();
minVersion.C = 0;
} else if (versionList.length() == 3) {
minVersion.A = versionList.at(0).toUInt();
minVersion.B = versionList.at(1).toUInt();
minVersion.C = versionList.at(2).toUInt();
}
if (!a_bc_dMode)
minVersion.type = MinVersion::A_B_C;
} break;
default: {
// A-BC-D
ok = splitA_BC_DMode(&warningMessage);
} break;
}
end:
if (!warningMessage.isEmpty())
qWarning() << "ensureOsVersion" << warningMessage;
return ok;
}This keeps:
- Single
QMutexLockerand a single fast‑path check. - Centralized logging via
warningMessageat one exit point. - All your new, more descriptive warning messages.
- Existing
splitA_BC_DMode(&warningMessage)behavior.
But it removes:
- The inline lambda and its extra indentation / scope.
- The duplicated locking and duplicated
osBuild.A > 0 && !inTest()check.
yixinshark
force-pushed
the
fix-reentrant-log-deadlock
branch
from
38bd83b to
6205648
Compare
Move qWarning output out of the mutex-held region in DSysInfoPrivate::ensureOsVersion. Previously, qWarning was called while holding DSysInfoPrivate::mutex, which can deadlock if the installed Qt message handler (dtklog) re-enters DSysInfo during log formatting. Now warning text is collected into a local QString and emitted after the lock is released. Also refactor splitA_BC_DMode to accept an optional warningMessage output parameter so its diagnostic is deferred the same way, and replace the D_ASSET_EXIT macro with explicit early returns for clarity. The DDesktopEntry construction is moved before the lock so its internal warnings also stay outside the critical section. 修复 DSysInfo::ensureOsVersion 中持锁日志导致的重入死锁 将 qWarning 输出从 DSysInfoPrivate::ensureOsVersion 持锁区域移到 锁释放之后。此前在持有 DSysInfoPrivate::mutex 时调用 qWarning，若 已安装的 Qt 消息处理器（dtklog）在日志格式化过程中再次进入 DSysInfo，会导致死锁。现在告警信息先收集到局部 QString，在释放 锁后再输出。 同时重构 splitA_BC_DMode 增加可选的 warningMessage 输出参数，使 其诊断信息同样延迟输出；用显式提前返回替代 D_ASSET_EXIT 宏以 提升可读性。DDesktopEntry 构造移到加锁之前，使其内部告警也保持 在临界区之外。 Log: fix reentrant deadlock in DSysInfo::ensureOsVersion logging Change-Id: I031975ecf72528d1c2f3e48bd546e7d710641df5
yixinshark
force-pushed
the
fix-reentrant-log-deadlock
branch
from
6205648 to
39fa460
Compare
Contributor
deepin pr auto review★ 总体评分：95分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 --- a/src/dsysinfo.cpp
+++ b/src/dsysinfo.cpp
@@ -91,7 +91,7 @@ class Q_DECL_HIDDEN DSysInfoPrivate
};
MinVersion minVersion;
OSBuild osBuild;
- bool osVersionParsed = false;
+ std::atomic<bool> osVersionParsed{false};
bool splitA_BC_DMode(const QString &minorVersion, MinVersion *minVersion, QString *warningMessage = nullptr);
#endif |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
qWarningoutput out of the mutex-held region inDSysInfoPrivate::ensureOsVersionto prevent deadlock when the Qt message handler (dtklog) re-entersDSysInfoduring log formattingsplitA_BC_DModeto accept an optionalwarningMessageoutput parameter so its diagnostic is deferred the same wayDDesktopEntryconstruction before the lock so its internal warnings also stay outside the critical section; replaceD_ASSET_EXITmacro with explicit early returnsContext
Coordinates with a companion change in dtklog that reduces reentrancy surface in the log formatting path.
Test plan
ut_dsysinfoto verify version parsing behavior unchangedqWarningis emitted whileDSysInfoPrivate::mutexis heldSummary by Sourcery
Prevent reentrant deadlocks in DSysInfo OS version handling by moving warning logging and desktop entry construction outside the mutex-critical section and restructuring error handling.
Bug Fixes:
Enhancements: