Skip to content

Conversation

@lichaofan2008
Copy link
Contributor

@lichaofan2008 lichaofan2008 commented Jan 20, 2026

On the Zhaoxin processor, there is a probabilistic occurrence of null pointer/double-free issues with audio_buffers[i].data, so a NULL check needs to be added.
在兆芯处理器上,audio_buffers[i].data 概率性出现空指针/重复释放的情况,所以需要增加 NULL 判断。

Bug: https://pms.uniontech.com/bug-view-345713.html

v20 BUG 分支合一到v25主线
Task: https://pms.uniontech.com/task-view-383481.html

Summary by Sourcery

Bug Fixes:

  • Prevent null-pointer and double-free errors in audio_free_buffers() by skipping null entries and resetting freed buffer pointers to NULL.

On the Zhaoxin processor, there is a probabilistic occurrence of null pointer/double-free issues with `audio_buffers[i].data`, so a NULL check needs to be added.
在兆芯处理器上,audio_buffers[i].data 概率性出现空指针/重复释放的情况,所以需要增加 NULL 判断。

Bug: https://pms.uniontech.com/bug-view-345713.html

v20 BUG 分支合一到v25主线
Task: https://pms.uniontech.com/task-view-383481.html
@sourcery-ai
Copy link

sourcery-ai bot commented Jan 20, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adds defensive NULL handling when freeing audio buffers to avoid double-free and null-pointer issues on specific processors.

State diagram for audio_buffers[i].data pointer lifecycle after fix

stateDiagram-v2
    [*] --> Uninitialized

    Uninitialized --> Allocated: buffer_allocated
    Allocated --> Freed: audio_free_buffers free
    Freed --> Null: audio_free_buffers set_to_NULL

    Allocated --> Null: explicit_set_NULL
    Null --> [*]
Loading

File-Level Changes

Change Details Files
Harden audio_free_buffers() against NULL and double-free issues for buffer data pointers.
  • Add a NULL check for each audio_buffers[i].data before calling free()
  • Set audio_buffers[i].data to NULL immediately after freeing it to prevent subsequent double-free
  • Document in-code (Chinese comment) that the check is required due to probabilistic NULL/double-free issues on Zhaoxin processors
libcam/libcam_audio/audio.c

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

@deepin-ci-robot
Copy link

deepin pr auto review

这段代码修改主要是为了解决在特定硬件平台(兆芯处理器)上出现的内存管理问题。以下是对该 git diff 的详细审查意见,包括语法逻辑、代码质量、性能和安全性方面的分析:

1. 语法逻辑

  • 审查结果:通过。
  • 分析
    • 引入了 if (audio_buffers[i].data == NULL) 判断,逻辑上是合理的。在 C 语言中,标准规定对 NULL 指针调用 free() 是安全的(不会产生副作用),但这段代码显然是为了应对更复杂的场景(如“重复释放”或“内存已被破坏导致指针异常”)。
    • free 之后立即将指针置为 NULL (audio_buffers[i].data = NULL),这是防止“悬垂指针”的标准做法,逻辑正确。

2. 代码质量

  • 审查结果:良好,但有小幅提升空间。
  • 分析
    • 注释清晰:代码中明确添加了注释解释了修改原因(兆芯处理器上的概率性空指针/重复释放问题),这对于后续维护非常重要。
    • 防御性编程:增加了非空判断和置空操作,体现了防御性编程的思想,提高了代码的健壮性。
  • 改进建议
    • 考虑到 audio_buffers 本身也是一个指针,在循环结束后才 free(audio_buffers)。如果 audio_buffers 是全局变量或静态变量(看起来是静态的),且在 free(audio_buffers) 之后没有立即置为 NULL,那么如果其他地方有代码再次调用 audio_free_buffers,可能会导致 audio_buffers 变成悬垂指针,进而引发崩溃。
    • 建议:在函数末尾增加 audio_buffers = NULL;

3. 代码性能

  • 审查结果:影响极小,可忽略。
  • 分析
    • 增加了一次 if 判断和一次赋值操作。由于 AUDBUFF_NUM 通常是一个较小的常数(音频缓冲区数量),且此函数 audio_free_buffers 通常只在程序结束或关闭设备时调用(非热路径),因此性能损耗完全可以忽略不计。

4. 代码安全

  • 审查结果:显著提升。
  • 分析
    • 防止重复释放:这是本次修改的核心。虽然在标准 C 库中 free(NULL) 是安全的,但如果指针指向的内存已经被释放且未被置空,再次调用 free 将导致 Double Free(双重释放)漏洞,可能引发程序崩溃或被攻击利用。将指针置为 NULL 彻底杜绝了这种情况。
    • 防止悬垂指针:释放后置空,防止了后续代码误用已释放的内存。
    • 应对硬件异常:注释提到兆芯处理器上概率性出现空指针,这可能是由于编译器优化、缓存一致性或硬件层面的内存对齐问题导致的。显式的 NULL 检查和置空增加了代码的确定性,有助于规避此类底层硬件兼容性问题。

总结与建议修改后的代码

这段修改是非常必要的,主要解决了内存安全和特定硬件兼容性问题。为了进一步提高代码质量,建议补充对 audio_buffers 自身的置空操作。

建议优化后的代码片段:

static void audio_free_buffers()
{
	int i;

	// 增加 audio_buffers 自身的非空判断,防止函数被重复调用时崩溃
	if (audio_buffers == NULL) {
		return;
	}

	for(i = 0; i < AUDBUFF_NUM; ++i)
	{
		// 在兆芯处理器上,audio_buffers[i].data 概率性出现空指针/重复释放的情况,所以这里需要增加判断
		if (audio_buffers[i].data != NULL) {
			free(audio_buffers[i].data);
			audio_buffers[i].data = NULL;
		}
	}

	free(audio_buffers);
	
	// 修复:释放主指针后立即置空,防止产生悬垂指针
	audio_buffers = NULL;
}

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:

  • Instead of tying the comment to a specific processor, consider rephrasing it in generic terms (e.g., explaining that audio_free_buffers may be called multiple times and we guard against double free / NULL) so the rationale is clear and future-proof.
  • If audio_free_buffers can be called concurrently, this change alone may not fully prevent double-free or race issues; you might want to confirm and, if needed, guard calls with appropriate synchronization rather than relying solely on a NULL check.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Instead of tying the comment to a specific processor, consider rephrasing it in generic terms (e.g., explaining that audio_free_buffers may be called multiple times and we guard against double free / NULL) so the rationale is clear and future-proof.
- If audio_free_buffers can be called concurrently, this change alone may not fully prevent double-free or race issues; you might want to confirm and, if needed, guard calls with appropriate synchronization rather than relying solely on a NULL check.

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.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lichaofan2008, lzwind

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

@lichaofan2008
Copy link
Contributor Author

/forcemerge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jan 20, 2026

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit e0aa6bf into linuxdeepin:master Jan 20, 2026
16 of 18 checks passed
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