test(editor): fix heap-buffer-overflow crash in UT_Editwrapper_loadContent - #509
Merged
deepin-bot[bot] merged 1 commit intoAug 7, 2026
Conversation
…ntent
UT_Editwrapper_loadContent_001/002 stubbed QString::length to a large
value (41MB/10KB) in order to drive EditWrapper::loadContent into its
large-file and instant-open branches. This was both ineffective and
harmful:
- loadContent() reads the content size from strContent.length(), and
strContent is a QByteArray, so stubbing QString::length never affected
the branch selection.
- QString::length() is an out-of-line symbol in libQt5Core, so the stub
globally corrupted every QString::length() call in the process.
QDebug::operator<<(const QString&) (qdebug.h) calls t.length(), which
made qDebug read far past the string buffer. This triggered an ASan
heap-buffer-overflow (READ of size 12) and aborted the whole test
process when DDropdownMenu::setText() printed the endline menu text
("Unix") via BottomBar::setEndlineMenuText() -> setCurrentTextOnly()).
Fix: stub QByteArray::length instead -- the real type of strContent, so
loadContent() now takes the intended large-file / instant-open branches.
This is safe because qDebug prints a QByteArray via size() (not length())
and the actual data reads in loadContent()/customEvent() use size()/d->size
rather than length(), so no out-of-bounds read is introduced.
Verified with an ASan+UBSan build:
UT_Editwrapper_loadContent.UT_Editwrapper_loadContent_001 -> OK
UT_Editwrapper_loadContent.UT_Editwrapper_loadContent_002 -> OK
No heap-buffer-overflow; the 4 other pre-existing UT_Editwrapper failures
are unchanged and unrelated to this crash.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @pengfeixx, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the EditWrapper loadContent unit tests to stub QByteArray::length instead of QString::length, preventing a global corruption of QString length calls that led to an ASan-detected heap-buffer-overflow, and documents the rationale in-code. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review★ 总体评分：100分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // 当前代码已为最佳实践，无需进一步修改，以下为当前正确实现的重申
// loadContent() determines the file size via `strContent.length()` where
// strContent is a QByteArray, so the size must be faked on QByteArray, not
// QString. Stubbing QString::length globally corrupts every QString in the
// process: QDebug::operator<<(const QString&) (qdebug.h) calls t.length(),
// which then makes qDebug read past the string buffer and triggers a
// heap-buffer-overflow (e.g. when DDropdownMenu::setText prints the endline
// text). QByteArray::length is safe to stub here because qDebug prints a
// QByteArray through size() and the actual data reads in loadContent use
// size()/d->size rather than length().
Stub s1;
s1.set(ADDR(QByteArray,length),retintstub);
intvalue = 41*1024*1024;
wra->loadContent("ddd"); |
lzwind
approved these changes
Aug 7, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, pengfeixx The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
/merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Running the deepin-editor unit tests with ASan+UBSan aborted the whole test process at
UT_Editwrapper_loadContent.UT_Editwrapper_loadContent_001:Root cause
UT_Editwrapper_loadContent_001/002installed a global stub that madeQString::length()return a very large value (41 * 1024 * 1024/10 * 1024):This was both ineffective and harmful:
EditWrapper::loadContent()decides the branch fromint len = strContent.length();, andstrContentis aQByteArray. StubbingQString::lengththerefore never influenced the branch selection —lenalways stayed the real size of"ddd".QString::length()is an out-of-line symbol inlibQt5Core, so the stub globally corrupted everyQString::length()call in the whole process.QDebug::operator<<(const QString&)(qdebug.h:161) isputString(t.constData(), uint(t.length())), so whenDDropdownMenu::setText()later printed the endline menu text ("Unix") viaqDebug(), it tried to append41Mcharacters from a tiny string buffer, reading past the end of the heap allocation and aborting under ASan.Fix
Stub
QByteArray::lengthinstead — the real type ofstrContent:s1.set(ADDR(QByteArray,length),retintstub);This makes
loadContent()actually take the intended large-file / instant-open branches. It is safe because:qDebug()prints aQByteArraythroughsize()(notlength()), and prints aQStringthroughQString::length()which is no longer stubbed — so no out-of-bounds read is introduced.loadContent()/customEvent()usesize()/d->size(e.g.codec->toUnicode(..., text.size(), ...),QByteArray::mid()) rather thanlength(), so the large stubbed length only drives the loop/branch control flow, never a data copy.Verification
Built with the project's UT configuration (
-DCMAKE_BUILD_TYPE=Debug -DCMAKE_SAFETYTEST_ARG=CMAKE_SAFETYTEST_ARG_ON, which enables-fsanitize=undefined,address):No
heap-buffer-overflow, noABORTING. The fullUT_Editwrapper_*suite shows no regressions; the remainingUT_Editwrapper_saveFile_004/005,UT_Editwrapper_saveAsFile_003andUT_Editwrapper_handleFileLoadFinished_004_errorfailures are pre-existing (they already fail on master before this change) and unrelated to this crash.Summary by Sourcery
Adjust EditWrapper loadContent unit tests to stub the correct QByteArray length API and document the rationale to prevent sanitizer-detected crashes.
Bug Fixes:
Tests: