Skip to content

fix(config): prevent dconfig core dump caused by malformed config value format#333

Merged
zhaohuiw42 merged 1 commit intolinuxdeepin:masterfrom
zhaohuiw42:master
Mar 17, 2026
Merged

fix(config): prevent dconfig core dump caused by malformed config value format#333
zhaohuiw42 merged 1 commit intolinuxdeepin:masterfrom
zhaohuiw42:master

Conversation

@zhaohuiw42
Copy link
Contributor

No description provided.

@github-actions
Copy link

github-actions bot commented Mar 17, 2026

CLA Assistant Lite bot:
提交邮箱中包含我们的合作伙伴,但您似乎并非合作伙伴的成员或对接人,请联系相关对接人将您添加至组织之中,或由其重新发起 Pull Request。
The commit email domain belongs to one of our partners, but it seems you are not yet a member of the current organization, please contact the contact person to add you to the organization or let them submit the Pull Request.

You can retrigger this bot by commenting recheck in this Pull Request

@deepin-ci-robot
Copy link

deepin pr auto review

这段代码的修改是为了解决与 dde-dconfig 的兼容性问题,将原本直接保存 []int 类型的数据改为保存 []dbus.Variant 类型。以下是对这段 diff 的详细审查和改进建议:

1. 语法与逻辑审查

  • 代码逻辑正确性

    • 修改后的逻辑是将输入的 checkRange ([]int) 转换为 []dbus.Variant 切片,然后调用 c.save
    • 测试用例 TestSetStartCheckRangeSavesDBusVariants 使用了 mock 对象来验证 SetValue 方法是否被正确调用,并且传入的参数类型和值是否符合预期。
    • 逻辑上是自洽的,能够达到临时修复 bug 的目的。
  • 语法正确性

    • 代码符合 Go 语言规范。
    • 切片预分配 make([]dbus.Variant, 0, len(checkRange)) 是很好的实践,避免了多次 append 时的内存重新分配。

2. 代码质量

  • 可读性与注释
    • // TODO: remove this after bug of dde-dconfig fixed 注释非常关键。它明确指出了这是一个临时方案,并标记了移除的条件。建议在项目管理工具(如 Jira, GitHub Issues)中关联对应的 Bug ID,方便后续追踪和清理。
  • 测试覆盖
    • 新增的单元测试非常完善。它不仅验证了函数执行不报错,还通过 mock.MatchedBy 深入验证了传递给底层 DBus 接口的数据结构是否正确(即是否正确转换为了 []dbus.Variant 且内部值保持一致)。
  • 类型转换
    • dbus.MakeVariant(item) 的使用是标准的 DBus 绑定写法,没有问题。

3. 代码性能

  • 切片预分配
    • variants := make([]dbus.Variant, 0, len(checkRange)) 预先分配了容量,这是一个很好的性能优化点,避免了切片扩容带来的开销。
  • 循环开销
    • for 循环内的 append 操作是高效的。由于 checkRange 通常只包含少量整数(如版本号 [22, 21]),这里的性能开销可以忽略不计。

4. 代码安全

  • 潜在风险
    • Mock 与实现的匹配度:测试中 Mock 的 SetValue 签名是 (dbus.Flags, string, dbus.Variant)。请确保 Config.save 方法内部调用 dsLastoreManager.SetValue 时,第三个参数确实是包裹了切片的 dbus.Variant,而不是直接传递切片。根据代码 return c.save(dSettingsKeyStartCheckRange, variants),这里 variants[]dbus.Variantc.save 应该会将其包装进一个 dbus.Variant 中。测试代码中的 mock 验证逻辑 value.Value().([]dbus.Variant) 证实了这一点:外层是一个 Variant,里面存的是 []dbus.Variant。这一点逻辑是闭环的。
    • 数据一致性:函数内部 c.StartCheckRange = checkRange 保存的是原始的 []int,而持久化存储的是转换后的 []dbus.Variant。如果 Load 或读取逻辑没有相应修改(或者依赖 dde-dconfig 自动转换回来),可能会导致读取时的数据不一致。不过,既然这是为了修复 dde-dconfig 的 bug,假设读取路径是兼容的,或者读取路径也会相应处理,那么在当前 scope 下是安全的。

5. 改进意见

虽然代码本身已经写得不错,但可以从以下角度进行微调或增强:

  1. 辅助函数提取(可选)
    如果未来还有其他地方需要将 []int 转为 []dbus.Variant,可以将这段逻辑提取为一个辅助函数,例如:

    func toDBusVariants(ints []int) []dbus.Variant {
        variants := make([]dbus.Variant, 0, len(ints))
        for _, item := range ints {
            variants = append(variants, dbus.MakeVariant(item))
        }
        return variants
    }

    这样 SetStartCheckRange 会更简洁:

    func (c *Config) SetStartCheckRange(checkRange []int) error {
        c.StartCheckRange = checkRange
        // TODO: remove this after bug of dde-dconfig fixed
        return c.save(dSettingsKeyStartCheckRange, toDBusVariants(checkRange))
    }
  2. 空切片处理
    如果 checkRangenil 或空,当前代码会保存一个空的 []dbus.Variant。这是符合逻辑的,但建议确认 dde-dconfig 对空数组 Variant 的处理是否健壮,防止空值导致后端崩溃。

  3. 测试用例增强
    建议在测试用例中增加对边界情况的测试,例如:

    • 空切片:checkRange := []int{}
    • 单元素切片:checkRange := []int{22}
    • 这能确保类型转换逻辑在各种长度下都能正常工作。
  4. Mock 对象的生命周期
    在测试中 manager 是局部变量。如果 Config 结构体中有其他字段在 SetStartCheckRangesave 中被访问,可能会导致 nil pointer panic。当前代码看起来只依赖 dsLastoreManager,所以是安全的,但值得注意。

总结

这段代码是一个高质量的临时补丁。它针对特定的底层 bug 进行了适配,没有破坏现有的业务逻辑,并且配备了详尽的单元测试。代码性能良好,安全性在当前上下文中是有保障的。唯一的建议是关注后续的清理工作(TODO 事项)以及考虑提取重复的转换逻辑。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: qiuzhiqian, zhaohuiw42

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@zhaohuiw42 zhaohuiw42 merged commit ae29a91 into linuxdeepin:master Mar 17, 2026
11 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants