tests: add service tests #479
Merged
Conversation
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
add the debug for service-manager-plugin-network Log: add debug for service-manager
Reviewer's GuideRefactors the example test application away from the legacy DCC network plugin UI into a minimal Qt6/Dtk6-based widget and new DBus service test harness, wiring in a policy-driven service manager implementation and enabling the example build under Qt6. Sequence diagram for DBus call filtering via QTDbusHook and PolicysequenceDiagram
participant ClientApp
participant DBusDaemon
participant QtDBus as Qt_DBUS
participant QTDbusHook as QTDbusHook_singleton
participant Service as ServiceQtDBus
participant Policy
ClientApp->>DBusDaemon: DBus method call (service, path, interface, member)
DBusDaemon->>QtDBus: Deliver message to service process
QtDBus->>QTDbusHook: QTDBusSpyHook(msg) or QTDBusHook(baseService, msg)
QTDbusHook->>QTDbusHook: getServiceObject("", msg.path, servicePtr, isSubPath, realPath)
alt service object found
QTDbusHook->>Service: isRegister()
alt not registered
QTDbusHook->>Service: registerService()
end
alt msg is Introspect on org.freedesktop.DBus.Introspectable
QTDbusHook->>Policy: checkPathHide(realPath)
alt path hidden
QTDbusHook->>Service: qDbusConnection()
Service-->>QtDBus: send empty introspection reply
QtDBus-->>DBusDaemon: send reply
DBusDaemon-->>ClientApp: receive reply
else path visible
QTDbusHook-->>QtDBus: allow normal handling
end
else msg is Set on org.freedesktop.DBus.Properties
QTDbusHook->>Service: qDbusConnection()
QTDbusHook->>Policy: checkPropertyPermission(cmd, realPath, iface, property)
alt permission denied
QTDbusHook-->>QtDBus: send error reply Permission.Deny
QtDBus-->>DBusDaemon: send error
DBusDaemon-->>ClientApp: receive error
else permission granted
QTDbusHook-->>QtDBus: allow normal handling
end
else other interface and member
QTDbusHook->>Policy: checkMethodPermission(cmd, realPath, interface, member)
alt permission denied
QTDbusHook->>Service: qDbusConnection()
QTDbusHook-->>QtDBus: send error reply Permission.Deny
QtDBus-->>DBusDaemon: send error
DBusDaemon-->>ClientApp: receive error
else permission granted
QTDbusHook-->>QtDBus: allow normal handling
end
end
else service object not found
QTDbusHook-->>QtDBus: allow normal handling
end
Class diagram for new service manager and policy typesclassDiagram
class Policy {
+QString name
+QString group
+QString pluginPath
+QString version
+QString startType
+QStringList dependencies
+SDKType sdkType
+int startDelay
+int idleTime
+QMapWhitelists mapWhitelist
+QMapPathHide mapPathHide
+QMapSubPath mapSubPath
+QMapPath mapPath
+Policy(QObject *parent)
+void parseConfig(QString path)
+bool checkPathHide(QString path)
+bool checkMethodPermission(QString process, QString path, QString interface, QString method)
+bool checkPropertyPermission(QString process, QString path, QString interface, QString property)
+bool checkPermission(QString process, QString path, QString interface, QString dest, CallDestType type)
+QStringList paths()
+bool allowSubPath(QString path)
+bool isResident()
+void print()
-bool readJsonFile(QJsonDocument outDoc, QString fileName)
-bool parseWhitelist(QJsonObject obj)
-bool parsePolicy(QJsonObject obj)
-bool parsePolicyPath(QJsonObject obj)
-bool parsePolicyInterface(QJsonObject obj, PolicyPath policyPath)
-bool parsePolicyMethod(QJsonObject obj, PolicyInterface policyInterface)
-bool parsePolicyProperties(QJsonObject obj, PolicyInterface policyInterface)
-bool jsonGetString(QJsonObject obj, QString key, QString value, QString defaultValue)
-bool jsonGetStringList(QJsonObject obj, QString key, QStringList value, QStringList defaultValue)
-bool jsonGetBool(QJsonObject obj, QString key, bool value, bool defaultValue)
-bool jsonGetInt(QJsonObject obj, QString key, int value, int defaultValue)
}
class PolicyWhitelist {
+QString name
+QStringList process
}
class PolicyMethod {
+QString method
+bool needPermission
+QStringList processes
}
class PolicyProperty {
+QString property
+bool needPermission
+QStringList processes
}
class PolicyInterface {
+QString interface
+bool needPermission
+QStringList processes
+QMapMethod methods
+QMapProperty properties
}
class PolicyPath {
+QString path
+bool needPermission
+QStringList processes
+QMapInterface interfaces
}
class SDKType {
<<enum>>
QT
SD
}
class CallDestType {
<<enum>>
Method
Property
}
class ServiceBase {
+Policy *policy
+bool m_isRegister
+bool m_isLockTimer
+QDBusConnection.BusType m_sessionType
+SDKType m_SDKType
+QTimer *m_timer
+ServiceBase(QObject *parent)
+~ServiceBase()
+bool isRegister() const
+bool isLockTimer() const
+bool registerService()
+bool unregisterService()
+void init(QDBusConnection.BusType busType, Policy *p)
+void restartTimer()
+void idleSignal()
-void initService()
-void initThread()
}
class ServiceQtDBus {
+QLibrary *m_library
+ServiceQtDBus(QObject *parent)
+QDBusConnection qDbusConnection()
+bool registerService()
+bool unregisterService()
-void initThread()
-bool libFuncCall(QString funcName, bool isRegister)
}
class QTDbusHook {
-ServiceObjectMap m_serviceMap
+QTDbusHook()
+bool getServiceObject(QString name, QString path, ServiceBase **service, bool isSubPath, QString realPath)
+bool setServiceObject(ServiceBase *obj)
+static QTDbusHook *instance()
}
class ServiceObjectMap {
<<typedef>>
QMap~QString, ServiceBase *~
}
class QMapWhitelists {
<<typedef>>
QMap~QString, PolicyWhitelist~
}
class QMapPathHide {
<<typedef>>
QMap~QString, bool~
}
class QMapSubPath {
<<typedef>>
QMap~QString, bool~
}
class QMapMethod {
<<typedef>>
QMap~QString, PolicyMethod~
}
class QMapProperty {
<<typedef>>
QMap~QString, PolicyProperty~
}
class QMapInterface {
<<typedef>>
QMap~QString, PolicyInterface~
}
class QMapPath {
<<typedef>>
QMap~QString, PolicyPath~
}
ServiceQtDBus --|> ServiceBase
ServiceBase o--> Policy
Policy --> QMapWhitelists
Policy --> QMapPathHide
Policy --> QMapSubPath
Policy --> QMapPath
PolicyPath --> QMapInterface
PolicyInterface --> QMapMethod
PolicyInterface --> QMapProperty
QTDbusHook --> ServiceObjectMap
ServiceBase --> SDKType
Policy --> SDKType
Policy --> CallDestType
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto reviewGit Diff 代码审查报告总体评价这次代码变更主要涉及从Qt5迁移到Qt6，以及重构了example目录的代码结构。主要变化包括：
语法与逻辑审查CMakeLists.txt
example/dccplugintestwidget.cpp/h
example/main.cpp
新增服务相关文件
代码质量
代码性能
代码安全
具体改进建议
总结这次代码变更主要完成了从Qt5到Qt6的迁移，并重构了example目录的代码结构。整体上代码质量良好，但在错误处理、性能优化和安全性方面还有改进空间。建议在后续开发中：
这些改进将有助于提高代码的健壮性、性能和安全性。 |
![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 5 issues, and left some high level feedback:
- In main.cpp, the
Policy policy(nullptr);created on the stack is passed intoServiceQtDBus::init, which callsp->setParent(this)and transfers ownership to a heap QObject; this will lead to invalid QObject parenting and potential double-free whenpolicygoes out of scope, so either allocatePolicyon the heap or avoid setting its parent here. - In Policy::parsePolicyPath, the
subpathflag is read from JSON butmapSubPath.insert(path, pathHide);storespathHideinstead ofsubpath, which means the subpath configuration is never respected; this should insert thesubpathvalue. - In qtdbushook.cpp, the error reply path in the non-filter build (
QTDBusSpyHook) usesQDBusConnection::connectToBus(QDBusConnection::SessionBus, QString("org.dsdsf.dsfsdf"))instead of the service connection used elsewhere, which looks like leftover test code and likely prevents the error reply from reaching the caller; it should mirror the normal send path via the existing bus connection.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In main.cpp, the `Policy policy(nullptr);` created on the stack is passed into `ServiceQtDBus::init`, which calls `p->setParent(this)` and transfers ownership to a heap QObject; this will lead to invalid QObject parenting and potential double-free when `policy` goes out of scope, so either allocate `Policy` on the heap or avoid setting its parent here.
- In Policy::parsePolicyPath, the `subpath` flag is read from JSON but `mapSubPath.insert(path, pathHide);` stores `pathHide` instead of `subpath`, which means the subpath configuration is never respected; this should insert the `subpath` value.
- In qtdbushook.cpp, the error reply path in the non-filter build (`QTDBusSpyHook`) uses `QDBusConnection::connectToBus(QDBusConnection::SessionBus, QString("org.dsdsf.dsfsdf"))` instead of the service connection used elsewhere, which looks like leftover test code and likely prevents the error reply from reaching the caller; it should mirror the normal send path via the existing bus connection.
## Individual Comments
### Comment 1
<location> `example/service/policy.cpp:325-327` </location>
<code_context>
+ jsonGetBool(obj, "pathhide", pathHide, false);
+ mapPathHide.insert(path, pathHide);
+
+ bool subpath;
+ jsonGetBool(obj, "subpath", subpath, false);
+ mapSubPath.insert(path, pathHide);
+
+ PolicyPath policyPath;
</code_context>
<issue_to_address>
**issue (bug_risk):** Bug: `subpath` flag is read but `pathHide` is stored into `mapSubPath` instead.
In `parsePolicyPath`, you read `subpath` but then insert `pathHide` into `mapSubPath`, so the `subpath` flag is never used and `allowSubPath()` will behave incorrectly. This should be `mapSubPath.insert(path, subpath);`.
</issue_to_address>
### Comment 2
<location> `example/service/qtdbushook.cpp:84-93` </location>
<code_context>
+ .arg(serviceObj->policy->idleTime);
+ QTimer::singleShot(0, serviceObj, SLOT(restartTimer()));
+ }
+ if (msg.member() == "Introspect" && msg.interface() == "org.freedesktop.DBus.Introspectable") {
+ if (serviceObj->policy->checkPathHide(realPath)) {
+ qCDebug(dsm_hook_qt) << "--call Introspect" << msg.path() << " ,is hided!";
+ QList<QVariant> arguments;
+ arguments << "";
+ QDBusMessage reply = msg.createReply(arguments);
+ ServiceQtDBus *srv = qobject_cast<ServiceQtDBus *>(serviceObj);
+ if (srv) {
+ srv->qDbusConnection().send(reply);
+ }
+ // ((ServiceQtDBus*)serviceObj)->qDbusConnection().send(reply);
+ }
+ } else if (msg.member() == "Set" && msg.interface() == "org.freedesktop.DBus.Properties") {
</code_context>
<issue_to_address>
**issue (bug_risk):** Hidden Introspect replies are sent but the original call is not blocked in the filter hook.
In `QTDBusHook`, when `Introspect` is handled and `checkPathHide(realPath)` is true, you send a custom reply but still return `0`, so the original call proceeds and may still return real introspection data. After sending the reply, return a failure code (e.g. `-1`) to stop the original handling and actually hide the path.
</issue_to_address>
### Comment 3
<location> `example/service/qtdbushook.cpp:125-126` </location>
<code_context>
+ if (srv) {
+ // srv->qDbusConnection().send(reply);
+ // QDBusConnection::sessionBus().send(reply);
+ QDBusConnection::connectToBus(QDBusConnection::SessionBus,
+ QString("org.dsdsf.dsfsdf"))
+ .send(reply);
+ return;
</code_context>
<issue_to_address>
**issue (bug_risk):** Error replies are sent on a new, hard-coded DBus connection instead of the correct bus.
In `QTDBusSpyHook`, the error reply for denied calls is sent via `QDBusConnection::connectToBus(SessionBus, "org.dsdsf.dsfsdf")`, which creates/uses an unrelated connection, so the reply likely never reaches the original caller. Instead, send the reply on the same connection as the service (e.g. `srv->qDbusConnection().send(reply);`) and remove the dummy bus connection/commented-out code.
</issue_to_address>
### Comment 4
<location> `example/service/servicebase.cpp:43-47` </location>
<code_context>
+
+void ServiceBase::initService()
+{
+ QThread *th = new QThread();
+ setParent(nullptr);
+ moveToThread(th);
+ connect(th, &QThread::started, this, &ServiceBase::initThread);
+ th->start();
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Service threads are created without ownership or shutdown, which can leak threads or keep the process alive.
`initService` allocates a `QThread` with `new` and no parent, and there’s no visible shutdown or deletion path. This risks leaking threads and preventing clean application exit. Please ensure the thread is stopped when the service is unregistered/app exits, give it appropriate ownership, and connect `QThread::finished` to `deleteLater()` for cleanup.
Suggested implementation:
```cpp
#include "servicebase.h"
#include "policy.h"
#include <QDBusConnection>
#include <QDBusMessage>
#include <QDebug>
#include <QThread>
#include <QTimer>
void ServiceBase::initService()
{
// Create a dedicated thread for this service with proper ownership
QThread *th = new QThread(this);
// If the class has a thread member, e.g. QThread *m_thread, assign it here:
// m_thread = th;
// Move this service object to the worker thread
moveToThread(th);
// Initialize the service when the thread starts
connect(th, &QThread::started, this, &ServiceBase::initThread);
// Ensure the thread object is deleted when it finishes
connect(th, &QThread::finished, th, &QObject::deleteLater);
th->start();
}
```
To fully implement safe shutdown and avoid keeping the process alive, you should also:
1. In `servicebase.h`:
- Add a member to track the thread if it does not already exist:
`QThread *m_thread = nullptr;`
- Update `initService()` to assign `m_thread = th;` as commented in the implementation above.
2. In the destructor `ServiceBase::~ServiceBase()` (or wherever the service is unregistered, e.g. `unregisterService()`):
- Ensure the thread is asked to stop and waited on before destruction:
```cpp
if (m_thread) {
m_thread->quit();
m_thread->wait();
// m_thread will delete itself due to finished -> deleteLater connection
m_thread = nullptr;
}
```
- Make sure this code runs before the `ServiceBase` object is destroyed or before the application exits.
3. If multiple services share a thread or if the application has a central thread manager, adapt the ownership and shutdown logic accordingly so that the thread is not deleted while other objects still use it.
</issue_to_address>
### Comment 5
<location> `example/service/qtdbushook.cpp:56` </location>
<code_context>
+}
+
+// if it is not a local message, hook exec at main thread
+void QTDBusSpyHook(const QDBusMessage &msg)
+{
+ qCInfo(dsm_hook_qt) << "--msg=" << msg;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the shared DBus handling logic and simplifying `getServiceObject`’s interface to eliminate duplication and unused parameters while keeping current behavior intact.
You can significantly reduce complexity here without changing behavior by:
### 1. Deduplicating `QTDBusSpyHook` and `QTDBusHook`
Almost all of this code is copy‑pasted between the two hooks. You can centralize the common logic into a small helper and keep the per‑hook differences explicit.
One approach is to extract:
- Common: object lookup, registration, timer management, policy checks.
- Variant: how denial is signaled (send reply via which connection, return code, the special `connectToBus` in the spy hook).
For example:
```cpp
enum class HookResult {
Allow,
DenyWithReply, // reply already sent
NoServiceObject // object not found or unusable
};
static HookResult handleDbusMessage(ServiceBase *serviceObj,
const QDBusMessage &msg,
const QString &realPath,
bool useWeirdSpyDenyBehavior)
{
if (!serviceObj->isRegister()) {
qCInfo(dsm_hook_qt) << "--to register dbus object: " << msg.path();
serviceObj->registerService();
}
if (!serviceObj->policy->isResident() && !serviceObj->isLockTimer()) {
qCInfo(dsm_hook_qt) << QString("--service: %1 will unregister in %2 minutes!")
.arg(serviceObj->policy->name)
.arg(serviceObj->policy->idleTime);
QTimer::singleShot(0, serviceObj, SLOT(restartTimer()));
}
ServiceQtDBus *srv = qobject_cast<ServiceQtDBus *>(serviceObj);
if (!srv)
return HookResult::Allow;
// Introspect
if (msg.member() == "Introspect"
&& msg.interface() == "org.freedesktop.DBus.Introspectable") {
if (serviceObj->policy->checkPathHide(realPath)) {
qCDebug(dsm_hook_qt) << "--call Introspect" << msg.path() << " ,is hided!";
QList<QVariant> arguments;
arguments << "";
QDBusMessage reply = msg.createReply(arguments);
srv->qDbusConnection().send(reply);
return HookResult::DenyWithReply;
}
return HookResult::Allow;
}
// Properties.Set
if (msg.member() == "Set"
&& msg.interface() == "org.freedesktop.DBus.Properties") {
const QList<QVariant> &args = msg.arguments();
if (args.size() >= 2) {
if (!serviceObj->policy->checkPropertyPermission(
getCMD(serviceObj, msg.service()),
realPath,
args.at(0).toString(),
args.at(1).toString())) {
QDBusMessage reply = msg.createErrorReply(
"com.deepin.service.Permission.Deny", "The call is deny");
srv->qDbusConnection().send(reply);
return HookResult::DenyWithReply;
}
}
return HookResult::Allow;
}
// Normal methods
if (msg.interface() != "org.freedesktop.DBus.Properties"
&& msg.interface() != "org.freedesktop.DBus.Introspectable"
&& msg.interface() != "org.freedesktop.DBus.Peer")) {
if (!serviceObj->policy->checkMethodPermission(
getCMD(serviceObj, msg.service()),
realPath,
msg.interface(),
msg.member())) {
QDBusMessage reply = msg.createErrorReply(
"com.deepin.service.Permission.Deny", "The call is deny2");
if (useWeirdSpyDenyBehavior) {
// preserves current spy behavior
QDBusConnection::connectToBus(QDBusConnection::SessionBus,
QStringLiteral("org.dsdsf.dsfsdf"))
.send(reply);
} else {
srv->qDbusConnection().send(reply);
}
return HookResult::DenyWithReply;
}
}
return HookResult::Allow;
}
```
Then each hook becomes much smaller and easier to reason about:
```cpp
void QTDBusSpyHook(const QDBusMessage &msg)
{
ServiceBase *serviceObj = nullptr;
bool isSubPath;
QString realPath;
if (!QTDbusHook::instance()->getServiceObject(
QString(), msg.path(), &serviceObj, isSubPath, realPath)) {
qCWarning(dsm_hook_qt) << "--can not find hook object: " << msg.path();
return;
}
const auto result = handleDbusMessage(serviceObj, msg, realPath,
/*useWeirdSpyDenyBehavior=*/true);
Q_UNUSED(result); // spy hook does not need to return anything
}
```
```cpp
int QTDBusHook(const QString &baseService, const QDBusMessage &msg)
{
Q_UNUSED(baseService);
ServiceBase *serviceObj = nullptr;
bool isSubPath;
QString realPath;
if (!QTDbusHook::instance()->getServiceObject(
QString(), msg.path(), &serviceObj, isSubPath, realPath)) {
qCWarning(dsm_hook_qt) << "--can not find hook object:" << msg.path();
return 0;
}
const auto result = handleDbusMessage(serviceObj, msg, realPath,
/*useWeirdSpyDenyBehavior=*/false);
if (result == HookResult::DenyWithReply)
return -1;
return 0;
}
```
This keeps *all* current behaviors (including the unusual `connectToBus` path) but removes the large duplicated if‑else trees.
### 2. Clarifying / removing `isSubPath`
`getServiceObject` writes `isSubPath`, but in this file both call sites ignore it:
```cpp
bool isSubPath;
QString realPath;
QTDbusHook::instance()->getServiceObject("", msg.path(), &serviceObj, isSubPath, realPath);
```
To reduce mental overhead:
- If `isSubPath` is not used anywhere else, drop it from the signature and implementation:
```cpp
bool QTDbusHook::getServiceObject(QString name,
QString path,
ServiceBase **service,
QString &realPath)
{
Q_UNUSED(name);
auto iterService = m_serviceMap.find(path);
if (iterService != m_serviceMap.end()) {
*service = iterService.value();
realPath = iterService.key();
return true;
}
for (auto iter = m_serviceMap.begin(); iter != m_serviceMap.end(); ++iter) {
if (path.startsWith(iter.key()) && iter.value()->policy->allowSubPath(iter.key())) {
*service = iter.value();
realPath = iter.key();
return true;
}
}
return false;
}
```
- And adapt call sites accordingly:
```cpp
ServiceBase *serviceObj = nullptr;
QString realPath;
bool findRet = QTDbusHook::instance()->getServiceObject(
QString(), msg.path(), &serviceObj, realPath);
```
- If `isSubPath` is needed by other users of `getServiceObject`, consider either:
- using it in these hooks as well (and naming it clearly, e.g. `matchedExactPath`), or
- splitting into two overloads: one simple version for hooks, one detailed version for callers that care about the distinction.
### 3. Normalizing denial reply behavior
Right now, denial replies are constructed in multiple places with slightly different strings and sending mechanisms (including commented‑out code and the debug‑looking `org.dsdsf.dsfsdf` connection).
The shared helper above already centralizes:
- the error names,
- the messages,
- which connection is used.
If the odd `connectToBus` is intentional, keeping it behind a `useWeirdSpyDenyBehavior` flag makes that intent explicit and localizes the “special case,” instead of inlining it in a big branch. If it’s not intentional, this helper makes it very easy to swap it out in one place later without touching both hooks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+325
to
+327
| bool subpath; | ||
| jsonGetBool(obj, "subpath", subpath, false); | ||
| mapSubPath.insert(path, pathHide); |
There was a problem hiding this comment.
issue (bug_risk): Bug: subpath flag is read but pathHide is stored into mapSubPath instead.
In parsePolicyPath, you read subpath but then insert pathHide into mapSubPath, so the subpath flag is never used and allowSubPath() will behave incorrectly. This should be mapSubPath.insert(path, subpath);.
Comment on lines
+84
to
+93
| if (msg.member() == "Introspect" && msg.interface() == "org.freedesktop.DBus.Introspectable") { | ||
| if (serviceObj->policy->checkPathHide(realPath)) { | ||
| qCDebug(dsm_hook_qt) << "--call Introspect" << msg.path() << " ,is hided!"; | ||
| QList<QVariant> arguments; | ||
| arguments << ""; | ||
| QDBusMessage reply = msg.createReply(arguments); | ||
| ServiceQtDBus *srv = qobject_cast<ServiceQtDBus *>(serviceObj); | ||
| if (srv) { | ||
| srv->qDbusConnection().send(reply); | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): Hidden Introspect replies are sent but the original call is not blocked in the filter hook.
In QTDBusHook, when Introspect is handled and checkPathHide(realPath) is true, you send a custom reply but still return 0, so the original call proceeds and may still return real introspection data. After sending the reply, return a failure code (e.g. -1) to stop the original handling and actually hide the path.
Comment on lines
+125
to
+126
| QDBusConnection::connectToBus(QDBusConnection::SessionBus, | ||
| QString("org.dsdsf.dsfsdf")) |
There was a problem hiding this comment.
issue (bug_risk): Error replies are sent on a new, hard-coded DBus connection instead of the correct bus.
In QTDBusSpyHook, the error reply for denied calls is sent via QDBusConnection::connectToBus(SessionBus, "org.dsdsf.dsfsdf"), which creates/uses an unrelated connection, so the reply likely never reaches the original caller. Instead, send the reply on the same connection as the service (e.g. srv->qDbusConnection().send(reply);) and remove the dummy bus connection/commented-out code.
Comment on lines
+43
to
+47
| QThread *th = new QThread(); | ||
| setParent(nullptr); | ||
| moveToThread(th); | ||
| connect(th, &QThread::started, this, &ServiceBase::initThread); | ||
| th->start(); |
There was a problem hiding this comment.
suggestion (bug_risk): Service threads are created without ownership or shutdown, which can leak threads or keep the process alive.
initService allocates a QThread with new and no parent, and there’s no visible shutdown or deletion path. This risks leaking threads and preventing clean application exit. Please ensure the thread is stopped when the service is unregistered/app exits, give it appropriate ownership, and connect QThread::finished to deleteLater() for cleanup.
Suggested implementation:
#include "servicebase.h"
#include "policy.h"
#include <QDBusConnection>
#include <QDBusMessage>
#include <QDebug>
#include <QThread>
#include <QTimer>
void ServiceBase::initService()
{
// Create a dedicated thread for this service with proper ownership
QThread *th = new QThread(this);
// If the class has a thread member, e.g. QThread *m_thread, assign it here:
// m_thread = th;
// Move this service object to the worker thread
moveToThread(th);
// Initialize the service when the thread starts
connect(th, &QThread::started, this, &ServiceBase::initThread);
// Ensure the thread object is deleted when it finishes
connect(th, &QThread::finished, th, &QObject::deleteLater);
th->start();
}
To fully implement safe shutdown and avoid keeping the process alive, you should also:
-
In
servicebase.h:- Add a member to track the thread if it does not already exist:
QThread *m_thread = nullptr; - Update
initService()to assignm_thread = th;as commented in the implementation above.
- Add a member to track the thread if it does not already exist:
-
In the destructor
ServiceBase::~ServiceBase()(or wherever the service is unregistered, e.g.unregisterService()):- Ensure the thread is asked to stop and waited on before destruction:
if (m_thread) { m_thread->quit(); m_thread->wait(); // m_thread will delete itself due to finished -> deleteLater connection m_thread = nullptr; }
- Make sure this code runs before the
ServiceBaseobject is destroyed or before the application exits.
-
If multiple services share a thread or if the application has a central thread manager, adapt the ownership and shutdown logic accordingly so that the thread is not deleted while other objects still use it.
| } | ||
|
|
||
| // if it is not a local message, hook exec at main thread | ||
| void QTDBusSpyHook(const QDBusMessage &msg) |
There was a problem hiding this comment.
issue (complexity): Consider extracting the shared DBus handling logic and simplifying getServiceObject’s interface to eliminate duplication and unused parameters while keeping current behavior intact.
You can significantly reduce complexity here without changing behavior by:
1. Deduplicating QTDBusSpyHook and QTDBusHook
Almost all of this code is copy‑pasted between the two hooks. You can centralize the common logic into a small helper and keep the per‑hook differences explicit.
One approach is to extract:
- Common: object lookup, registration, timer management, policy checks.
- Variant: how denial is signaled (send reply via which connection, return code, the special
connectToBusin the spy hook).
For example:
enum class HookResult {
Allow,
DenyWithReply, // reply already sent
NoServiceObject // object not found or unusable
};
static HookResult handleDbusMessage(ServiceBase *serviceObj,
const QDBusMessage &msg,
const QString &realPath,
bool useWeirdSpyDenyBehavior)
{
if (!serviceObj->isRegister()) {
qCInfo(dsm_hook_qt) << "--to register dbus object: " << msg.path();
serviceObj->registerService();
}
if (!serviceObj->policy->isResident() && !serviceObj->isLockTimer()) {
qCInfo(dsm_hook_qt) << QString("--service: %1 will unregister in %2 minutes!")
.arg(serviceObj->policy->name)
.arg(serviceObj->policy->idleTime);
QTimer::singleShot(0, serviceObj, SLOT(restartTimer()));
}
ServiceQtDBus *srv = qobject_cast<ServiceQtDBus *>(serviceObj);
if (!srv)
return HookResult::Allow;
// Introspect
if (msg.member() == "Introspect"
&& msg.interface() == "org.freedesktop.DBus.Introspectable") {
if (serviceObj->policy->checkPathHide(realPath)) {
qCDebug(dsm_hook_qt) << "--call Introspect" << msg.path() << " ,is hided!";
QList<QVariant> arguments;
arguments << "";
QDBusMessage reply = msg.createReply(arguments);
srv->qDbusConnection().send(reply);
return HookResult::DenyWithReply;
}
return HookResult::Allow;
}
// Properties.Set
if (msg.member() == "Set"
&& msg.interface() == "org.freedesktop.DBus.Properties") {
const QList<QVariant> &args = msg.arguments();
if (args.size() >= 2) {
if (!serviceObj->policy->checkPropertyPermission(
getCMD(serviceObj, msg.service()),
realPath,
args.at(0).toString(),
args.at(1).toString())) {
QDBusMessage reply = msg.createErrorReply(
"com.deepin.service.Permission.Deny", "The call is deny");
srv->qDbusConnection().send(reply);
return HookResult::DenyWithReply;
}
}
return HookResult::Allow;
}
// Normal methods
if (msg.interface() != "org.freedesktop.DBus.Properties"
&& msg.interface() != "org.freedesktop.DBus.Introspectable"
&& msg.interface() != "org.freedesktop.DBus.Peer")) {
if (!serviceObj->policy->checkMethodPermission(
getCMD(serviceObj, msg.service()),
realPath,
msg.interface(),
msg.member())) {
QDBusMessage reply = msg.createErrorReply(
"com.deepin.service.Permission.Deny", "The call is deny2");
if (useWeirdSpyDenyBehavior) {
// preserves current spy behavior
QDBusConnection::connectToBus(QDBusConnection::SessionBus,
QStringLiteral("org.dsdsf.dsfsdf"))
.send(reply);
} else {
srv->qDbusConnection().send(reply);
}
return HookResult::DenyWithReply;
}
}
return HookResult::Allow;
}Then each hook becomes much smaller and easier to reason about:
void QTDBusSpyHook(const QDBusMessage &msg)
{
ServiceBase *serviceObj = nullptr;
bool isSubPath;
QString realPath;
if (!QTDbusHook::instance()->getServiceObject(
QString(), msg.path(), &serviceObj, isSubPath, realPath)) {
qCWarning(dsm_hook_qt) << "--can not find hook object: " << msg.path();
return;
}
const auto result = handleDbusMessage(serviceObj, msg, realPath,
/*useWeirdSpyDenyBehavior=*/true);
Q_UNUSED(result); // spy hook does not need to return anything
}int QTDBusHook(const QString &baseService, const QDBusMessage &msg)
{
Q_UNUSED(baseService);
ServiceBase *serviceObj = nullptr;
bool isSubPath;
QString realPath;
if (!QTDbusHook::instance()->getServiceObject(
QString(), msg.path(), &serviceObj, isSubPath, realPath)) {
qCWarning(dsm_hook_qt) << "--can not find hook object:" << msg.path();
return 0;
}
const auto result = handleDbusMessage(serviceObj, msg, realPath,
/*useWeirdSpyDenyBehavior=*/false);
if (result == HookResult::DenyWithReply)
return -1;
return 0;
}This keeps all current behaviors (including the unusual connectToBus path) but removes the large duplicated if‑else trees.
2. Clarifying / removing isSubPath
getServiceObject writes isSubPath, but in this file both call sites ignore it:
bool isSubPath;
QString realPath;
QTDbusHook::instance()->getServiceObject("", msg.path(), &serviceObj, isSubPath, realPath);To reduce mental overhead:
- If
isSubPathis not used anywhere else, drop it from the signature and implementation:
bool QTDbusHook::getServiceObject(QString name,
QString path,
ServiceBase **service,
QString &realPath)
{
Q_UNUSED(name);
auto iterService = m_serviceMap.find(path);
if (iterService != m_serviceMap.end()) {
*service = iterService.value();
realPath = iterService.key();
return true;
}
for (auto iter = m_serviceMap.begin(); iter != m_serviceMap.end(); ++iter) {
if (path.startsWith(iter.key()) && iter.value()->policy->allowSubPath(iter.key())) {
*service = iter.value();
realPath = iter.key();
return true;
}
}
return false;
}- And adapt call sites accordingly:
ServiceBase *serviceObj = nullptr;
QString realPath;
bool findRet = QTDbusHook::instance()->getServiceObject(
QString(), msg.path(), &serviceObj, realPath);- If
isSubPathis needed by other users ofgetServiceObject, consider either:- using it in these hooks as well (and naming it clearly, e.g.
matchedExactPath), or - splitting into two overloads: one simple version for hooks, one detailed version for callers that care about the distinction.
- using it in these hooks as well (and naming it clearly, e.g.
3. Normalizing denial reply behavior
Right now, denial replies are constructed in multiple places with slightly different strings and sending mechanisms (including commented‑out code and the debug‑looking org.dsdsf.dsfsdf connection).
The shared helper above already centralizes:
- the error names,
- the messages,
- which connection is used.
If the odd connectToBus is intentional, keeping it behind a useWeirdSpyDenyBehavior flag makes that intent explicit and localizes the “special case,” instead of inlining it in a big branch. If it’s not intentional, this helper makes it very easy to swap it out in one place later without touching both hooks.
caixr23
approved these changes
Jan 29, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, ut003640 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 |
Contributor
Author
|
/forcemerge |
Contributor
|
This pr force merged! (status: unstable) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
add the debug for service-manager-plugin-network
Log: add debug for service-manager
Summary by Sourcery
Introduce a simplified Qt6-based example application that can either display a basic DCC plugin test widget or run a service-manager-style DBus service with policy-based access control, replacing the previous dcc-network plugin example and wiring it into the build.
New Features:
Enhancements:
Build: