Skip to content

fix: add null check for connection update#513

Merged
caixr23 merged 1 commit intolinuxdeepin:masterfrom
caixr23:master
Mar 5, 2026
Merged

fix: add null check for connection update#513
caixr23 merged 1 commit intolinuxdeepin:masterfrom
caixr23:master

Conversation

@caixr23
Copy link
Contributor

@caixr23 caixr23 commented Mar 4, 2026

Added null pointer check before updating network connection settings to prevent potential crashes when the connection object is not found. This ensures the application handles missing connections gracefully by logging a warning and returning early instead of attempting to update a non-existent connection.

Log: Fixed potential crash when updating non-existent network connections

Influence:

  1. Test network connection updates with valid and invalid UUIDs
  2. Verify warning messages are logged for missing connections
  3. Confirm application stability when connection objects are not found
  4. Test that valid connection updates continue to work normally

fix: 添加连接更新前的空指针检查

在更新网络连接设置前添加空指针检查,防止在连接对象未找到时出现潜在崩溃。
这确保应用程序能够优雅地处理缺失的连接,通过记录警告信息并提前返回,而不
是尝试更新不存在的连接。

Log: 修复更新不存在的网络连接时可能出现的崩溃问题

Influence:

  1. 测试使用有效和无效UUID更新网络连接
  2. 验证缺失连接时是否正确记录警告信息
  3. 确认连接对象未找到时应用程序的稳定性
  4. 测试有效的连接更新功能是否正常工作

PMS: BUG-307037

Summary by Sourcery

Bug Fixes:

  • Prevent a potential crash when attempting to update a network connection that cannot be found by UUID by returning early after logging a warning.

@sourcery-ai
Copy link

sourcery-ai bot commented Mar 4, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adds a null check when updating an existing network connection so that if the connection lookup by UUID fails, the code logs a warning and returns early instead of dereferencing a null connection pointer.

Sequence diagram for updated network connection update with null check

sequenceDiagram
    participant NetManagerThreadPrivate
    participant NetworkManager
    participant Logger

    NetManagerThreadPrivate->>NetworkManager: findConnectionByUuid(uuid)
    NetworkManager-->>NetManagerThreadPrivate: connection_or_null
    alt connection is null
        NetManagerThreadPrivate->>Logger: qCWarning DNC, Update connection failed, error with uuid
        NetManagerThreadPrivate-->>NetManagerThreadPrivate: return
    else connection exists
        NetManagerThreadPrivate->>NetManagerThreadPrivate: finalSettings = settings.toMap()
        alt connection isUnsaved
            NetManagerThreadPrivate->>NetworkManager: updateUnsaved finalSettings
        else
            NetManagerThreadPrivate->>NetworkManager: update finalSettings
        end
    end
Loading

Flow diagram for null-checked connection update logic

flowchart TD
    A[doSetConnectInfo update existing connection] --> B[connection = findConnectionByUuid uuid]
    B --> C{connection is null}
    C -- Yes --> D[Log warning with uuid]
    D --> E[Return without updating]
    C -- No --> F[finalSettings = settings.toMap]
    F --> G{connection isUnsaved}
    G -- Yes --> H[connection.updateUnsaved finalSettings]
    G -- No --> I[connection.update finalSettings]
    H --> J[End]
    I --> J[End]
Loading

File-Level Changes

Change Details Files
Guard connection update path with a null check and warning log when connection lookup by UUID fails.
  • After finding the connection by UUID for an update, add a null check on the returned connection object
  • If the connection is null, log a warning including the UUID and return early from the method to avoid calling update on a null pointer
  • Leave the existing logic for building settings and performing update/updateUnsaved unchanged when a valid connection is found
net-view/operation/private/netmanagerthreadprivate.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@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.

Hey - I've left some high level feedback:

  • Consider making the warning log more informative by including both the requested connection id and the UUID (and possibly the network type) so that failures to find a connection can be correlated more easily in logs.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider making the warning log more informative by including both the requested connection id and the UUID (and possibly the network type) so that failures to find a connection can be correlated more easily in logs.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@caixr23 caixr23 force-pushed the master branch 3 times, most recently from 627e3b0 to aa692ed Compare March 4, 2026 08:56
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743, caixr23

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

Added null pointer check before updating network connection settings
to prevent potential crashes when the connection object is not found.
This ensures the application handles missing connections gracefully by
logging a warning and returning early instead of attempting to update a
non-existent connection.

Log: Fixed potential crash when updating non-existent network
connections

Influence:
1. Test network connection updates with valid and invalid UUIDs
2. Verify warning messages are logged for missing connections
3. Confirm application stability when connection objects are not found
4. Test that valid connection updates continue to work normally

fix: 添加连接更新前的空指针检查

在更新网络连接设置前添加空指针检查,防止在连接对象未找到时出现潜在崩溃。
这确保应用程序能够优雅地处理缺失的连接,通过记录警告信息并提前返回,而不
是尝试更新不存在的连接。

Log: 修复更新不存在的网络连接时可能出现的崩溃问题

Influence:
1. 测试使用有效和无效UUID更新网络连接
2. 验证缺失连接时是否正确记录警告信息
3. 确认连接对象未找到时应用程序的稳定性
4. 测试有效的连接更新功能是否正常工作

PMS: BUG-303389
@caixr23 caixr23 merged commit 7384cbf into linuxdeepin:master Mar 5, 2026
13 of 17 checks passed
@deepin-ci-robot
Copy link

deepin pr auto review

这段代码的修改主要是为了防止在更新网络连接时出现空指针解引用(Null Pointer Dereference)的问题。以下是对这段代码的详细审查意见:

1. 语法逻辑

现状
代码逻辑正确。在调用 connection 的方法之前,增加了 if (!connection) 的检查。如果 findConnectionByUuid 返回了空指针,代码会输出警告日志并直接返回,避免了后续对空指针的操作。

改进意见
逻辑是正确的,但可以考虑在返回前做一些清理工作(如果有分配的资源需要释放),或者调用者是否需要知道更新失败(例如通过返回值或信号)。目前直接 return 可能会让调用者误以为操作成功。

2. 代码质量

现状

  • 使用了 qCWarning 进行日志记录,符合 Qt 的日志规范。
  • 增加了必要的空指针检查,提高了代码的健壮性。

改进意见

  • 错误处理:目前函数返回类型是 void,无法告知调用者操作是否成功。如果调用者需要知道更新是否成功,建议修改函数签名,返回 bool 或枚举类型表示操作状态。
  • 日志信息:日志中包含了 UUID,便于排查问题,这是好的做法。

3. 代码性能

现状

  • findConnectionByUuid 可能是一个线性查找操作(取决于实现),如果连接数量很多,可能会有性能问题。但通常网络连接数量不会特别多,所以影响不大。

改进意见

  • 如果 findConnectionByUuid 是线性查找且连接数量可能很多,可以考虑优化数据结构(如使用 QHashQMap 存储连接,以 UUID 为键)。
  • 目前没有明显的性能问题。

4. 代码安全

现状

  • 增加了空指针检查,避免了潜在的空指针解引用风险。
  • 日志中记录了 UUID,但需要注意 UUID 是否包含敏感信息(通常 UUID 是公开的,问题不大)。

改进意见

  • 如果 settings->uuid() 返回的字符串可能为空或无效,建议在调用 findConnectionByUuid 之前检查 UUID 的有效性。
  • 如果 connection 是共享指针(如 QSharedPointer),可以更安全地管理资源。

5. 其他建议

  • 函数命名doSetConnectInfo 是一个私有方法,命名符合 Qt 的风格(do 前缀通常表示实际执行操作的方法)。
  • 代码注释:可以增加注释说明为什么需要检查 connection 是否为空(例如:// 防止连接已被移除的情况)。

改进后的代码示例

void NetManagerThreadPrivate::doSetConnectInfo(const QString &id, NetType::NetItem type, const NetworkManager::ConnectionSettings::Ptr &settings)
{
    if (!settings || settings->uuid().isEmpty()) {
        qCWarning(DNC) << "Invalid settings or UUID";
        return;
    }

    if (type == NetType::Available) {
        // 新增连接的逻辑...
    } else {
        // 更新
        connection = findConnectionByUuid(settings->uuid());
        if (!connection) {
            qCWarning(DNC) << "Update connection failed: connection not found for UUID" << settings->uuid();
            return;
        }
        NMVariantMapMap finalSettings = settings->toMap();
        QDBusPendingReply<> reply = connection->isUnsaved() ? connection->updateUnsaved(finalSettings) : connection->update(finalSettings);
    }
}

总结

这段代码的修改是正确的,有效避免了空指针解引用的风险。建议进一步优化错误处理和日志记录,以提高代码的健壮性和可维护性。

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.

3 participants