[LiHua000]

cherry-pick 2 commits to :
remove useless code and Enhance command execution security and robustness

bugs: https://pms.uniontech.com/bug-view-350769.html

[deepin-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LiHua000, max-lvs

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

Details

Needs approval from an approver in each of these files:

• debian/deepin/OWNERS

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

[LiHua000]

/merge

[deepin-bot]

This pr cannot be merged! (status: unstable)

[LiHua000]

/forcemerge

[deepin-bot]

This pr force merged! (status: unstable)

[deepin-ci-robot]

deepin pr auto review

Git Diff 代码审查报告

总体概述

本次代码变更主要进行了以下重构：

1. 移除了独立的 opsLogViewerService 子项目，将其功能合并到 logViewerService 中
2. 将原来使用 system() 执行 shell 命令的方式改为使用 QProcess，提高了安全性和可控性
3. 优化了日志导出功能的实现，增加了通配符路径处理等功能
4. 改进了 DBus 调用方式，使用异步调用替代同步调用

详细审查意见

1. 语法和逻辑问题

1.1 grepA 函数实现问题

QList<QStringList> grepA(const QByteArray &data, const QString &pattern, int afterLines)
{
    // ...
    QTextStream in(&buffer);
    // 假设数据是 UTF-8 编码，如果不是，需要设置正确的 codec
    // in.setCodec("UTF-8");
    
    while (!in.atEnd()) {
        QString line = in.readLine();
        // 检查当前行是否包含目标模式
        if (line.contains(pattern)) {
            // ...
        }
    }
    // ...
}

问题：

• 注释中提到需要设置正确的 codec，但实际代码中没有设置
• 对于不同编码的日志文件，可能会导致乱码

建议：

QTextStream in(&buffer);
in.setCodec("UTF-8");  // 明确设置编码

1.2 expandPathWithWildcardIterator 函数问题

static QStringList expandPathWithWildcardIterator(const QString &pathWithWildcard)
{
    // ...
    QDirIterator it(dirPath, nameFilters, QDir::Files | QDir::NoDotAndDotDot, QDirIterator::NoIteratorFlags);
    
    while (it.hasNext()) {
        matchedFiles.append(it.next());
    }
    return matchedFiles;
}

问题：

• 没有检查 dirPath 是否存在
• 如果目录不存在，函数会返回空列表，但不会给出任何提示

建议：

static QStringList expandPathWithWildcardIterator(const QString &pathWithWildcard)
{
    QFileInfo fileInfo(pathWithWildcard);
    QString dirPath = fileInfo.path();
    QString fileNamePattern = fileInfo.fileName();
    
    QStringList matchedFiles;
    
    // 检查目录是否存在
    QDir dir(dirPath);
    if (!dir.exists()) {
        qWarning() << "Directory does not exist:" << dirPath;
        return matchedFiles;
    }
    
    QStringList nameFilters;
    nameFilters << fileNamePattern;
    
    QDirIterator it(dirPath, nameFilters, QDir::Files | QDir::NoDotAndDotDot, QDirIterator::NoIteratorFlags);
    
    while (it.hasNext()) {
        matchedFiles.append(it.next());
    }
    
    return matchedFiles;
}

1.3 appendToFile 函数重复定义

在 utils.cpp 和 opslogexport.cpp 中都定义了相同的 appendToFile 函数。

问题：

• 代码重复，违反 DRY (Don't Repeat Yourself) 原则
• 维护时需要同时修改两处代码

建议：
将 appendToFile 函数提取到一个公共的头文件中，或者将其作为静态工具类的方法。

2. 代码质量问题

2.1 错误处理不完善

static void appendToFile(const QString &filePath, const QByteArray &content)
{
    QFile file(filePath);
    
    if (file.open(QIODevice::WriteOnly | QIODevice::Append | QIODevice::Text)) {
        file.write(content);
    } else {
        qWarning() << "Error opening file for writing:" << file.errorString();
    }
}

问题：

• 打开文件失败后只记录警告，但函数没有返回值，调用者无法知道操作是否成功
• 写入文件后没有检查是否成功

建议：

static bool appendToFile(const QString &filePath, const QByteArray &content)
{
    QFile file(filePath);
    
    if (!file.open(QIODevice::WriteOnly | QIODevice::Append | QIODevice::Text)) {
        qWarning() << "Error opening file for writing:" << file.errorString();
        return false;
    }
    
    if (file.write(content) != content.size()) {
        qWarning() << "Error writing to file:" << file.errorString();
        return false;
    }
    
    return true;
}

2.2 executeCmd 函数命名不一致

在 utils.cpp 中函数名为 executeCmd，而在 opslogexport.cpp 中函数名为 executCmd（少了一个 'e'）。

建议：
统一函数命名，避免拼写错误。

2.3 代码注释不足

static int executCmd(const char *strCmd, const char *outputFile = nullptr, const QString &pattern = "", int afterLines = -1)
{
    qDebug() << "executCmd" << strCmd;
    
    QString error = "";
    int exitcode = 0;
    QProcess proc;
    QString prog;
    
    QStringList args = parseCombinedArgString(strCmd);
    if (args.isEmpty()) {
        error = "args is empty";
        exitcode = -1;
        goto quit;
    }
    prog = args.takeFirst();
    
    proc.setProgram(prog);
    proc.setArguments(args);
    proc.start(QIODevice::ReadWrite);
    
    proc.waitForFinished(-1);
    if (outputFile) {
        if (pattern != "" && afterLines >= 0) {
            appendToFileWithGrepA(outputFile, proc.readAllStandardOutput(), pattern, afterLines);
        } else {
            appendToFile(outputFile, proc.readAllStandardOutput());
        }
    }
    error = proc.errorString();
    exitcode = proc.exitCode();
    proc.close();
    
quit:
    qDebug() << "executCmd exitcode:" << exitcode << error;
    return exitcode;
}

问题：

• 使用了 goto 语句，虽然在错误处理中可以使用，但不符合现代 C++ 编码风格
• 函数参数较多，缺乏注释说明各个参数的作用

建议：

/**
 * @brief 执行命令并可选择性地将输出写入文件
 * @param strCmd 要执行的命令字符串
 * @param outputFile 输出文件路径，如果为 nullptr 则不写入文件
 * @param pattern 如果不为空，则只输出包含该模式的行及其后 afterLines 行
 * @param afterLines 在匹配 pattern 后额外输出的行数
 * @return 命令的退出码，-1 表示参数错误
 */
static int executeCmd(const char *strCmd, const char *outputFile = nullptr, 
                      const QString &pattern = "", int afterLines = -1)
{
    qDebug() << "executeCmd" << strCmd;
    
    int exitcode = -1;
    QProcess proc;
    
    QStringList args = parseCombinedArgString(strCmd);
    if (args.isEmpty()) {
        qWarning() << "Empty command arguments";
        return exitcode;
    }
    
    QString prog = args.takeFirst();
    proc.setProgram(prog);
    proc.setArguments(args);
    proc.start(QIODevice::ReadWrite);
    
    if (!proc.waitForFinished(-1)) {
        qWarning() << "Command execution failed:" << proc.errorString();
        return exitcode;
    }
    
    if (outputFile) {
        if (!pattern.isEmpty() && afterLines >= 0) {
            appendToFileWithGrepA(outputFile, proc.readAllStandardOutput(), pattern, afterLines);
        } else {
            appendToFile(outputFile, proc.readAllStandardOutput());
        }
    }
    
    exitcode = proc.exitCode();
    qDebug() << "executeCmd exitcode:" << exitcode << proc.errorString();
    
    return exitcode;
}

3. 代码性能问题

3.1 grepA 函数性能优化

QList<QStringList> grepA(const QByteArray &data, const QString &pattern, int afterLines)
{
    QList<QStringList> allMatches;
    
    QBuffer buffer;
    buffer.setData(data);
    if (!buffer.open(QIODevice::ReadOnly | QIODevice::Text)) {
        qWarning() << "Cannot open QBuffer for reading.";
        return allMatches;
    }
    
    QTextStream in(&buffer);
    // in.setCodec("UTF-8");
    
    while (!in.atEnd()) {
        QString line = in.readLine();
        
        if (line.contains(pattern)) {
            QStringList currentMatchGroup;
            currentMatchGroup.append(line);
            
            for (int i = 0; i < afterLines && !in.atEnd(); ++i) {
                currentMatchGroup.append(in.readLine());
            }
            
            allMatches.append(currentMatchGroup);
        }
    }
    
    return allMatches;
}

问题：

• 对于大文件，逐行读取可能会比较慢
• 使用 QString::contains 进行匹配，对于大量数据可能效率不高

建议：

• 对于大文件，可以考虑使用更高效的搜索算法
• 如果性能成为瓶颈，可以考虑使用多线程处理

4. 代码安全问题

4.1 命令注入风险

static int executCmd(const char *strCmd, const char *outputFile = nullptr, const QString &pattern = "", int afterLines = -1)
{
    // ...
    QStringList args = parseCombinedArgString(strCmd);
    if (args.isEmpty()) {
        error = "args is empty";
        exitcode = -1;
        goto quit;
    }
    prog = args.takeFirst();
    
    proc.setProgram(prog);
    proc.setArguments(args);
    proc.start(QIODevice::ReadWrite);
    // ...
}

优点：

• 使用 QProcess 替代 system() 函数，大大降低了命令注入的风险
• 通过 parseCombinedArgString 函数正确处理命令参数

建议：

• 可以考虑添加一个白名单机制，只允许执行特定的命令
• 对于某些敏感命令（如 rm、chmod 等），可以添加额外的检查

4.2 文件路径安全问题

static void appendToFile(const QString &filePath, const QByteArray &content)
{
    QFile file(filePath);
    
    if (file.open(QIODevice::WriteOnly | QIODevice::Append | QIODevice::Text)) {
        file.write(content);
    } else {
        qWarning() << "Error opening file for writing:" << file.errorString();
    }
}

问题：

• 没有验证文件路径是否合法
• 可能存在路径遍历攻击的风险

建议：

static bool appendToFile(const QString &filePath, const QByteArray &content)
{
    // 规范化路径，防止路径遍历攻击
    QString normalizedPath = QDir::cleanPath(filePath);
    
    // 检查路径是否在允许的目录范围内
    // 这里可以根据实际情况添加更多检查
    if (normalizedPath.startsWith("../") || normalizedPath.contains("/../")) {
        qWarning() << "Invalid file path:" << filePath;
        return false;
    }
    
    QFile file(normalizedPath);
    
    if (!file.open(QIODevice::WriteOnly | QIODevice::Append | QIODevice::Text)) {
        qWarning() << "Error opening file for writing:" << file.errorString();
        return false;
    }
    
    if (file.write(content) != content.size()) {
        qWarning() << "Error writing to file:" << file.errorString();
        return false;
    }
    
    return true;
}

5. 其他建议

5.1 DBus 调用改进

bool DLDBusHandler::exportOpsLog(const QString &outDir, const QString &userHomeDir)
{
    m_dbus->setTimeout(1200000);
    QDBusPendingReply<bool> reply = m_dbus->exportOpsLog(outDir, userHomeDir);
    reply.waitForFinished();
    m_dbus->setTimeout(-1);
    
    if (reply.isError()) {
        qCritical(logDBusHandler) << "call dbus interface 'exportHwOpsLog' failed. error info:" << reply.error().message();
        return false;
    } else {
        qCDebug(logDBusHandler) << "exportOpsLog succeeded:" << reply.value();
        return reply.value();
    }
}

问题：

• 超时时间设置为 1200000 毫秒（20 分钟），可能过长
• 错误消息中提到 exportHwOpsLog，但实际调用的是 exportOpsLog

建议：

bool DLDBusHandler::exportOpsLog(const QString &outDir, const QString &userHomeDir)
{
    // 使用合理的超时时间，例如 5 分钟
    const int timeoutMs = 300000;
    m_dbus->setTimeout(timeoutMs);
    
    QDBusPendingReply<bool> reply = m_dbus->exportOpsLog(outDir, userHomeDir);
    reply.waitForFinished();
    
    // 恢复默认超时时间
    m_dbus->setTimeout(-1);
    
    if (reply.isError()) {
        qCritical(logDBusHandler) << "call dbus interface 'exportOpsLog' failed. error info:" << reply.error().message();
        return false;
    } else {
        qCDebug(logDBusHandler) << "exportOpsLog succeeded:" << reply.value();
        return reply.value();
    }
}

5.2 代码组织建议

• 将 appendToFile、expandPathWithWildcardIterator 等通用工具函数提取到一个单独的工具类中
• 将 grepA、appendToFileWithGrepA 等日志处理相关的函数组织在一起
• 考虑使用命名空间来组织这些工具函数，避免全局命名空间污染

总结

本次代码变更在安全性和可维护性方面有显著改进，特别是将 system() 调用替换为 QProcess，大大降低了命令注入的风险。但在代码质量、错误处理和性能优化方面还有改进空间。建议按照上述意见进行优化，以提高代码的健壮性和可维护性。