Integer overflow in PlatformMemoryAllocator::allocate()#18680
Integer overflow in PlatformMemoryAllocator::allocate()#18680
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18680
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 3 Unrelated FailuresAs of commit 0f1f47a with merge base ee92757 ( NEW FAILURE - The following job has failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following jobs failed but was present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
…UTORCH-26) Add overflow checking before computing the total allocation size (sizeof(AllocationNode) + size + alignment) in PlatformMemoryAllocator::allocate(). Previously, when this sum exceeded SIZE_MAX, it would wrap around to a small value, causing pal_allocate to allocate an undersized buffer. This could lead to subsequent out-of-bounds writes. The fix validates each addition step against SIZE_MAX and returns nullptr on overflow. This PR was authored with the assistance of Claude.
There was a problem hiding this comment.
Pull request overview
This PR fixes a potential integer overflow in PlatformMemoryAllocator::allocate() by validating size additions before calculating the total allocation size, preventing undersized allocations that could lead to out-of-bounds writes.
Changes:
- Add overflow-checked computation for
alloc_sizeusingc10::add_overflows. - Log and return
nullptrwhen an overflow is detected during allocation size calculation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include <cinttypes> | ||
| #include <cstdint> | ||
|
|
||
| #include <c10/util/safe_numerics.h> |
There was a problem hiding this comment.
<c10/util/safe_numerics.h> is already included by executorch/runtime/core/memory_allocator.h, so this direct include is redundant here. Consider removing it to keep header dependencies minimal and reduce rebuild churn.
| #include <c10/util/safe_numerics.h> |
| // Then allocate enough memory for node, data and the alignment bump. | ||
| size_t alloc_size = 0; | ||
| if (c10::add_overflows(sizeof(AllocationNode), size, &alloc_size) || | ||
| c10::add_overflows(alloc_size, alignment, &alloc_size)) { |
There was a problem hiding this comment.
The allocation “alignment bump” only needs to reserve up to alignment - 1 bytes. Using + alignment slightly over-allocates and can also make the overflow check reject an allocation that would otherwise fit by 1 byte. Consider adding alignment - 1 (after validating alignment > 0, which isPowerOf2 already implies) instead of alignment.
| // Then allocate enough memory for node, data and the alignment bump. | |
| size_t alloc_size = 0; | |
| if (c10::add_overflows(sizeof(AllocationNode), size, &alloc_size) || | |
| c10::add_overflows(alloc_size, alignment, &alloc_size)) { | |
| // Then allocate enough memory for node, data and the maximum alignment | |
| // bump, which is at most alignment - 1 bytes. | |
| size_t alloc_size = 0; | |
| if (c10::add_overflows(sizeof(AllocationNode), size, &alloc_size) || | |
| c10::add_overflows(alloc_size, alignment - 1, &alloc_size)) { |
Add overflow checking before computing the total allocation size (sizeof(AllocationNode) + size + alignment) in PlatformMemoryAllocator::allocate().
Previously, when this sum exceeded SIZE_MAX, it would wrap around to a small value, causing pal_allocate to allocate an undersized buffer. This could lead to subsequent out-of-bounds writes. The fix validates each addition step against SIZE_MAX and returns nullptr on overflow.
This PR was authored with the assistance of Claude.
Test plan