refactor: Service safety rectification #548
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
- Security:
- Updated _FORTIFY_SOURCE to version 2 in CMakeLists.txt files for enhanced buffer overflow detection.
- Added User=root to deepin-devicecontrol.service for appropriate privilege execution.
- Refactored D-Bus authorization checks in controlinterface.cpp and main.cpp to use SystemBusNameSubject for more robust service-level authentication.
- Integrated authorization checks into deviceinterface.cpp methods (getInfo, refreshInfo, setMonitorDeviceFlag) and controlinterface.cpp methods to ensure privileged operations are properly authorized.
- Dependency Management:
- Updated debian/control to change the package section from devel to utils.
- Added new dependencies (usbutils, pciutils, smartmontools, bluez, apt, lastore-daemon, kmod, iputils-ping, net-tools) to debian/control to support new functionalities.
- Build System & Code Refactoring:
- Modified DriverInstallerApt::executeCommand to accept QStringList arguments, improving command execution flexibility.
- Replaced temporary path handling in DriverBackupThread.cpp with QTemporaryDir for safer and more reliable temporary directory management.
- Removed deprecated D-Bus service files (com.deepin.Devicemanager.service, com.deepin.Devicemanager.xml) and related build configurations.
- Updated .gitignore and rpm/deepin-devicemanager.spec to reflect build artifact changes and D-Bus service removal.
deepin pr auto review我来对这次代码变更进行审查：
总体来说，这次变更主要提升了系统的安全性和代码质量，是一个积极的改进。建议在后续版本中考虑上述改进建议。 |
Reviewer's GuideThis PR hardens service safety by upgrading compile-time fortify settings, refactoring D-Bus authorization to use SystemBusNameSubject, injecting authorization guards into control and device interfaces, enhancing command execution flexibility and temporary path management, and cleaning up deprecated service artifacts and packaging dependencies. Sequence diagram for refactored D-Bus authorization using SystemBusNameSubjectsequenceDiagram
participant Client
participant "D-Bus Service"
participant Authority
Client->>"D-Bus Service": Request privileged operation
"D-Bus Service"->>Authority: checkAuthorizationSync("com.deepin.deepin-devicemanager.checkAuthentication" SystemBusNameSubject(service()) AllowUserInteraction)
Authority-->>"D-Bus Service": Authorization result
alt Authorized
"D-Bus Service"-->>Client: Perform operation
else Not authorized
"D-Bus Service"-->>Client: Deny operation
end
Entity relationship diagram for updated debian/control dependencieserDiagram
PACKAGE ||--o{ DEPENDENCY : includes
PACKAGE {
string name
string section
}
DEPENDENCY {
string name
}
PACKAGE ||--o{ usbutils : "usbutils"
PACKAGE ||--o{ pciutils : "pciutils"
PACKAGE ||--o{ smartmontools : "smartmontools"
PACKAGE ||--o{ bluez : "bluez"
PACKAGE ||--o{ apt : "apt"
PACKAGE ||--o{ lastore-daemon : "lastore-daemon"
PACKAGE ||--o{ kmod : "kmod"
PACKAGE ||--o{ iputils-ping : "iputils-ping"
PACKAGE ||--o{ net-tools : "net-tools"
Class diagram for updated DriverInstallerApt command executionclassDiagram
class DriverInstallerApt {
+void aptClean()
+QString executeCommand(const QString &cmd, const QStringList &args)
+void doOperate(const QString &package, const QString &version)
-QProcess *m_process
}
DriverInstallerApt : executeCommand(const QString &cmd, const QStringList &args)
Class diagram for authorization guards in DeviceInterface and ControlInterfaceclassDiagram
class DeviceInterface {
+QString getInfo(const QString &key)
+void refreshInfo()
+void setMonitorDeviceFlag(bool flag)
-bool getUserAuthorPasswd()
}
class ControlInterface {
+QString getRemoveInfo()
+QString getAuthorizedInfo()
+bool isDeviceEnabled(const QString &unique_id)
+int isNetworkWakeup(const QString &logicalName)
+bool monitorWorkingDBFlag()
+QStringList checkModuleInUsed(const QString &modulename)
+bool isBlackListed(const QString &modName)
+bool isArchMatched(const QString &filePath)
+bool isDebValid(const QString &filePath)
-bool getUserAuthorPasswd()
}
DeviceInterface : getUserAuthorPasswd()
ControlInterface : getUserAuthorPasswd()
Class diagram for updated DriverBackupThread temporary directory managementclassDiagram
class DriverBackupThread {
+void run()
}
class QTemporaryDir {
+QString path()
}
DriverBackupThread --> QTemporaryDir : uses
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 there - I've reviewed your changes - here's some feedback:
- The repeated getUserAuthorPasswd checks could be extracted into a common guard or macro to reduce duplication and ensure consistency across methods.
- In DriverInstallerApt, consider checking QProcess exit status and capturing stderr to properly detect and handle command failures instead of only reading stdout.
- Review the QTemporaryDir usage in DriverBackupThread—its template argument, scope, and lifetime—to ensure the temp directory isn’t removed prematurely and is created safely.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated getUserAuthorPasswd checks could be extracted into a common guard or macro to reduce duplication and ensure consistency across methods.
- In DriverInstallerApt, consider checking QProcess exit status and capturing stderr to properly detect and handle command failures instead of only reading stdout.
- Review the QTemporaryDir usage in DriverBackupThread—its template argument, scope, and lifetime—to ensure the temp directory isn’t removed prematurely and is created safely.
## Individual Comments
### Comment 1
<location> `deepin-devicemanager-server/deepin-devicecontrol/src/drivercontrol/driverinstallerapt.cpp:84` </location>
<code_context>
+ qCDebug(appLog) << "Executing command:" << cmd << args;
QProcess process;
- process.start("bash", QStringList() << "-c" << cmd);
+ process.start(cmd, args);
process.waitForFinished();
return QString::fromUtf8(process.readAllStandardOutput());
</code_context>
<issue_to_address>
**suggestion:** Directly starting commands without a shell may affect environment and command parsing.
Confirm that all commands and arguments used are compatible with direct invocation, as shell-specific features will no longer be available.
Suggested implementation:
```cpp
/*
* Note: This function now directly invokes the command without a shell.
* Ensure that 'cmd' and 'args' do not use shell-specific features (pipes, redirection, globbing, etc).
* If shell features are needed, use: process.start("bash", QStringList() << "-c" << fullCommandString);
*/
QString DriverInstallerApt::executeCommand(const QString &cmd, const QStringList &args)
{
qCDebug(appLog) << "Executing command:" << cmd << args;
// Check for shell-specific features in cmd or args before invoking
// (This is a manual process: audit all calls to executeCommand in this file and others)
QProcess process;
process.start(cmd, args);
process.waitForFinished();
return QString::fromUtf8(process.readAllStandardOutput());
}
```
You must audit all calls to `executeCommand` throughout the codebase. If any call passes a command or arguments that use shell features (e.g., `"ls | grep foo"`, `"echo $PATH"`, `"*.deb"`), you must refactor those calls to either:
- Avoid shell features and use only direct command/argument invocation, or
- Use the shell explicitly: `process.start("bash", QStringList() << "-c" << fullCommandString);`
For example, if you previously had:
`executeCommand("bash", QStringList() << "-c" << "ls | grep foo");`
You should keep this as-is if shell features are required.
If you had:
`executeCommand("apt", QStringList() << "update");`
This is safe, as it does not use shell features.
Document this requirement for future maintainers.
</issue_to_address>
### Comment 2
<location> `deepin-devicemanager-server/deepin-deviceinfo/src/loadinfo/deviceinterface.cpp:47-50` </location>
<code_context>
qCDebug(appLog) << "Getting info for key:" << key;
+
+ // 获取设备信息需要身份验证
+ if (!getUserAuthorPasswd()) {
+ qCWarning(appLog) << "Authorization failed for getInfo operation";
+ return "0";
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Returning "0" on authorization failure may be ambiguous.
Use an empty string or a distinct error value to clearly indicate authorization failure, avoiding confusion with valid '0' results.
```suggestion
if (!getUserAuthorPasswd()) {
qCWarning(appLog) << "Authorization failed for getInfo operation";
return "";
}
```
</issue_to_address>
### Comment 3
<location> `deepin-devicemanager/src/DriverControl/DriverBackupThread.cpp:38-39` </location>
<code_context>
}
- QString backupPath = QString("%1/driver/%2").arg(DB_PATH_TMP).arg(debname);
+ QTemporaryDir tempDir(QDir::tempPath() + "/XXXXXX");
+ QString backupPath = QString("%1/driver/%2").arg(tempDir.path()).arg(debname);
QDir destdir(backupPath);
if (!destdir.exists()) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Using QTemporaryDir may result in backup data being deleted prematurely.
If backupPath is required after the thread completes, QTemporaryDir will remove the directory, risking data loss. Use a persistent directory or ensure QTemporaryDir's lifetime covers all necessary operations.
</issue_to_address>
### Comment 4
<location> `deepin-devicemanager/src/main.cpp:108` </location>
<code_context>
qCDebug(appLog) << "PageDriverControl::installDriverLogical at name page";
- // 驱动安装之前需要先提权
- Authority::Result result = Authority::instance()->checkAuthorizationSync("com.deepin.deepin-devicemanager.checkAuthentication",
- UnixProcessSubject(getpid()),
- Authority::AllowUserInteraction);
- if (result != Authority::Yes) {
</code_context>
<issue_to_address>
**🚨 question (security):** Changing authorization subject may affect which user is authorized.
Confirm that using SystemBusNameSubject aligns with your security requirements, particularly in multi-user or service scenarios.
</issue_to_address>
### Comment 5
<location> `deepin-devicemanager-server/deepin-devicecontrol/src/controlinterface.cpp:130` </location>
<code_context>
QString ControlInterface::getRemoveInfo()
{
+ if (!getUserAuthorPasswd())
+ return {};
return EnableSqlManager::getInstance()->removedInfo();
</code_context>
<issue_to_address>
**issue (complexity):** Consider using macros or inline helpers to centralize the authorization guard logic and reduce repetitive code in each method.
Consider centralizing the guard into a couple of very small macros (or inline helpers) so each method only needs one line, e.g.:
```cpp
// In ControlInterface.cpp, near the top:
#define AUTH_GUARD_RET(val) \
do { if (!getUserAuthorPasswd()) return (val); } while(0)
#define AUTH_GUARD() AUTH_GUARD_RET(/*void*/)
```
Then each method becomes:
```cpp
QString ControlInterface::getRemoveInfo()
{
AUTH_GUARD_RET({}); // was: if (!getUserAuthorPasswd()) return {};
return EnableSqlManager::getInstance()->removedInfo();
}
void ControlInterface::disableOutDevice(const QString &devInfo)
{
AUTH_GUARD(); // was: if (!getUserAuthorPasswd()) return;
EnableUtils::disableOutDevice(devInfo);
}
bool ControlInterface::enable(const QString &hclass, …)
{
AUTH_GUARD_RET(false); // was: if (!getUserAuthorPasswd()) return {};
…
}
```
That removes the repetitive boilerplate while preserving exactly the same behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
lzwind
approved these changes
Nov 10, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, wangrong1069 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
|
/merge |
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.
Security:
Dependency Management:
Build System & Code Refactoring:
Summary by Sourcery
Strengthen service security and refactor build and dependency management across the project
Enhancements:
Build:
Deployment:
Chores: