Skip to content

fix: resolve potential crash and resource management issues in network module#551

Open
18202781743 wants to merge 1 commit into
linuxdeepin:masterfrom
18202781743:master
Open

fix: resolve potential crash and resource management issues in network module#551
18202781743 wants to merge 1 commit into
linuxdeepin:masterfrom
18202781743:master

Conversation

@18202781743
Copy link
Copy Markdown
Contributor

  1. Add proper destructor for DccNetwork to disconnect signals before
    object destruction
  2. Fix potential null pointer dereference in root() method by adding
    null check for m_manager
  3. Clean up VPN state update timer in NetManagerThreadPrivate destructor
    to prevent dangling timer callbacks
  4. Increase thread wait timeout from 200ms to 1000ms in destructor to
    avoid forced thread termination during active operations
  5. Replace QTimer::singleShot with a member timer for VPN state updates
    to prevent callback accumulation and ensure proper cleanup
  6. Add mutex protection for NetworkController singleton instance and
    free operations to fix thread-safety issues

Log: Fixed potential crashes and resource leaks in network management;
improved thread safety for singleton access

Influence:

  1. Test network plugin initialization and destruction cycles repeatedly
  2. Verify DccNetwork object creation and destruction does not cause
    signal-slot errors
  3. Test VPN connection state changes and verify no duplicate timer
    callbacks
  4. Test rapid VPN state transitions to confirm proper timer reset
    behavior
  5. Test NetworkController::instance() and free() from multiple threads
  6. Verify thread termination during active network operations completes
    gracefully
  7. Check for any memory leaks or dangling pointers after repeated
    initialization

fix: 修复网络模块中的潜在崩溃和资源管理问题

  1. 为 DccNetwork 添加正确的析构函数,在对象销毁前断开信号连接
  2. 修复 root() 方法中潜在的空指针解引用问题,添加对 m_manager 的空值检查
  3. 清理 NetManagerThreadPrivate 析构函数中的 VPN 状态更新定时器,防止悬
    空定时器回调
  4. 将析构函数中的线程等待超时从 200ms 增加到 1000ms,避免正在执行的任务
    被强制终止
  5. 用成员定时器替换 QTimer::singleShot 进行 VPN 状态更新,防止回调累积并
    确保正确清理
  6. 为 NetworkController 单例的 instance() 和 free() 操作添加互斥锁保护,
    解决线程安全问题

Log: 修复了网络管理中的潜在崩溃和资源泄漏问题,改善了单例访问的线程安
全性

Influence:

  1. 反复测试网络插件的初始化和销毁周期
  2. 验证 DccNetwork 对象的创建和销毁不会导致信号槽错误
  3. 测试 VPN 连接状态变化,确认没有重复的定时器回调
  4. 测试快速切换 VPN 状态,验证定时器重置行为正确
  5. 从多个线程测试 NetworkController::instance() 和 free()
  6. 验证线程在活跃网络操作期间能优雅地终止
  7. 检查重复初始化后是否有内存泄漏或悬空指针

PMS: BUG-353781

module

1. Add proper destructor for DccNetwork to disconnect signals before
object destruction
2. Fix potential null pointer dereference in root() method by adding
null check for m_manager
3. Clean up VPN state update timer in NetManagerThreadPrivate destructor
to prevent dangling timer callbacks
4. Increase thread wait timeout from 200ms to 1000ms in destructor to
avoid forced thread termination during active operations
5. Replace QTimer::singleShot with a member timer for VPN state updates
to prevent callback accumulation and ensure proper cleanup
6. Add mutex protection for NetworkController singleton instance and
free operations to fix thread-safety issues

Log: Fixed potential crashes and resource leaks in network management;
improved thread safety for singleton access

Influence:
1. Test network plugin initialization and destruction cycles repeatedly
2. Verify DccNetwork object creation and destruction does not cause
signal-slot errors
3. Test VPN connection state changes and verify no duplicate timer
callbacks
4. Test rapid VPN state transitions to confirm proper timer reset
behavior
5. Test NetworkController::instance() and free() from multiple threads
6. Verify thread termination during active network operations completes
gracefully
7. Check for any memory leaks or dangling pointers after repeated
initialization

fix: 修复网络模块中的潜在崩溃和资源管理问题

1. 为 DccNetwork 添加正确的析构函数,在对象销毁前断开信号连接
2. 修复 root() 方法中潜在的空指针解引用问题,添加对 m_manager 的空值检查
3. 清理 NetManagerThreadPrivate 析构函数中的 VPN 状态更新定时器,防止悬
空定时器回调
4. 将析构函数中的线程等待超时从 200ms 增加到 1000ms,避免正在执行的任务
被强制终止
5. 用成员定时器替换 QTimer::singleShot 进行 VPN 状态更新,防止回调累积并
确保正确清理
6. 为 NetworkController 单例的 instance() 和 free() 操作添加互斥锁保护,
解决线程安全问题

Log: 修复了网络管理中的潜在崩溃和资源泄漏问题,改善了单例访问的线程安
全性

Influence:
1. 反复测试网络插件的初始化和销毁周期
2. 验证 DccNetwork 对象的创建和销毁不会导致信号槽错误
3. 测试 VPN 连接状态变化,确认没有重复的定时器回调
4. 测试快速切换 VPN 状态,验证定时器重置行为正确
5. 从多个线程测试 NetworkController::instance() 和 free()
6. 验证线程在活跃网络操作期间能优雅地终止
7. 检查重复初始化后是否有内存泄漏或悬空指针

PMS: BUG-353781
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @18202781743, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743

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

@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这份 Git Diff 主要对网络管理模块进行了多项改进,包括内存管理、线程安全、事件防抖以及空指针防护。整体改动方向正确,但在细节上仍有一些逻辑和性能方面的隐患。以下是我的详细审查意见:

一、 语法与逻辑

1. 析构函数中断开信号可能无效且存在逻辑矛盾

  • 文件: dccnetwork.cpp
  • 代码:
    DccNetwork::~DccNetwork()
    {
        if (m_manager) {
            disconnect(m_manager, &NetManager::request, this, &DccNetwork::request);
        }
    }
  • 问题: DccNetwork 继承自 QObject,在 QObject 的析构过程中,会自动断开该对象的所有信号槽连接。因此,在析构函数中手动 disconnect 是冗余的。此外,如果 m_manager 的生命周期比 DccNetwork 长,且在另一个线程,这里存在潜在的线程安全风险(访问正在析构的对象)。
  • 改进建议: 如果没有特殊原因,建议移除析构函数中的 disconnect 调用,或者直接移除该析构函数(使用编译器默认生成的即可)。

2. Q_GLOBAL_STATIC 配合 deleteLater 存在潜在的死锁或崩溃风险

  • 文件: networkcontroller.cpp
  • 代码:
    Q_GLOBAL_STATIC(QMutex, s_networkControllerMutex)
    
    void NetworkController::free()
    {
        QMutexLocker locker(s_networkControllerMutex());
        if (m_networkController) {
            m_networkController->deleteLater();
            m_networkController = nullptr;
        }
    }
  • 问题: Q_GLOBAL_STATIC 创建的互斥锁在程序退出(atexit 阶段)时会被销毁。如果 free() 是在主事件循环停止后被调用(例如全局静态变量的析构函数中),deleteLater 将不会被执行,导致内存泄漏;更严重的是,如果在 Q_GLOBAL_STATIC 销毁后还有代码尝试调用 instance()free()s_networkControllerMutex() 会触发断言失败导致程序崩溃。
  • 改进建议:
    • 如果 NetworkController 的生命周期贯穿整个应用,建议直接使用 Q_GLOBAL_STATIC 包装 NetworkController 本身,省去手动 new/free 的麻烦。
    • 如果必须手动管理,建议使用 Q_BASIC_ATOMIC_POINTER 配合 QMutex,或者确保 free() 在主事件循环退出前被显式调用,并改用 delete 同步释放。

二、 代码质量

1. m_manager 的空指针检查不够彻底

  • 文件: dccnetwork.cpp
  • 代码:
    NetItem *DccNetwork::root() const
    {
        return m_manager ? m_manager->root() : nullptr;
    }
  • 问题: 这里增加了对 m_manager 的空指针检查,说明 m_manager 有可能为空。但是在构造函数中:
    QMetaObject::invokeMethod(this, "init", Qt::QueuedConnection);
    init 是异步执行的,这意味着在 init 执行前,m_manager 尚未初始化。如果在构造函数和 init 调用之间有其他代码访问了 root(),会返回 nullptr。调用方是否有对 nullptr 的防范?
  • 改进建议: 检查所有调用 root() 的地方,确保它们能正确处理返回 nullptr 的情况。同时,检查 DccNetwork 的其他方法是否也需要增加对 m_manager 的空指针保护。

2. 析构逻辑中 disconnect() 过于粗暴

  • 文件: netmanagerthreadprivate.cpp
  • 代码:
    NetManagerThreadPrivate::~NetManagerThreadPrivate()
    {
        // 先断开所有信号,防止析构期间再有新任务(如singleShot)入队
        disconnect();
        // ...
    }
  • 问题: 无参的 disconnect() 会断开该对象所有的信号槽连接,包括与其他对象的连接。这可能会掩盖设计上的问题,如果在析构期间还有其他对象向该对象发送信号,说明对象的生命周期管理存在逻辑漏洞。
  • 改进建议: 建议精确断开特定的信号,或者确保在对象析构前,外部已经停止向其发送信号。如果只是为了防止定时器触发,只需确保 m_vpnStateUpdateTimer->stop() 即可。

三、 代码性能

1. 线程等待时间过长可能阻塞主线程

  • 文件: netmanagerthreadprivate.cpp
  • 代码:
    m_thread->wait(QDeadlineTimer(1000));
    if (m_thread->isRunning()) {
        m_thread->terminate();
    }
    m_thread->wait(QDeadlineTimer(500));
  • 问题: 析构时最多会阻塞当前线程 1500ms(1.5秒)。如果 NetManagerThreadPrivate 是在主线程中析构的,这会导致 UI 冻结 1.5 秒,用户体验极差。
  • 改进建议:
    • 优化工作线程的退出逻辑,确保线程能够快速响应 quit() 并退出事件循环。
    • 如果确实需要等待,考虑将超时时间缩短(例如 200ms 正常退出等待,200ms terminate 等待)。
    • 如果是耗时的清理工作,考虑使用异步析构或单独的后台清理线程。

四、 代码安全

1. terminate() 终止线程是极度危险的操作

  • 文件: netmanagerthreadprivate.cpp
  • 代码:
    if (m_thread->isRunning()) {
        m_thread->terminate();
    }
  • 问题: QThread::terminate() 会在任意点强制终止线程,这可能导致互斥锁死锁、内存泄漏、文件描述符泄漏或数据损坏。Qt 官方文档强烈建议不要使用 terminate()
  • 改进建议:
    • 检查为什么线程在 1000ms 内无法正常退出。是因为有死循环?还是因为阻塞在某个同步 IO 操作上?
    • 在工作线程的循环或长耗时任务中增加 QThread::currentThread()->isInterruptionRequested() 检查,配合 requestInterruption() 实现优雅退出。
    • 如果必须强行停止,考虑使用 QThread::wait() 无限期等待,或者将模块设计为独立进程,通过杀进程的方式清理,避免影响主进程的稳定性。

总结

此次 Diff 的核心亮点是QTimer::singleShot 改为了成员变量 QTimer 并设置 setSingleShot(true),这是一个非常好的防抖优化,避免了高频信号导致定时器堆积。

主要风险点在于线程的强制终止 (terminate)Q_GLOBAL_STATICdeleteLater 的混用。建议优先解决这两个隐患,以提升程序的长期稳定性和安全性。

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.

2 participants