CHIPArgParser: fix heap buffer overflow in SplitArgs realloc#72256
Open
woody-apple wants to merge 1 commit into
Open
CHIPArgParser: fix heap buffer overflow in SplitArgs realloc#72256woody-apple wants to merge 1 commit into
woody-apple wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces type-aware heap allocation wrappers MemoryAllocTyped and MemoryCallocTyped to simplify heap allocations when the element type is known. The review feedback highlights a potential integer overflow vulnerability in MemoryAllocTyped when calculating the allocation size (sizeof(T) * count) without validation, and suggests adding an overflow check to prevent smaller-than-expected memory allocations.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #72256 +/- ##
==========================================
+ Coverage 55.52% 55.57% +0.04%
==========================================
Files 1629 1630 +1
Lines 111092 111137 +45
Branches 13415 13404 -11
==========================================
+ Hits 61683 61761 +78
+ Misses 49409 49376 -33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #72256: Size comparison from 8ad5f7b to 3fe7328 Full report (25 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32)
|
|
PR #72256: Size comparison from 8ad5f7b to 7f9d0e4 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
typed-alloc-wrappers
branch
from
7f9d0e4 to
687d7a2
Compare
|
PR #72256: Size comparison from 4894db9 to 687d7a2 Full report (31 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
typed-alloc-wrappers
branch
from
687d7a2 to
0b7e39d
Compare
|
PR #72256: Size comparison from 4894db9 to 0b7e39d Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
typed-alloc-wrappers
branch
from
0b7e39d to
2876164
Compare
|
PR #72256: Size comparison from 4894db9 to 2876164 Full report (21 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
woody-apple
force-pushed
the
typed-alloc-wrappers
branch
from
2876164 to
cc761c4
Compare
|
PR #72256: Size comparison from 8a162c6 to 4d319f6 Full report (21 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
woody-apple
force-pushed
the
typed-alloc-wrappers
branch
from
4d319f6 to
b1daa37
Compare
|
PR #72256: Size comparison from 8a162c6 to 0a01009 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
added a commit
to woody-apple/connectedhomeip
that referenced
this pull request
May 30, 2026
…d test rationale Address round-2 review consensus on PR project-chip#72256 (typed-alloc wrappers). Issue 1 (MAJOR — sizeof(T)==1 overflow guard hole): verified the kMemoryAllocTypedMaxBytes second-cap guard is already in the diff under review at CHIPMem.h:178/186/204. Updated test comments in TestMemAllocTyped_OneByteTypeMaxCountReturnsNull and TestMemCallocTyped_OverflowReturnsNull to state that the wrapper itself enforces the contract via kMemoryAllocTypedMaxBytes — not the underlying allocator. This removes the prior implication that the test passes only because libc happens to refuse SIZE_MAX, which would have been false under sanitizer-instrumented or embedded pool allocators. Issue 2 (MINOR — count==0 returns nullptr divergence): strengthened the doxygen block comment on the typed wrappers to flag the deliberate departure from MemoryAlloc(0) / MemoryCalloc(0,n) and from libc malloc(0) / calloc(0,n), all of which are implementation-defined and may return a non-null freeable pointer. Added TestMemAllocTyped_ZeroCountIsAlwaysNull to pin the divergence for both wrappers so future maintainers see the intent. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
woody-apple
force-pushed
the
typed-alloc-wrappers
branch
from
bd49d8a to
d863481
Compare
|
PR #72256: Size comparison from 8a162c6 to d863481 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
typed-alloc-wrappers
branch
from
d863481 to
d156132
Compare
|
PR #72256: Size comparison from 8a162c6 to d156132 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
changed the title
woody-apple
force-pushed
the
typed-alloc-wrappers
branch
from
d156132 to
5570462
Compare
|
PR #72256: Size comparison from 8a162c6 to 5570462 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
typed-alloc-wrappers
branch
2 times, most recently
from
d156132 to
9b6a34f
Compare
|
PR #72256: Size comparison from 8a162c6 to 9b6a34f Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
typed-alloc-wrappers
branch
from
9b6a34f to
a78917c
Compare
|
PR #72256: Size comparison from 8a162c6 to a78917c Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
Signed-off-by: Justin Wood <woody@apple.com>
woody-apple
force-pushed
the
typed-alloc-wrappers
branch
from
a78917c to
d1f7da6
Compare
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
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.
Summary
Fixes a heap buffer overflow in
SplitArgs()(src/lib/support/CHIPArgParser.cpp). This is a memory-corruption fix, not a no-op.SplitArgs()allocates an array ofchar *pointers starting atInitialArgListSize(10) and doubles when full. The doubling path passed the element count as the byte size toMemoryRealloc():MemoryRealloc's second argument is a byte count, so this under-allocated by a factor ofsizeof(char *)(8 on 64-bit). Once the arg list grew past 10 entries, a request for 20 pointers actually shrank the buffer to 20 bytes (~2 pointers). The nextargList[argCount++] = nextArgand the trailing NULL terminator then wrote out of bounds — heap corruption, caught by ASan in the no-shell-asan CI job, silent corruption otherwise. Reachable from any caller parsing more than 10 whitespace-separated tokens.Changes
sizeof(char *)so the realloc request is in bytes.size_t-overflow guard: the bareargListSize *= 2would wrap silently on pathological input and under-allocate again; on overflowSplitArgsnow returns-1(parse failure) instead of corrupting the heap. This branch is unreachable in practice given the starting size but makes the doubling provably safe.Testing
Three new regression tests in
TestCHIPArgParserdriveSplitArgspast theInitialArgListSizebucket across one and two doublings: pure non-option tokens, the exactn+1boundary, and a mix of options and args. All three fail against the pre-fix code (ASan abort / wrong argc) and pass with the fix. Existing arg-parser and memory unit tests continue to pass.Scope note
An earlier revision of this PR also added speculative
MemoryAllocTyped/MemoryCallocTypedwrappers inCHIPMem.hplus cosmetic call-site migrations (Darwin/ESP32 DNS-SD, the twoMemoryAllocsites in this file). Those have been dropped here so this PR is a self-contained, fast-trackable correctness fix. The typed-wrapper idea, if pursued, belongs in its own PR where its API surface andcount==0 -> nullptrsemantics can be debated on their own merits — it is not needed for this fix (the originalstatic_cast<char **>(MemoryRealloc(..., argListSize * sizeof(char *)))form fixes the bug).