修复多次信号注册BUG-runtime-AtomGit

已合并

修复多次信号注册BUG #1338

jinyingqi创建于 3月31日从jinyingqi/runtime_900： 9.0.0 合入到 cann/runtime： 9.0.0

修复多次信号注册BUG #1338

已合并

jinyingqi创建于 3月31日

从jinyingqi/runtime_900 9.0.0合入到cann/runtime 9.0.0

jinyingqi成员

3月31日

描述

同步master PR到9.0.0分支，原始PR https://gitcode.com/cann/runtime/pull/1274
问题背景:
1、同一个进程对 SIGINT 信号只能注册一个信号处理函数
2、首次调用时，oldSigHandler 保存了原有的信号处理函数（通常是默认的退出函数）
3、未加保护时，第二次调用会将 oldSigHandler 覆盖为 newSigHandler
4、导致 Ctrl+C 后无法正常退出程序

修复方案:
1、在注册信号处理函数前，检查 oldSigHandler 是否已被设置
2、如果已设置，直接返回，避免重复注册

关联的Issue

msprof acl api方式支持优雅退出。

测试

msprof acl api方式采集，ctrl c退出，验证可以优雅退出。

文档更新

无

类型标签

Bug修复
新特性
性能优化
文档更新
其他，请描述：

Pull Request已成功合入, 合并人@CANN-robot

（感谢 jinyingqi 的贡献）

J jinyingqi成员

3月31日创建了 pull request，commit dac073b5

CANN-robot成员

3月31日评论：

Thanks for your pull-request.
The full list of commands accepted by me can be found at here。
You can get sig-info at here

PR Approval Progress

✅ Congratulations! All modules have met the lgtm and approve requirements.

Module Approval Details

module	lgtm status	approve status
src/dfx	✅ newstarzj, zhuliangying (2/2)	✅ zhuliangying (1/1)
tests/ut/msprof	✅ zhuliangying, newstarzj (2/2)	✅ zhuliangying (1/1)

💡 Tip:

Committer can comment /approve or /lgtm

Commenting /approve implies both code review (lgtm) and intent to merge (approve)

CLA Signature Pass

jinyingqi, thanks for your pull request. All authors of the commits have signed the CLA. 👍

CANN-robot成员

3月31日添加了label：cann-cla/yes

CANN-robot成员

3月31日将newstarzj,chenhao_1209,zhuliangying,wangtao43,zhangpengpeng8,yanmingxiang,Reyn52166,gcw_kUomxQ2l,Andy-lb,gcw_Dm2TsRUi,derekxu,tingwood设为评审人

CANN-robot成员

3月31日将newstarzj,chenhao_1209,zhuliangying设为审查人

jinyingqi成员

3月31日评论：

compile

CANN-robot成员

3月31日添加了label：ci-pipeline-running

CANN-robot成员

3月31日评论：

流水线任务触发成功
任务链接 [bbf92f3889ec47c78308525b8f9aa60b][流水线指导]

任务名称	状态	日志	下载链接
codecheck	✅ SUCCESS	>>>>>
SCA	✅ SUCCESS	>>>>>
anti_virus	✅ SUCCESS	>>>>>
Check_Pr	✅ SUCCESS	>>>>>
Compile_Ascend_X86	✅ SUCCESS	>>>>>	>>>>>
Compile_Ascend_ARM	✅ SUCCESS	>>>>>	>>>>>
pre_comment	✅ SUCCESS	>>>>>
UT_Test_acl	✅ SUCCESS		>>>>>
UT_Test_rts	✅ SUCCESS		>>>>>
UT_Test_rts_c	✅ SUCCESS		>>>>>
UT_Test_platform	✅ SUCCESS		>>>>>
UT_Test_qs	✅ SUCCESS		>>>>>
UT_Test_aicpusd	✅ SUCCESS		>>>>>
UT_Test_tsd	✅ SUCCESS		>>>>>
UT_Test_dfx	✅ SUCCESS		>>>>>
UT_Test_mmpa	✅ SUCCESS		>>>>>

[2026-03-31 12:54:37] CI执行结束

CANN-robot成员

3月31日删除了label：ci-pipeline-running

CANN-robot成员

3月31日添加了label：ci-pipeline-passed

CANN-robot成员

3月31日评论：

🤖 CANN 代码审查报告

PR: #1338 - 修复多次信号注册BUG
严重性: ✅ Low
审查时间: 2026-03-31 13:05

📊 审查结论

✅ 建议合入

严重性: Low
代码质量: 良好
内存安全: ✅ 无风险
安全性: ✅ 无漏洞
测试覆盖: 完整
文档完整性: NA

本 PR 修复了信号处理函数重复注册的问题，修改范围小、逻辑清晰，并附带了相应的单元测试，代码质量良好。

📋 修改概述

修复 RegisterSiganlHandler 函数重复注册信号处理函数导致 Ctrl+C 无法正常退出的问题。

修改文件: 2个 (+14行, -0行)
核心变更:
- prof_acl_mgr.cpp: 添加 oldSigHandler 非空检查，防止重复注册
- prof_acl_core_utest.cpp: 新增单元测试 RegisterSignalHandlerTwice

🔍 代码质量检查

1. 内存安全 ✅

内存泄漏: 无风险 - 无动态内存分配
指针操作: 安全 - 使用空指针检查
动态分配: NA - 无动态分配操作
资源管理: 安全 - 信号处理为系统资源

2. 安全性 ✅

输入验证: NA - 无外部输入
边界检查: NA - 无数组操作
潜在漏洞: 无 - 简单的条件检查逻辑

3. 可读性 ✅

代码清晰度: 良好 - 修改意图明确
命名规范: 符合 - 遵循现有代码风格
注释完整性: 部分 - PR 描述充分，代码行级注释可增强

4. 逻辑正确性 ✅

算法逻辑: 正确 - 条件检查逻辑清晰
边界条件: 处理完整 - 考虑了重复调用场景
影响范围: 明确 - 仅影响信号注册逻辑

💡 改进建议

线程安全考虑: 当前使用静态全局变量 oldSigHandler，如果 Init() 可能被多线程调用，建议添加互斥锁保护。但从代码上下文看，Init() 通常在主线程单次调用，风险较低。
函数命名: RegisterSiganlHandler 存在拼写错误（应为 Signal），建议在后续迭代中修正为 RegisterSignalHandler。
测试代码格式: 新增的测试代码中存在制表符（Tab）混用空格的情况，建议统一使用空格缩进，符合项目代码风格。
增强防御性: 可以考虑使用原子操作 std::atomic<ProfSignalHandler> 来确保多线程环境下的安全性。

✅ 代码亮点

最小化修改: 仅添加 1 行核心代码，修复精准
测试覆盖完整: 新增单元测试验证修复效果
问题定位清晰: PR 描述详细说明了问题背景和修复方案
向后兼容: 不影响现有功能，仅添加保护逻辑
CI 通过: 已通过 ci-pipeline-passed 和 CLA 检查

📝 修复逻辑分析

问题根因：
```
首次调用: oldSigHandler = signal(SIGINT, newSigHandler); // 保存原处理函数
二次调用: oldSigHandler = signal(SIGINT, newSigHandler); // 被覆盖为 newSigHandler
```
导致 newSigHandler 中调用 oldSigHandler(signum) 时形成递归调用。

修复方案：
```cpp
if (oldSigHandler != NULL) return; // 已注册则直接返回
```

逻辑正确，有效防止重复注册。

总体评价：这是一个高质量的小型修复 PR，修改精准、测试充分，建议合入。

⚠️ 声明: 本审查报告由 AI 自动生成，仅供参考。建议人工复核后再做最终决定。

zhangjie成员

3月31日评论：

/lgtm

CANN-robot成员

3月31日评论：

🤖 CANN 代码审查报告

PR: #1338 - 修复多次信号注册BUG
严重性: ✅ Low
审查时间: 2026-03-31 14:11

📊 审查结论

✅ 建议合入

严重性: Low
代码质量: 良好
内存安全: ✅ 无风险
安全性: ⚠️ 有隐患（线程安全问题，但风险较低）
测试覆盖: 完整（新增单元测试）
文档完整性: NA

修复逻辑正确且简洁，有效解决了多次信号注册导致的问题。新增了单元测试验证修复效果。唯一需要注意的是潜在的线程安全问题，但在实际场景中风险较低。

📋 修改概述

本 PR 修复了 SIGINT 信号处理函数重复注册的 BUG，防止多次调用 RegisterSiganlHandler 时覆盖原有的信号处理函数。

修改文件: 2个 (+14行, -0行)
核心变更:
- src/dfx/msprof/collector/dvvp/msprof/engine/src/prof_acl_mgr.cpp: 添加 oldSigHandler 非空检查，避免重复注册
- tests/ut/msprof/msprof/test/prof_acl_core_utest.cpp: 新增单元测试验证修复效果

🔍 代码质量检查

1. 内存安全 ✅

✅ 无内存泄漏风险

本次修改不涉及动态内存分配（malloc/free/new/delete）
使用全局变量 oldSigHandler 保存原有信号处理函数指针，无需动态管理

✅ 无资源泄漏

信号处理函数注册/注销由操作系统管理，无需手动释放

2. 线程安全 ⚠️

⚠️ 存在竞态条件（但风险较低）

问题代码：

static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    if (oldSigHandler != NULL) return;  // ⚠️ 非原子操作
    oldSigHandler = signal(SIGINT, newSigHandler);
    profAclMgrObjPtr = ptr;
    MSPROF_LOGI("RegisterSiganlHandler done");
}

潜在问题：

oldSigHandler 是全局变量，多线程环境下存在竞态条件
理论上可能出现：线程A检查为NULL → 线程B检查为NULL → 两个线程都调用 signal()

风险评估：

实际风险较低：信号处理通常在进程启动阶段完成，此时多为单线程
建议：如果将来有多线程注册需求，应添加互斥锁保护

改进建议：

static std::mutex g_signalMutex;

static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    std::lock_guard<std::mutex> lock(g_signalMutex);
    if (oldSigHandler != NULL) return;
    oldSigHandler = signal(SIGINT, newSigHandler);
    profAclMgrObjPtr = ptr;
    MSPROF_LOGI("RegisterSiganlHandler done");
}

3. 安全性 ✅

✅ 无输入验证问题

不涉及外部输入，无需验证

✅ 边界检查完整

无数组/缓冲区操作

✅ 无安全漏洞

NULL 指针检查已添加
信号处理函数使用标准 API

4. 逻辑正确性 ✅

✅ 修复逻辑正确

问题分析：

同一个进程对 SIGINT 信号只能注册一个处理函数
首次调用：oldSigHandler 保存原有处理函数（通常为默认退出函数）
未加保护时，第二次调用会将 oldSigHandler 覆盖为 newSigHandler
导致 Ctrl+C 后无法正常退出程序

修复验证：

// 首次注册
RegisterSiganlHandler();  
// oldSigHandler = 原有处理函数（如 SIG_DFL）

// 再次注册（新增的检查会直接返回）
RegisterSiganlHandler();  
// oldSigHandler 仍为原有处理函数，未被覆盖

✅ 边界条件处理完整

NULL 检查防止重复注册
如果 oldSigHandler 已被设置为其他处理函数，保持不变

✅ 影响范围明确

仅影响 SIGINT 信号处理
不影响其他信号
修改范围小，风险可控

5. 可读性 ✅

✅ 代码清晰度良好

修改简洁明了（1行核心代码）
逻辑直观易懂

✅ 命名规范符合

遵循项目命名风格（驼峰命名）
变量名 oldSigHandler 语义明确

⚠️ 注释完整性可改进

建议添加注释说明重复注册检查的必要性

改进建议：

static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    // 防止重复注册，避免覆盖原有的信号处理函数
    if (oldSigHandler != NULL) return;
    oldSigHandler = signal(SIGINT, newSigHandler);
    profAclMgrObjPtr = ptr;
    MSPROF_LOGI("RegisterSiganlHandler done");
}

🧪 测试覆盖

✅ 新增单元测试完整

测试用例 RegisterSignalHandlerTwice 验证：

初始状态 oldSigHandler 为 NULL
首次注册后 oldSigHandler 被设置为自定义处理函数
再次注册后 oldSigHandler 保持不变

测试代码：

TEST_F(MSPROF_ACL_CORE_UTEST, RegisterSignalHandlerTwice) {
    signal(SIGINT, CustomerSigHandler);
    EXPECT_EQ(Msprofiler::Api::ProfAclMgr::oldSigHandler, NULL);
    RegisterSiganlHandler();
    EXPECT_EQ(Msprofiler::Api::ProfAclMgr::oldSigHandler, CustomerSigHandler);
    RegisterSiganlHandler();  // 再次调用
    EXPECT_EQ(Msprofiler::Api::ProfAclMgr::oldSigHandler, CustomerSigHandler);
}

💡 改进建议

1. 线程安全改进（优先级：中）

虽然当前风险较低，但建议为将来扩展添加互斥锁保护：

static std::mutex g_signalMutex;

static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    std::lock_guard<std::mutex> lock(g_signalMutex);
    if (oldSigHandler != NULL) return;
    oldSigHandler = signal(SIGINT, newSigHandler);
    profAclMgrObjPtr = ptr;
    MSPROF_LOGI("RegisterSiganlHandler done");
}

2. 添加注释（优先级：低）

建议在关键检查处添加注释，提高代码可维护性：

// 防止重复注册，避免覆盖原有的信号处理函数
if (oldSigHandler != NULL) return;

3. 考虑提供注销接口（优先级：低）

如果需要支持动态卸载/重载，建议提供对应的 UnregisterSignalHandler 接口：

static void UnregisterSignalHandler() {
    if (oldSigHandler != NULL) {
        signal(SIGINT, oldSigHandler);
        oldSigHandler = NULL;
        profAclMgrObjPtr = NULL;
    }
}

4. 单元测试改进（优先级：低）

建议在测试中恢复信号处理函数，避免影响其他测试：

TEST_F(MSPROF_ACL_CORE_UTEST, RegisterSignalHandlerTwice) {
    auto originalHandler = signal(SIGINT, CustomerSigHandler);
    EXPECT_EQ(Msprofiler::Api::ProfAclMgr::oldSigHandler, NULL);
    RegisterSiganlHandler();
    EXPECT_EQ(Msprofiler::Api::ProfAclMgr::oldSigHandler, CustomerSigHandler);
    RegisterSiganlHandler();
    EXPECT_EQ(Msprofiler::Api::ProfAclMgr::oldSigHandler, CustomerSigHandler);
    
    // 恢复原有处理函数
    signal(SIGINT, originalHandler);
}

✅ 代码亮点

修复精准 - 仅用1行代码解决核心问题，改动最小化
测试先行 - 同步添加单元测试，确保修复有效性
风险可控 - 修改范围小，不影响现有功能
问题追溯完整 - PR 描述清晰说明了问题背景和修复方案
CI 通过 - 标签显示 ci-pipeline-passed

📝 总体评价

这是一个高质量的小型修复 PR，针对性强、改动精准、测试完整。修复逻辑正确，有效解决了多次信号注册导致的问题。

优点：

✅ 修复逻辑简单明确
✅ 新增单元测试覆盖
✅ 改动范围小，风险低
✅ PR 描述清晰

不足：

⚠️ 存在理论上的线程安全问题（但实际风险低）
⚠️ 缺少代码注释

建议：

当前实现已满足需求，可以合入
如果将来有多线程场景，建议添加互斥锁保护
建议补充代码注释，提高可维护性

⚠️ 声明: 本审查报告由 AI 自动生成，仅供参考。建议人工复核后再做最终决定。

CANN-robot成员

3月31日评论：

🤖 CANN 代码审查报告

PR: #1274 - 修复多次信号注册BUG
作者: jinyingqi
严重性: ✅ Low
审查时间: 2026-03-31 11:06

📊 审查结论

✅ 建议合入

严重性: Low
代码质量: 良好
内存安全: ✅ 无风险
安全性: ✅ 修复了信号处理问题
测试覆盖: 完整（新增单元测试）
文档完整性: 部分（PR描述完整，代码无注释）

修复方案简洁有效，通过添加防护检查避免了信号处理函数的重复注册，解决了Ctrl+C无法正常退出的问题。代码逻辑清晰，测试覆盖充分。

📋 修改概述

本次PR修复了信号处理函数重复注册导致程序无法正常退出的问题。

修改文件: 2个 (+14行, -0行)
核心变更:
- prof_acl_mgr.cpp: 在RegisterSiganlHandler函数中添加防护检查
- prof_acl_core_utest.cpp: 新增单元测试验证修复效果

🔍 代码质量检查

1. 内存安全 ✅

内存泄漏: 无风险（无动态内存分配）
指针操作: 安全（添加了NULL检查，避免指针覆盖）
动态分配: 不适用
资源管理: 合理（使用静态全局变量管理信号处理状态）

2. 安全性 ✅

输入验证: 完整（NULL检查有效）
边界检查: 不适用
潜在漏洞: 修复了原有问题（重复注册导致信号处理链断裂）

3. 可读性 ✅

代码清晰度: 优秀（修改简洁明了）
命名规范: 符合（与现有代码风格一致）
注释完整性: 部分（建议添加注释说明检查目的）

4. 逻辑正确性 ✅

算法逻辑: 正确（符合PR描述的修复方案）
边界条件: 处理完整（NULL检查覆盖所有情况）
影响范围: 明确（仅影响RegisterSiganlHandler函数）

💡 改进建议

线程安全: 建议在文档或注释中说明该函数的线程安全性假设。当前实现依赖于Init()的调用模式，如果多线程并发调用RegisterSiganlHandler，仍存在竞态条件风险（读-检查-写操作非原子）。建议：
- 添加注释说明调用约束
- 或考虑使用std::atomic<decltype(oldSigHandler)>来保证原子性

代码注释: 建议在防护检查处添加注释，说明为什么需要这个检查：

// 避免重复注册信号处理函数，防止覆盖oldSigHandler导致信号处理链断裂
if (oldSigHandler != NULL) return;

代码规范: 函数名 RegisterSiganlHandler 中 "Siganl" 疑似 "Signal" 拼写错误，建议后续统一修正（此问题非本次引入）。

资源恢复: 考虑在UnInit()中恢复原始信号处理函数，确保资源清理完整：

if (oldSigHandler != NULL && oldSigHandler != SIG_IGN) {
    signal(SIGINT, oldSigHandler);
    oldSigHandler = NULL;
}

✅ 代码亮点

修复方案简洁有效：仅用1行代码就解决了问题
测试覆盖充分：新增单元测试验证修复效果
保持向后兼容：不影响现有信号处理链
PR描述详细：清晰说明了问题背景、修复方案和测试验证
符合防御性编程原则：添加的NULL检查提高了代码健壮性

总体评价：这是一个高质量的Bug修复PR，代码简洁、逻辑正确、测试充分。建议合入。上述改进建议为锦上添花，不影响本次合入。

⚠️ 声明: 本审查报告由 AI 自动生成，仅供参考。建议人工复核后再做最终决定。

CANN-robot成员

3月31日评论：

🤖 CANN 代码审查报告

PR: #1338 - 修复多次信号注册BUG
严重性: ⚠️ Medium
审查时间: 2026-03-31 14:21

📊 审查结论

⚠️ 建议修改后合入

严重性: Medium
代码质量: 良好
内存安全: ✅ 无风险
安全性: ⚠️ 有隐患
测试覆盖: 部分
文档完整性: 部分

修复方案思路正确,但存在多线程竞态条件风险,建议增加保护机制。

📋 修改概述

本 PR 修复了信号处理函数重复注册导致 Ctrl+C 无法正常退出的问题。

修改文件: 2个 (+14行, -0行)
核心变更:
- prof_acl_mgr.cpp: 在信号注册函数中添加重复注册检查
- prof_acl_core_utest.cpp: 新增重复注册场景的单元测试

🔍 代码质量检查

1. 内存安全 ✅

内存泄漏: 无风险 - 无动态内存分配
指针操作: 安全 - 使用函数指针,无复杂指针运算
动态分配: N/A - 使用静态全局变量
资源管理: 合理 - 信号处理遵循 POSIX 标准

2. 安全性 ⚠️

输入验证: N/A
边界检查: N/A
潜在漏洞:
- ⚠️ 竞态条件: 在多线程环境下,检查 oldSigHandler != NULL 和设置 oldSigHandler 不是原子操作,可能导致重复注册
- 建议使用 std::atomic<ProfSignalHandler> 或添加互斥锁保护
信号处理链:
- ✅ 正确保持了原有的信号处理链 (newSigHandler 调用 oldSigHandler)
- ⚠️ 检查条件可能不够完整,应该也检查 SIG_IGN 和 SIG_DFL 常量

3. 可读性 ✅

代码清晰度: 良好 - 修改简洁明了
命名规范: 符合 - 函数名清晰表达意图
注释完整性: 部分缺失 - 建议添加注释说明为什么需要这个检查

4. 逻辑正确性 ✅

算法逻辑: 正确 - 修复了重复注册问题
边界条件: 部分处理 - 考虑了首次调用和重复调用场景
影响范围: 明确 - 仅影响信号注册逻辑

💡 改进建议

1. 线程安全性 (重要)

当前实现在多线程环境下存在竞态条件:

// 当前代码
static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    if (oldSigHandler != NULL) return;  // 检查
    oldSigHandler = signal(SIGINT, newSigHandler);  // 设置
    // ...
}

建议方案 A - 使用原子变量:

#include <atomic>

static std::atomic<ProfSignalHandler> oldSigHandler{nullptr};

static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    ProfSignalHandler expected = nullptr;
    if (!oldSigHandler.compare_exchange_strong(expected, signal(SIGINT, newSigHandler))) {
        return;  // 已经被其他线程注册
    }
    profAclMgrObjPtr = ptr;
    MSPROF_LOGI("RegisterSiganlHandler done");
}

建议方案 B - 使用互斥锁:

#include <mutex>

static std::mutex signalMutex;
static ProfSignalHandler oldSigHandler = nullptr;

static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    std::lock_guard<std::mutex> lock(signalMutex);
    if (oldSigHandler != NULL) return;
    oldSigHandler = signal(SIGINT, newSigHandler);
    profAclMgrObjPtr = ptr;
    MSPROF_LOGI("RegisterSiganlHandler done");
}

2. 完善检查条件 (建议)

当前检查 oldSigHandler != NULL 可能不够完整,建议也检查特殊值:

static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    if (oldSigHandler != NULL && oldSigHandler != SIG_DFL && oldSigHandler != SIG_IGN) {
        return;  // 已经注册过自定义处理函数
    }
    oldSigHandler = signal(SIGINT, newSigHandler);
    // ...
}

3. 添加代码注释 (建议)

建议添加注释说明检查的目的:

static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    // 防止重复注册信号处理函数
    // 避免覆盖 oldSigHandler,保持信号处理链的完整性
    if (oldSigHandler != NULL) return;
    
    oldSigHandler = signal(SIGINT, newSigHandler);
    profAclMgrObjPtr = ptr;
    MSPROF_LOGI("RegisterSiganlHandler done");
}

4. 增强单元测试 (建议)

当前测试覆盖了重复调用场景,建议增加以下测试:

测试在已有自定义信号处理函数时的行为
测试信号处理链是否正确传递
(如果应用了线程安全修复) 测试多线程并发注册场景

✅ 代码亮点

修复思路正确: 通过检查避免重复注册,保持信号处理链完整性
代码简洁: 仅增加 1 行核心代码,最小化改动范围
向后兼容: 不影响现有功能和 API
有单元测试: 新增测试验证修复效果
日志记录: 保留了日志输出,便于调试

📝 PR 描述质量

✅ 清晰描述了问题背景和修复方案
✅ 提供了测试验证方法
✅ 标注了类型(Bug修复)
⚠️ 建议补充线程安全性说明

总体评价

修复方案思路正确,有效解决了重复注册信号处理函数的问题,代码简洁且向后兼容。但存在多线程竞态条件的潜在风险,建议在合入前考虑以下选项:

如果确定不会在多线程环境使用 → 可以直接合入
如果可能在多线程环境使用 → 建议先应用线程安全修复再合入
折中方案 → 先合入,但创建新的 Issue 跟踪线程安全改进

建议审查者根据实际使用场景决定是否需要立即修复线程安全问题。

⚠️ 声明: 本审查报告由 AI 自动生成,仅供参考。建议人工复核后再做最终决定。

CANN-robot成员

3月31日评论：

🤖 CANN 代码审查报告

PR: #1338 - 修复多次信号注册BUG
严重性: ⚠️ Medium
审查时间: 2026-03-31 14:21

📊 审查结论

⚠️ 建议修改后合入

严重性: Medium
代码质量: 良好
内存安全: ✅ 无风险
安全性: ⚠️ 有隐患
测试覆盖: 部分
文档完整性: 部分

修复方案思路正确,但存在多线程竞态条件风险,建议增加保护机制。

📋 修改概述

本 PR 修复了信号处理函数重复注册导致 Ctrl+C 无法正常退出的问题。

修改文件: 2个 (+14行, -0行)
核心变更:
- prof_acl_mgr.cpp: 在信号注册函数中添加重复注册检查
- prof_acl_core_utest.cpp: 新增重复注册场景的单元测试

🔍 代码质量检查

1. 内存安全 ✅

内存泄漏: 无风险 - 无动态内存分配
指针操作: 安全 - 使用函数指针,无复杂指针运算
动态分配: N/A - 使用静态全局变量
资源管理: 合理 - 信号处理遵循 POSIX 标准

2. 安全性 ⚠️

输入验证: N/A
边界检查: N/A
潜在漏洞:
- ⚠️ 竞态条件: 在多线程环境下,检查 oldSigHandler != NULL 和设置 oldSigHandler 不是原子操作,可能导致重复注册
- 建议使用 std::atomic<ProfSignalHandler> 或添加互斥锁保护
信号处理链:
- ✅ 正确保持了原有的信号处理链 (newSigHandler 调用 oldSigHandler)
- ⚠️ 检查条件可能不够完整,应该也检查 SIG_IGN 和 SIG_DFL 常量

3. 可读性 ✅

代码清晰度: 良好 - 修改简洁明了
命名规范: 符合 - 函数名清晰表达意图
注释完整性: 部分缺失 - 建议添加注释说明为什么需要这个检查

4. 逻辑正确性 ✅

算法逻辑: 正确 - 修复了重复注册问题
边界条件: 部分处理 - 考虑了首次调用和重复调用场景
影响范围: 明确 - 仅影响信号注册逻辑

💡 改进建议

1. 线程安全性 (重要)

当前实现在多线程环境下存在竞态条件:

// 当前代码
static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    if (oldSigHandler != NULL) return;  // 检查
    oldSigHandler = signal(SIGINT, newSigHandler);  // 设置
    // ...
}

建议方案 A - 使用原子变量:

#include <atomic>

static std::atomic<ProfSignalHandler> oldSigHandler{nullptr};

static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    ProfSignalHandler expected = nullptr;
    if (!oldSigHandler.compare_exchange_strong(expected, signal(SIGINT, newSigHandler))) {
        return;  // 已经被其他线程注册
    }
    profAclMgrObjPtr = ptr;
    MSPROF_LOGI("RegisterSiganlHandler done");
}

建议方案 B - 使用互斥锁:

#include <mutex>

static std::mutex signalMutex;
static ProfSignalHandler oldSigHandler = nullptr;

static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    std::lock_guard<std::mutex> lock(signalMutex);
    if (oldSigHandler != NULL) return;
    oldSigHandler = signal(SIGINT, newSigHandler);
    profAclMgrObjPtr = ptr;
    MSPROF_LOGI("RegisterSiganlHandler done");
}

2. 完善检查条件 (建议)

当前检查 oldSigHandler != NULL 可能不够完整,建议也检查特殊值:

static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    if (oldSigHandler != NULL && oldSigHandler != SIG_DFL && oldSigHandler != SIG_IGN) {
        return;  // 已经注册过自定义处理函数
    }
    oldSigHandler = signal(SIGINT, newSigHandler);
    // ...
}

3. 添加代码注释 (建议)

建议添加注释说明检查的目的:

static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    // 防止重复注册信号处理函数
    // 避免覆盖 oldSigHandler,保持信号处理链的完整性
    if (oldSigHandler != NULL) return;
    
    oldSigHandler = signal(SIGINT, newSigHandler);
    profAclMgrObjPtr = ptr;
    MSPROF_LOGI("RegisterSiganlHandler done");
}

4. 增强单元测试 (建议)

当前测试覆盖了重复调用场景,建议增加以下测试:

测试在已有自定义信号处理函数时的行为
测试信号处理链是否正确传递
(如果应用了线程安全修复) 测试多线程并发注册场景

✅ 代码亮点

修复思路正确: 通过检查避免重复注册,保持信号处理链完整性
代码简洁: 仅增加 1 行核心代码,最小化改动范围
向后兼容: 不影响现有功能和 API
有单元测试: 新增测试验证修复效果
日志记录: 保留了日志输出,便于调试

📝 PR 描述质量

✅ 清晰描述了问题背景和修复方案
✅ 提供了测试验证方法
✅ 标注了类型(Bug修复)
⚠️ 建议补充线程安全性说明

总体评价

修复方案思路正确,有效解决了重复注册信号处理函数的问题,代码简洁且向后兼容。但存在多线程竞态条件的潜在风险,建议在合入前考虑以下选项:

如果确定不会在多线程环境使用 → 可以直接合入
如果可能在多线程环境使用 → 建议先应用线程安全修复再合入
折中方案 → 先合入,但创建新的 Issue 跟踪线程安全改进

建议审查者根据实际使用场景决定是否需要立即修复线程安全问题。

⚠️ 声明: 本审查报告由 AI 自动生成,仅供参考。建议人工复核后再做最终决定。

CANN-robot成员

3月31日评论：

🤖 CANN 代码审查报告

PR: #1338 - 修复多次信号注册BUG
严重性: ✅ Low
审查时间: 2026-03-31 14:31

📊 审查结论

✅ 建议合入

严重性: Low
代码质量: 良好
内存安全: ✅ 无风险
安全性: ✅ 无漏洞
测试覆盖: 完整
文档完整性: 部分

该 PR 修复了信号处理器的重复注册问题，代码修改简洁有效，并提供了单元测试验证修复的正确性。建议合入。

📋 修改概述

修复了 msprof ACL API 模式中信号处理器重复注册导致 Ctrl+C 无法正常退出的问题。

修改文件: 2个 (+14行, -0行)
核心变更:
- src/dfx/msprof/collector/dvvp/msprof/engine/src/prof_acl_mgr.cpp: 添加重复注册保护
- tests/ut/msprof/msprof/test/prof_acl_core_utest.cpp: 添加单元测试

问题背景:

同一个进程对 SIGINT 信号只能注册一个信号处理函数
首次调用时，oldSigHandler 保存了原有的信号处理函数（通常是默认的退出函数）
未加保护时，第二次调用会将 oldSigHandler 覆盖为 newSigHandler
导致 Ctrl+C 后无法正常退出程序

修复方案:

在注册信号处理函数前，检查 oldSigHandler 是否已被设置
如果已设置，直接返回，避免重复注册

🔍 代码质量检查

1. 内存安全 ✅

内存泄漏: 无风险 - 未涉及动态内存分配
指针操作: 安全 - 静态全局变量使用合理
动态分配: 不适用 - 无动态内存分配
资源管理: 良好 - 使用静态变量管理信号处理器

代码分析:

static ProfSignalHandler oldSigHandler = nullptr;  // 初始化为 nullptr

static void RegisterSiganlHandler(ProfAclMgr* ptr) {
    if (oldSigHandler != NULL) return;  // ✅ 新增：防止重复注册
    oldSigHandler = signal(SIGINT, newSigHandler);  // 保存旧的信号处理器
    profAclMgrObjPtr = ptr;
    MSPROF_LOGI("RegisterSiganlHandler done");
}

static void newSigHandler(int signum) {
    if (profAclMgrObjPtr != NULL) {
        profAclMgrObjPtr->MsprofFinalizeHandle();
    }
    if(oldSigHandler && oldSigHandler != SIG_IGN && oldSigHandler != newSigHandler) {
        oldSigHandler(signum);  // ✅ 调用旧的信号处理器
    }
}

2. 安全性 ✅

输入验证: 不适用 - 信号处理器注册，无外部输入
边界检查: 不适用 - 无数组或缓冲区操作
潜在漏洞: 无

安全性分析:

✅ 正确检查 oldSigHandler 非空后才调用
✅ 正确处理 SIG_IGN 特殊情况
✅ 防止自调用（oldSigHandler != newSigHandler）
✅ 新增的保护检查避免了状态覆盖问题

POSIX 标准兼容性:

signal() 返回值可能是 SIG_DFL、SIG_IGN 或函数指针
SIG_DFL 在大多数平台上是非零值，因此 != NULL 检查是可靠的
但在极少数平台上如果 SIG_DFL 是 0，这个检查可能有潜在问题（虽然实际很少见）

3. 可读性 ✅

代码清晰度: 良好 - 修改简洁明了
命名规范: 符合 - 变量命名清晰（oldSigHandler、newSigHandler）
注释完整性: 部分 - PR 描述详细，但代码中缺少行内注释

建议: 在新增的检查行添加注释说明目的：

// Avoid duplicate registration: if oldSigHandler is set, we've already registered
if (oldSigHandler != NULL) return;

4. 逻辑正确性 ✅

算法逻辑: 正确 - 通过检查变量状态防止重复注册
边界条件: 处理完整 - 考虑了首次调用和重复调用的情况
影响范围: 明确 - 仅影响信号处理器注册逻辑

逻辑验证:

初始状态: oldSigHandler = nullptr
第一次调用: if (nullptr != NULL) → false，执行注册
第二次调用: if (old_handler != NULL) → true，直接返回 ✅

单元测试验证:

TEST_F(MSPROF_ACL_CORE_UTEST, RegisterSignalHandlerTwice) {
    signal(SIGINT, CustomerSigHandler);
    EXPECT_EQ(Msprofiler::Api::ProfAclMgr::oldSigHandler, NULL);  // ✅ 初始为 NULL
    RegisterSiganlHandler();
    EXPECT_EQ(Msprofiler::Api::ProfAclMgr::oldSigHandler, CustomerSigHandler);  // ✅ 保存了旧处理器
    RegisterSiganlHandler();
    EXPECT_EQ(Msprofiler::Api::ProfAclMgr::oldSigHandler, CustomerSigHandler);  // ✅ 未被覆盖
}

💡 改进建议

注释增强: 建议在新增的检查行添加注释说明防止重复注册的目的，提高代码可维护性
```
if (oldSigHandler != NULL) return;  // Avoid duplicate signal handler registration
```
常量使用: 考虑使用 nullptr 替代 NULL 以符合现代 C++ 风格
```
if (oldSigHandler != nullptr) return;
```
文档完善: 虽然单元测试充分，但建议在函数注释中说明该函数不是线程安全的，不应在多线程环境下并发调用
函数命名: RegisterSiganlHandler 中 "Siganl" 有拼写错误，应为 "Signal"（虽然这是历史遗留问题，不在本次修改范围内）

✅ 代码亮点

修复精准: 通过简单的状态检查就解决了重复注册的核心问题
测试充分: 添加了单元测试验证修复的正确性，测试覆盖了边界情况
最小改动: 修改量最小化，降低了引入新 bug 的风险
向后兼容: 不影响现有的信号处理逻辑
问题定位清晰: PR 描述详细说明了问题背景和修复方案

总体评价: 这是一个高质量的 Bug 修复 PR，代码简洁、逻辑正确、测试充分，建议合入。

⚠️ 声明: 本审查报告由 AI 自动生成，仅供参考。建议人工复核后再做最终决定。

zhuliangying成员

3月31日评论：

/approve

CANN-robot成员

3月31日添加了label：lgtmapproved

jinyingqi成员

3月31日评论：

/check-pr

jinyingqi成员

3月31日评论：

/check-pr

CANN-robot成员

3月31日评论：

Review Guide

This pull-request passes review.
Committers who wrote a comment of /approve are: zhuliangying.
Reviewers who wrote a comment of /lgtm are: newstarzj, zhuliangying.

CANN-robot成员

3月31日合入了pull request