已合并
fix(midi-ble): 安全告警修复 #87
fix(midi-ble): 安全告警修复 #87
已合并
acorwa创建于 8月3日
acorwa
acorwa成员
8月3日

一、内容说明(相关的Issue)

https://gitcode.com/openharmony/multimedia_midi_framework/issues/71

二、建议测试周期和提测地址

建议测试完成时间:xxxx.xx.xx
投产上线时间:xxxx.xx.xx
提测地址:CI环境/压测环境
测试账号:

三、变更内容

  • 3.1 关联PR列表

  • 3.2 数据库和部署说明

    1. 常规更新
    2. 重启unicorn
    3. 重启sidekiq
    4. 迁移任务:是否有迁移任务,没有写 "无"
    5. rake脚本:bundle exec xxx RAILS_ENV = production;没有写 "无"
  • 3.4 其他技术优化内容(做了什么,变更了什么)

    • 重构了 xxxx 代码
    • xxxx 算法优化
  • 3.5 废弃通知(什么字段、方法弃用?)

  • 3.6 后向不兼容变更(是否有无法向后兼容的变更?)

四、研发自测点(自测哪些?冒烟用例全部自测?)

自测测试结论:

五、测试关注点(需要提醒QA重点关注的、可能会忽略的地方)

检查点:

需求名称 是否影响xx公共模块 是否需要xx功能 需求升级是否依赖其他子产品
xxx 否 需要 不需要

接口测试:

性能测试:

并发测试:

其他:

likedislike
Pull Request已成功合入, 合并人@openharmony_ci
(感谢 acorwa 的贡献)
openharmony_ciopenharmony_ci成员
8月3日 添加了label:waiting_on_author
openharmony_ci
openharmony_ci成员
8月3日 评论:

感谢提交 Pull Requests!如果您提交的PR已经开发完毕,请评论 "start build" 触发门禁,更多交互操作,请访问OpenHarmony社区支持命令清单。如果需要调整订阅PR、Issue的变更状态,请访问订阅链接。


Thanks for submitting the pull request. If your Pull Request has already been developed, you can leave a "start build" comment to trigger the gated system. For more commands, please visit OpenHarmony Command List. If you need to change the subscription of a Pull Request or Issue, please visit the link.

likedislike
openharmony_ciopenharmony_ci成员
8月3日 添加了label:dco检查成功
acorwaacorwa成员
8月3日 修改了pull request 的描述
Noc
Noc成员8月3日进行代码检视1
services/server/src/midi_service_controller.cpp
@@ -155,4 +155,3 @@
155155 MIDI_INFO_LOG("Unload timer cancelled.");
156156 }
157- isUnloadPending_.store(false, std::memory_order_release);
158157 });
Noc
Noc8月3日评论:

移除 isUnloadPending_ 复位实际上修复了一个潜在的 std::terminate(建议补注释)

此处删掉了定时器线程末尾的 isUnloadPending_.store(false, ...)。经核查,这一删除是正确且有益的:旧代码在定时器自然触发(shouldUnload=true 并调用 UnloadSystemAbility)后把 isUnloadPending_ 置回 false,但此时 unloadThread_ 仍处于 joinable 状态(线程已退出但未被 join)。此后若服务未被真正回收(卸载异步未完成、或有新客户端接入),再次调用 ScheduleUnloadTask 时 if (isUnloadPending_) 为 false 会跳过 join 分支,直接执行 unloadThread_ = std::thread(...)——对一个仍 joinable 的 std::thread 赋值会触发 std::terminate。CancelUnloadTask 的快速返回路径同样会漏 join。保留 isUnloadPending_=true 可让后续重排/取消都进入 join 分支,从而避免崩溃。

影响: 旧实现存在"自然触发后服务仍存活"场景下的崩溃风险;新实现修复了该问题。

建议修复: 删除本身没问题,但删除原因并不直观。建议在此处补一行注释,例如:// 不复位 isUnloadPending_:保留 joinable 线程的 join 路径,避免对 joinable std::thread 赋值触发 std::terminate,防止后续维护者"修复性"地把它加回来。

严重程度: 低(可维护性)

likedislike
Noc
Noc成员8月3日进行代码检视1
services/server/src/midi_device_mananger.cpp
@@ -487,2 +491,3 @@
487491 std::lock_guard<std::mutex> lock(driversMutex_);
488- drivers_[type] = std::move(driver);
492+ if (driver) {
493+ drivers_[type] = std::shared_ptr<MidiDeviceDriver>(driver.release());
Noc
Noc8月3日评论:

InjectDriverForTest 注入 BLE 驱动时未同步更新 g_instance 单例

构造函数中 BLE 驱动通过 RegisterInstance(bleDriver) 注册到静态 g_instance,所有 BLE 回调(OnConnectionState 等)经 AcquireInstance() 取到的都是 g_instance 指向的驱动。但 InjectDriverForTest 只替换了 drivers_[type],并未调用 RegisterInstance。若测试注入 DEVICE_TYPE_BLE 的 mock,则 drivers_[BLE] 指向 mock,而 g_instance 仍指向构造期创建的原 BLE 驱动——BLE 回调会落到原驱动而非 mock;同时原驱动因被 g_instance 持有而不会被释放,存在两份 BLE 驱动对象。

影响: BLE 驱动的测试替身收不到回调,测试隔离性被破坏;仅在注入 BLE 类型时出现。

建议修复: 若测试需替换 BLE 驱动,InjectDriverForTest 对 DEVICE_TYPE_BLE 应同步调用 BleMidiTransportDeviceDriver::RegisterInstance(...)/UnregisterInstance();若当前测试仅注入 USB,可加注释说明该限制。

严重程度: 低(测试可用性)

likedislike
Noc
Noc成员8月3日进行代码检视2
services/server/src/midi_server_dump.cpp
已过期
@@ -60,3 +60,2 @@
6060 } else {
61- dumpString += "Unknown parameter: ";
62- dumpString += std::wstring_convert<std::codecvt_utf8_utf16<char16_t>, char16_t>{}.to_bytes(para);
61+ dumpString += "Unknown parameter";
Noc
Noc8月3日评论:

dump 不再回显未知参数值,调试信息有损

去掉 std::wstring_convert(C++17 deprecated、C++20 移除)是对的,但新实现只输出固定串 "Unknown parameter",丢失了具体参数名,排查 hidumper 误输入时少了关键线索。

建议修复: 改用循环逐字符转换 u16string 再追加(dump 参数基本为 ASCII,简单处理即可):

std::string paraUtf8;
for (char16_t c : para) {
    if (c < 0x80) { paraUtf8 += static_cast<char>(c); }
}
dumpString += "Unknown parameter: " + paraUtf8;

严重程度: 低(调试体验)

likedislike
System
系统消息系统
8月4日 评论:

changed this line on 7a3bfd6c view diff detail

Noc
Noc成员8月3日进行代码检视1
frameworks/native/midi/src/midi_client.cpp
@@ -32,6 +32,9 @@ namespace MIDI {
3232namespace {
3333 constexpr uint32_t MAX_EVENTS_NUMS = 1000;
3434 constexpr uint32_t PORT_GROUP_RANGE = 16;
35+ // Standard maximum length of a MIDI SysEx message
36+ // (determined by the MIDI protocol and business requirements).
37+ constexpr uint32_t MAX_SYSEX_BYTE_SIZE = 65535;
3538} // namespace
3639class MidiClientCallback : public MidiCallbackStub {
3740public:
@@ -610,6 +613,8 @@ int32_t MidiOutputPort::SendSysEx(uint32_t portIndex, const uint8_t *data, uint3
610613{
611614 CHECK_AND_RETURN_RET_LOG(data, OH_MIDI_STATUS_GENERIC_INVALID_ARGUMENT, "parameter is nullptr");
612615 CHECK_AND_RETURN_RET_LOG(byteSize > 0, OH_MIDI_STATUS_GENERIC_INVALID_ARGUMENT, "byteSize is invalid");
616+ CHECK_AND_RETURN_RET_LOG(byteSize <= MAX_SYSEX_BYTE_SIZE, OH_MIDI_STATUS_GENERIC_INVALID_ARGUMENT,
Noc
Noc8月3日评论:

MAX_SYSEX_BYTE_SIZE 与环形缓冲容量量级不一致

加上限做合理性校验是好的。但 MAX_SYSEX_BYTE_SIZE=65535 远大于环形缓冲容量 MAX_MMAP_BUFFER_SIZE - sizeof(ControlHeader)(约 8128 字节)。因此 8129–65535 之间的 SysEx 会通过此校验,却在后续 ringBuffer_ 写入阶段才失败,错误诊断路径割裂,也给调用方一种"在该范围内即合法"的错觉。

建议修复: 二选一:(a) 将上限收紧到与环形缓冲容量匹配的量级;或 (b) 注释说明该值仅为粗粒度防滥用的合理性上界,真正容量约束由 ring 写入路径强制,避免误读。

严重程度: 低(可维护性)

likedislike
Noc
Noc成员
8月3日 评论:

代码审查报告: fix(midi-ble): 安全告警修复

摘要

  • 仓库: openharmony/multimedia_midi_framework
  • PR: #87
  • 作者: acorwa
  • 状态: Open

总体评价

✅ 批准(附带 4 条低优先级建议)

这是一组高质量的 安全/健壮性 修复,核心改动经逐项核查均正确:

  • 共享环 Unmarshalling/Init 增加 ringSize/capacity_ 上界校验,堵住了 uint32_t 溢出绕过 MAX_MMAP_BUFFER_SIZE 导致的 OOB 读写(防御深度合理,并补了单测)。
  • UmpProcessor::ProcessUmp default 分支改为按 GetUmpWordCount 跳过整包,修复了多字包丢失同步的流解析 bug。
  • BLE 驱动单例由裸原子指针改为 shared_ptr 强引用,回调持强引用闭合了 load()->lock_ TOCTOU 窗口;析构先 UnregisterInstance 再清理 GATT 客户端,时序正确。
  • CreateNewInputPortConnection/CreateNewOutputPortConnection 失败路径补 CloseInputPort/CloseOutputPort,修复设备端口资源泄漏(已确认不会与连接对象析构形成双重关闭)。
  • hasBluetoothPermission_ 改 std::atomic<bool>,闭合 IPC 线程读写竞态(已验证 if (atomic_bool) 可正常编译)。
  • ScheduleUnloadTask 末尾删除 isUnloadPending_ 复位,实际修复了"自然触发后对 joinable std::thread 赋值触发 std::terminate"的潜在崩溃。

影响面分析

  • 受影响调用方/依赖方: midi_service_client.cpp、midi_client_private.h、midi_service_interface.h 中的 Open/Close/Get* 接口签名未变,本 PR 改动(端口失败清理、权限原子化、BLE 单例重写)不破坏其调用前提。
  • 受影响测试: 无;本 PR 新增 midi_shared_ring_unit_test.cpp 2 个溢出用例,与实现一致。
  • 测试覆盖缺口: BLE 单例/回调 TOCTOU、UMP default 多字包、端口失败清理路径均无单测覆盖,建议后续补齐(非阻断)。
  • 风险评分: 0

业务知识校验

  • KB 未针对 midi 仓注入(biz 文件未生成),跳过。

发现的问题

无 严重/主要 问题。4 条 次要 建议已作为行内评论发布:

  1. midi_service_controller.cpp — 删除 isUnloadPending_ 复位处建议补注释
  2. midi_device_mananger.cpp — InjectDriverForTest 注入 BLE 未同步单例
  3. midi_server_dump.cpp — 未知参数不再回显
  4. midi_client.cpp — MAX_SYSEX_BYTE_SIZE 与 ring 容量量级不一致

代码质量亮点

  • 防御深度到位:在不可信 parcel 输入处先校验再取 fd,避免 fd 泄漏。
  • 注释解释了"为什么"(溢出、TOCTOU、资源泄漏),便于维护。
  • BLE 析构"在锁外释放、回调持强引用"的设计经得起推敲。
  • 配套单测覆盖了溢出修复。

改进建议

  • 补 BLE 回调时序与端口失败路径的集成/单测。
  • 4 条次要建议见行内评论。

审查者: Noc (Committer代码审查)
审查日期: 2026-08-03

likedislike
NocNoc成员
8月3日 通过审查
openharmony_ci
openharmony_ci成员
8月3日 评论:

ISSUE关联关系有变化,之前验证结果无效,需要重新触发构建

likedislike
acorwa
acorwa成员
8月4日 评论:

感谢 @gregoire 的细致审查与认可。4 条次要建议均已处理(改动在本地,将随下一次提交合入),逐项说明如下:

1. midi_service_controller.cpp — 删除 isUnloadPending_ 复位
采纳。已在定时器线程末尾补注释,说明刻意不复位的原因:定时器自然触发(shouldUnload)后线程虽已退出但仍 joinable,保留 isUnloadPending_=true 可让后续 ScheduleUnloadTask / CancelUnloadTask 进入 join 分支,避免对 joinable 的 std::thread 赋值触发 std::terminate。

2. midi_device_mananger.cpp — InjectDriverForTest 注入 BLE 未同步 g_instance
经核查现象属实,但"同步调用 RegisterInstance"在当前测试架构下无法落地:g_instance 类型为 shared_ptr<BleMidiTransportDeviceDriver>,而 fuzzer / 单测注入的 MockMidiDeviceDriver 继承自基类 MidiDeviceDriver、并非 BleMidiTransportDeviceDriver 子类,类型不兼容、无法注册。已改用注释方案,明确该限制的根因与影响边界——经 deviceManager 调度的 BLE 驱动方法(OpenInputPort / SendData 等)可被 mock 覆盖;BLE 协议栈主动回调路径不在覆盖范围内,测试通过 g_mockBleDriver 等 raw 指针驱动替身。若后续需覆盖 BLE 回调路径,需改为注入 BleMidiTransportDeviceDriver 派生替身,届时再行评估。

3. midi_server_dump.cpp — 未知参数不再回显
采纳。已改为逐字符转换 u16string(dump 参数基本为 ASCII)后回显具体参数值,便于排查 hidumper 误输入;并清理了不再使用的 <codecvt> / <locale> 头文件。

4. midi_client.cpp — MAX_SYSEX_BYTE_SIZE 与 ring 容量量级不一致
采纳备选方案。保留 65535(MIDI 协议 SysEx 长度量级)不变,已补注释说明该值仅为粗粒度防滥用上界,真正容量由共享环写入路径强制(MAX_MMAP_BUFFER_SIZE - sizeof(ControlHeader) ≈ 8128 字节),(8128, 65535] 区间的 SysEx 会在此处通过、却在 ring 写入阶段才失败。未直接收紧上限,以避免对合法大包的行为变化。

likedislike
acorwaacorwa成员
8月4日 审查状态已重置,审查人: gregoire
acorwaacorwa成员
8月4日 推送  1 个提交:7a3bfd6c-bugifx
openharmony_ci
openharmony_ci成员
8月4日 评论:

感谢提交 Pull Requests!如果您提交的PR已经开发完毕,请评论 "start build" 触发门禁,更多交互操作,请访问OpenHarmony社区支持命令清单。如果需要调整订阅PR、Issue的变更状态,请访问订阅链接。


Thanks for submitting the pull request. If your Pull Request has already been developed, you can leave a "start build" comment to trigger the gated system. For more commands, please visit OpenHarmony Command List. If you need to change the subscription of a Pull Request or Issue, please visit the link.

likedislike
acorwaacorwa成员
8月4日 推送  1 个提交:9eb4760b-bugifx
openharmony_ci
openharmony_ci成员
8月4日 评论:

感谢提交 Pull Requests!如果您提交的PR已经开发完毕,请评论 "start build" 触发门禁,更多交互操作,请访问OpenHarmony社区支持命令清单。如果需要调整订阅PR、Issue的变更状态,请访问订阅链接。


Thanks for submitting the pull request. If your Pull Request has already been developed, you can leave a "start build" comment to trigger the gated system. For more commands, please visit OpenHarmony Command List. If you need to change the subscription of a Pull Request or Issue, please visit the link.

likedislike
acorwa
acorwa成员
8月4日 评论:

start build

likedislike
openharmony_ci
openharmony_ci成员
8月4日 评论:

首次触发
门禁构建开始,包含静态检查、代码编译和测试【master_inner_build编译, ohos-host_mini_tdd编译, dayu600_7885测试, dayu200编译, dayu600_7885编译, part_compile编译, dayu200测试, dayu200_tdd编译, x86_64_virt编译】,预计在60分钟内完成,门禁结果会同步发送到注册邮箱。您可以通过如下链接跟踪门禁进展:http://dcp.openharmony.cn/workbench/cicd/detail/6a7183ed64650f998b349a3e/runlist

likedislike
openharmony_ciopenharmony_ci成员
8月4日 添加了label:静态检查失败
openharmony_ci
openharmony_ci成员
8月4日 评论:

代码门禁未通过
您可以通过如下链接查看门禁报告:http://dcp.openharmony.cn/workbench/cicd/detail/6a7183ed64650f998b349a3e/runlist

静态检查:

# check type result report
1 codeCheck noPass >>>

编译测试:
# Device build result package
1 dayu200 pending NA
2 dayu200_tdd pending NA
3 part_compile pending NA
4 master_inner_build pending NA
5 ohos-host_mini_tdd pending NA
6 dayu600_7885 pending NA
7 x86_64_virt pending NA

likedislike
acorwaacorwa成员
8月4日 推送  1 个提交:6dce4f22-bugifx
acorwa
acorwa成员
8月4日 评论:

start build

likedislike
openharmony_ciopenharmony_ci成员
8月4日 删除了label:静态检查失败
openharmony_ci
openharmony_ci成员
8月4日 评论:

代码有更新,重置PR验证状态

likedislike
openharmony_ci
openharmony_ci成员
8月4日 评论:

本地或库上代码有更新,全量重新构建,重置所有关联PR的验证状态
门禁构建开始,包含静态检查、代码编译和测试【dayu200测试, master_inner_build编译, x86_64_virt编译, dayu200_tdd编译, part_compile编译, ohos-host_mini_tdd编译, dayu600_7885编译, dayu200编译】,预计在60分钟内完成,门禁结果会同步发送到注册邮箱。您可以通过如下链接跟踪门禁进展:http://dcp.openharmony.cn/workbench/cicd/detail/6a718f7a64650f998b3a2092/runlist

likedislike
openharmony_ci
openharmony_ci成员
8月4日 评论:

感谢提交 Pull Requests!如果您提交的PR已经开发完毕,请评论 "start build" 触发门禁,更多交互操作,请访问OpenHarmony社区支持命令清单。如果需要调整订阅PR、Issue的变更状态,请访问订阅链接。


Thanks for submitting the pull request. If your Pull Request has already been developed, you can leave a "start build" comment to trigger the gated system. For more commands, please visit OpenHarmony Command List. If you need to change the subscription of a Pull Request or Issue, please visit the link.

likedislike
NocNoc成员
8月4日 通过审查
openharmony_ciopenharmony_ci成员
8月4日 添加了label:编译成功
openharmony_ciopenharmony_ci成员
8月4日 添加了label:静态检查成功
openharmony_ciopenharmony_ci成员
8月4日 添加了label:冒烟测试成功
openharmony_ciopenharmony_ci成员
8月4日 通过测试
openharmony_ci
openharmony_ci成员
8月4日 评论:

代码门禁通过
您可以通过如下链接查看门禁报告:http://dcp.openharmony.cn/workbench/cicd/detail/6a718f7a64650f998b3a2092/runlist

静态检查:

# check type result report
1 codeCheck pass >>>

编译测试:
# Device build result test result package
1 dayu200 success success >>>
2 dayu200_tdd success NA >>>
3 part_compile success(IGNORE) NA >>>
4 master_inner_build success NA >>>
5 ohos-host_mini_tdd success NA >>>
6 dayu600_7885 success NA >>>
7 x86_64_virt success NA >>>

likedislike
openharmony_ciopenharmony_ci成员
8月4日 删除了label:waiting_on_author
openharmony_ciopenharmony_ci成员
8月4日 添加了label:waiting_for_review
openharmony_ci
openharmony_ci成员
8月4日 评论:

您好,Committer @suruoyan @gregoire ,请分配检视人员检视该PR,可以通过命令"assign [@someone_id]"分配检视人员,也可以直接评论"assign"分配给自己进行检视。


Hello, Committer @suruoyan @gregoire . Please assign someone to review the PR. You can assign a reviewer by using the command "assign [@someone_id]", or you can comment "assign" to review the PR by yourself.

likedislike
openharmony_ciopenharmony_ci成员
8月4日 关闭了关联的issue
openharmony_ciopenharmony_ci成员
8月4日 合入了pull request,合并节点 SHA:7f63c58c665a40615b29fa9af1823632b4fb4a5b
openharmony_ciopenharmony_ci成员
8月4日 删除了label:waiting_for_review
openharmony_ciopenharmony_ci成员
8月4日 添加了label:merged