已合并
[Profiling]修复--application参数错误把-m识别成路径,命令解析失败问题 #512
[Profiling]修复--application参数错误把-m识别成路径,命令解析失败问题 #512
已合并
zhengkai创建于 10 天前
zhengkai成员
10 天前

描述

问题:使用msprof --application="python3 -m ais-bench xxxx"场景时,在现有的设计里,会将python3后面的"-m"按appPath拆解成app_dir和app,会因为不支持校验和拆解而报错;
方案:
在用户执行的是python或者sh命令时,直接将python或sh按appPath处理,app_dir设置成当前路径,用于后续设置result_dir,app设置python或sh,用于后续的校验等流程;application.cpp里拼接执行命令时不再传入app_dir和app;
application.cpp里删除原app_dir和app的相关处理,直接使用cmdPath参数

关联的Issue

Issue #183

测试

msprof --application="python3 -m ais-bench xxxx"

文档更新

NA

类型标签

likedislike
Pull Request已成功合入, 合并人@CANN-robot
(感谢 zhengkai 的贡献)
Zzhengkai成员
10 天前 创建了 pull request,commit 384096c3
atomgit-bot
atomgit-bot
10 天前 评论:

变更摘要

本 PR([Profiling]fix application path)主要针对 Profiling 模块中应用路径(application path)的处理逻辑进行修复:移除了 Application::GetAppPath 的声明与实现,并在 Application::ResolveAppCmd 中删除了通过 GetAppPath 提取应用路径以及对路径进行 Utils::IsSoftLink 软链接校验的相关逻辑,同时同步清理了对应的单元测试。

主要改动

  • 移除 GetAppPath 函数实现:在 src/msprof/collector/dvvp/app/application.cpp 中删除了 Application::GetAppPath 的完整实现(包括空参数返回空串、按 Utils::IsAppName 判断取 paramsCmd[0]paramsCmd[1] 的逻辑)。
  • 删除 ResolveAppCmd 中的路径校验逻辑:在 Application::ResolveAppCmd 中移除对 GetAppPath 的调用、appPath 为空的失败返回,以及 Utils::IsSoftLink 软链接检查(含对应的 MSPROF_LOGE 错误日志)。
  • 移除 GetAppPath 声明:在 src/msprof/collector/dvvp/app/application.h 中删除 static std::string GetAppPath(std::vector<std::string> paramsCmd); 的私有方法声明。
  • 清理对应单元测试:在 test/ut/msprof/app/test/application_utest.cpp 中删除 GetAppPath 测试用例及 LaunchApp 测试中对 Utils::IsSoftLink 的 mock 桩设置。
likedislike
atomgit-bot
atomgit-bot
10 天前 评论:

代码审查

✅ 未发现问题

likedislike
CANN-robotCANN-robot成员
10 天前 添加了label:cann-cla/yes
CANN-robot
CANN-robot成员
10 天前 评论:

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.
You can self-configure the PR merge rules for this repository. For more details, please refer to Here.
For more, you also can visit HICANN.


PR Approval Progress

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

Module Approval Details

module lgtm status approve status
repo-cann/oam-tools 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

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

likedislike
CANN-robotCANN-robot成员
10 天前 将newstarzj,chenhao_1209,wulinyu,zhuliangying,yaoguangxiu,fujun19,jinyingqi,yuanjiang_设为评审人
CANN-robotCANN-robot成员
10 天前 将newstarzj,chenhao_1209,wulinyu,zhuliangying,yaoguangxiu,fujun19,jinyingqi,yuanjiang_设为审查人
zhengkai成员
10 天前 评论:

compile

likedislike
Zzhengkai成员
10 天前 update merge request[project id: 8784133, iid: 512, commit_id: 4c3f1658250460415cb8a8d2931aa4196f75ebc9] virtual merging success
Zzhengkai成员
10 天前 修改了pull request 的描述
CANN-robotCANN-robot成员
10 天前 添加了label:ci-pipeline-running
CANN-robot
CANN-robot成员
10 天前 评论:

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

任务名称状态日志下载链接
codecheck ✅ SUCCESS >>>>>
SCA ✅ SUCCESS >>>>>
antipoison ✅ SUCCESS >>>>>
Check_Pr ✅ SUCCESS >>>>>
Compile_Ascend_X86 ✅ SUCCESS >>>>> >>>>>
Compile_Ascend_ARM ✅ SUCCESS >>>>> >>>>>
Compile_Ascend_X86_ubuntu24 ✅ SUCCESS >>>>> >>>>>
pre_comment ✅ SUCCESS >>>>>
cocheck_codestyle ✅ SUCCESS
precommit ✅ SUCCESS >>>>>
StaticCheck_codespell ✅ SUCCESS
StaticCheck_link_validity ✅ SUCCESS
StaticCheck_resource_existence ✅ SUCCESS
StaticCheck_tag_closed ✅ SUCCESS
StaticCheck_markdownlint ✅ SUCCESS
UT_Test_asys ✅ SUCCESS >>>>>
UT_Test_msaicerr ✅ SUCCESS >>>>>
UT_Test_msprof ✅ SUCCESS >>>>>
API_Check ✅ SUCCESS >>>>>
PreSmoke_A900 ✅ SUCCESS >>>>>

[2026-08-19 11:35:33]    CI执行结束

likedislike
Zzhengkai成员
10 天前 修改了pull request 的描述
此处折叠了5条事件消息 查看更多
CANN-robotCANN-robot成员
10 天前 添加了label:ci-pipeline-passed
CANN-robot
CANN-robot成员
10 天前 评论:
流水线 PR-pipeline_cann_oam-tools#1711 [ commitID:d87acf4d ] 已完成
likedislike
zhangjie
zhangjie成员
10 天前 评论:

/lgtm

likedislike
zhuliangying成员
9 天前 评论:
likedislike
zhuliangying成员
9 天前 评论:

MR 512 代码审查

变更概述

本 MR 移除了 Application::GetAppPath() 函数及其相关的软链接检查逻辑,涉及:

  • 删除 GetAppPath() 函数实现(application.cpp)
  • 删除函数声明(application.h)
  • 删除对应的单元测试(application_utest.cpp)
  • ResolveAppCmd() 中移除对 GetAppPath() 的调用和软链接校验

潜在问题

1. 🔴 关键安全检查被移除

问题描述

// 被移除的代码
if (analysis::dvvp::common::utils::Utils::IsSoftLink(appPath)) {
    MSPROF_LOGE("app_dir(%s) is soft link.", Utils::BaseName(appPath).c_str());
    return PROFILING_FAILED;
}

风险

  • 软链接检查可能是出于安全考虑(防止符号链接攻击、路径遍历等)
  • 如果应用路径允许软链接,可能导致用户通过软链接绕过权限检查或指向意外位置
  • 这种安全检查的移除需要明确的需求或安全评审支持

建议

  1. 在 PR 描述中说明为什么软链接检查不再需要
  2. 如果是因为检查逻辑错误(如标题所说"错误把 -m 识别成路径"),应该修复检查逻辑而不是完全移除
  3. 如果确实不需要,请确认是否有其他地方进行了等效的安全检查

2. ⚠️ 空路径检查缺失

问题描述
原代码中 GetAppPath() 会提取并返回应用路径,ResolveAppCmd() 随后检查:

std::string appPath = GetAppPath(paramsCmd);
if (appPath.empty()) {
    MSPROF_LOGE("app_dir is empty.");
    return PROFILING_FAILED;
}

移除后,代码直接使用 paramsCmd[0]

cmd = GetCmdString(paramsCmd[0]);
if (cmd.empty()) {
    MSPROF_LOGE("app_dir(%s) is not valid.", Utils::BaseName(paramsCmd[0]).c_str());
}

分析

  • GetAppPath() 的逻辑是:如果 paramsCmd[0] 是应用名(如 bashpython),返回它;否则如果有第二个参数,返回 paramsCmd[1]
  • 这个提取逻辑对于处理 -m 参数等场景可能确实有问题(与 PR 标题吻合)
  • 但移除后是否正确处理了所有场景?特别是当 paramsCmd[0] 是类似 bash -c ... 这样的命令时

建议

  • 补充单元测试覆盖 --application 参数与 -m 参数组合的场景
  • 确认 GetCmdString() 能正确处理所有原来 GetAppPath() 处理的情况

3. 📋 测试覆盖度降低

问题描述

  • 删除了 GetAppPath 的单元测试(18 行)
  • 删除了 LaunchApp 测试中的 IsSoftLink mock

建议

  • 虽然移除了函数,但应该增加测试验证移除后的逻辑仍然正确
  • 特别是验证修复的问题(-m 被错误识别为路径)不再出现
  • 建议添加回归测试:
    TEST_F(PROF_APPLICATION_TEST, ApplicationParamWithDashM) {
        // 验证 -m 参数不会被当作路径处理
    }
    

代码质量

✅ 代码移除干净,没有留下死代码
✅ 相关的头文件声明同步更新
✅ 单元测试同步清理

总体评估

需要澄清

  1. 软链接检查移除的安全影响评估
  2. 原问题的详细复现场景(-m 如何被误识别)
  3. 修复后是否有端到端测试验证问题已解决

风险等级:🟡 中等

  • 移除安全检查需要谨慎
  • 逻辑简化可能遗漏边界场景
likedislike
zhuliangying成员
8 天前 评论:

/approve

likedislike
CANN-robotCANN-robot成员
8 天前 添加了label:lgtmapproved
CANN-robotCANN-robot成员
8 天前 合入了pull request