fix(accounts): guard userList against unavailable Accounts service - #148
fix(accounts): guard userList against unavailable Accounts service#148zccrs wants to merge 1 commit into
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideHardens DAccountsManager::userList() against unavailable Accounts services and malformed or invalid D-Bus results, preventing startup SIGSEGV while preserving error reporting. Sequence diagram for guarded Accounts user listingsequenceDiagram
participant UserModel
participant DAccountsManager
participant SystemBus as QDBusConnection
participant Accounts as org.freedesktop.Accounts
UserModel->>DAccountsManager: userList()
DAccountsManager->>SystemBus: isServiceRegistered(org.freedesktop.Accounts)
alt service unavailable
SystemBus-->>DAccountsManager: false
DAccountsManager-->>UserModel: DUnexpected ServiceUnknown
else service available
SystemBus-->>DAccountsManager: true
DAccountsManager->>Accounts: listCachedUsers()
Accounts-->>DAccountsManager: QDBusPendingReply
DAccountsManager->>DAccountsManager: waitForFinished()
alt isError() or !isValid()
DAccountsManager-->>UserModel: DUnexpected D-Bus error
else valid reply
DAccountsManager->>DAccountsManager: value()
DAccountsManager-->>UserModel: QList<quint64>
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/accounts/daccountsmanager.cpp" line_range="53-54" />
<code_context>
DExpected<QList<quint64>> DAccountsManager::userList() const
{
Q_D(const DAccountsManager);
+ if (!QDBusConnection::systemBus().interface()->isServiceRegistered(
+ QStringLiteral("org.freedesktop.Accounts"))) {
+ return DUnexpected{ DCORE_NAMESPACE::emplace_tag::USE_EMPLACE,
+ static_cast<int>(QDBusError::ServiceUnknown),
</code_context>
<issue_to_address>
**issue (testing):** The new guard always checks for `org.freedesktop.Accounts` on the system bus, but unit-test builds define `USE_FAKE_INTERFACE`, causing `DAccountsInterface` to use `com.deepin.daemon.FakeAccounts` on the session bus. The fake service is therefore treated as unavailable, `userList()` returns before calling `listCachedUsers()`, and `tests/accounts/ut-daccountsmanager.cpp` fails because `m_userListTrigger` remains false.
**Triggers:** When the repository's unit tests are built with `USE_FAKE_INTERFACE`.
**Suggested fix:** Use the same service name and bus selected by `DAccountsInterface` under `USE_FAKE_INTERFACE`, or provide a test-compatible service-registration abstraction for this guard.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
已按 @sourcery-ai[bot] 建议修复： |
|
已按建议为 |
|
已细化空路径 |
|
已按反馈重构可读性：新增 |
|
先不合，现在dtk向dtk6同步的流程被禁用了，但又没像其它dtk组件一样在dtk5仓库同时维护dtk5和dtk6的打包，需要先处理这个问题， |
|
Follow-up:
The check is now a blocking |
DAccountsManager::userList() could SIGSEGV on a null QArrayDataPointer when ListCachedUsers demarshalling failed. Check service registration synchronously on the same bus/service as DAccountsInterface so USE_FAKE_INTERFACE still works, reject isError replies, and skip empty object paths. Do not use DDBusInterface::serviceValid(): it is filled by an async NameHasOwner callback, so callers such as Treeland's UserModel that invoke userList() before QCoreApplication::exec() always saw the service as missing. Log: fix accounts userList SIGSEGV when service unavailable
bb50a86 to
d09cf2d
Compare
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 优秀 ✅ 通过 潜在问题： 建议： 代码语法正确，逻辑清晰。isServiceRegistered() 方法正确使用 QDBusConnectionInterface 同步查询服务注册状态，null 检查完善（if (!iface) return false）。userList() 中服务可用性前置检查、isError() || !isValid() 组合检查、reply.value() 拷贝后遍历、空 path 跳过等逻辑均正确无误。 2. 代码质量 ✅评价： 优秀 ✅ 通过 潜在问题：
建议： 代码结构清晰，isServiceRegistered() 方法注释详细解释了为何不使用 DDBusInterface::serviceValid() 的异步回调而改用同步查询，上下文充分。qWarning 日志内容合理（记录无效条目的索引和总数），建议将日志中的 em dash（—）替换为 ASCII 连字符（-）以增强日志兼容性。 3. 代码性能 ✅评价： 优秀 ✅ 通过 潜在问题： 建议： 性能良好，资源使用合理。isServiceRegistered() 的同步 D-Bus NameHasOwner 查询开销极小（微秒级），且原代码已有 waitForFinished() 阻塞调用，整体仍为阻塞函数，无额外性能瓶颈。const auto value = reply.value() 的拷贝是必要的安全措施，避免 reply 对象在遍历期间被修改。 4. 代码安全 🔒评价： 优秀 ✅ 通过
安全漏洞详情： 建议： 存在0个安全漏洞，安全合规。本次修复实质上增强了安全性：1) 防止了 Accounts 服务不可用时的空指针解引用崩溃；2) 添加了对 D-Bus 返回值的防御性检查（空 path 跳过）；3) 错误信息中不包含敏感数据。D-Bus 调用面向系统服务，无用户输入注入风险。 💡 改进建议代码示例// 建议将 qWarning 中的 em dash 替换为 ASCII 连字符
// 修改前:
qWarning() << "DAccountsManager::userList: empty QDBusObjectPath at index" << i
<< "of" << value.size() << "skipped — accounts-daemon returned invalid entry";
// 修改后:
qWarning() << "DAccountsManager::userList: empty QDBusObjectPath at index" << i
<< "of" << value.size() << "skipped - accounts-daemon returned invalid entry";本报告由 AI 代码审查工具自动生成 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, zccrs 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 |
3 participants
Problem
DAccountsManager::userList()在org.freedesktop.Accounts不可用时直接解引用空QArrayDataPointer<QDBusObjectPath>（needsDetach(this=0x0)），导致SIGSEGV（exit 139）。gdb栈：在
treeland启动时必现：Helper::init无条件实例化UserModelQML 单例，dde-system-daemon（org.deepin.dde.Accounts1）因 Goconcurrent map竞态崩溃后，org.freedesktop.Accounts虽在但QDBusPendingReply::value()的解包路径在部分传输下返回QDBusArgument而非QList<QDBusObjectPath>，原有!isValid检查不足，遍历空指针列表触发崩溃。dtk6systemsettings会自动从本仓库同步，故修复提交至此。Fix
QDBusConnection::systemBus().interface()->isServiceRegistered("org.freedesktop.Accounts")，不可用时直接返回DUnexpected{ServiceUnknown}waitForFinished后同时检查isError() || !isValid()const auto value = reply.value()拷贝后遍历，跳过空path()已在
archlinux+accounts-daemon 788+treeland验证：原库必SIGSEGV，补丁库后正常启动。Log: fix accounts userList SIGSEGV when service unavailable
Summary by Sourcery
Guard account user enumeration against unavailable services and malformed D-Bus responses.
Bug Fixes:
Enhancements: