Fix write-heap-buffer-overflow in et_copy_index #15605
Merged
Conversation
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
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/15605
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (4 Unrelated Failures)As of commit 1768563 with merge base b005f10 ( BROKEN TRUNK - The following jobs failed but were 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. |
meta-cla
bot
added
the
CLA Signed
Closed
This PR needs a
|
lucylq
added a commit
to lucylq/executorch-1
that referenced
this pull request
Nov 5, 2025
Summary: The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: `ET_CHECK_MSG(index >= 0, "Index must be non-negative");` and `ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds");`. These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. Additionally, reviewers may want to consider testing the patch with various inputs, including negative `index` values, `index` values that exceed the bounds of `copy_to`, and valid `index` values, to ensure that the patch correctly prevents the write-heap-buffer-overflow crash. Here is the commit message: ``` Fix write-heap-buffer-overflow crash in et_copy_index The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: ```cpp ET_CHECK_MSG(index >= 0, "Index must be non-negative"); ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds"); ``` These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. ``` NOTE: This diff is entirely auto-generated by LLM-based patch generator. Reviewer should carefully examine this diff as Lionhead does not guarrantee the correctnesss of the patch beyond fixing the crash and passing existing tests. Please commandeer this diff and revise as needed. Our bot does not respond to comments or revision requests (yet). Differential Revision: D80399111
lucylq
force-pushed
the
export-D80399111
branch
from
0cce3cc to
1816e48
Compare
kernels/prim_ops/et_copy_index.cpp
Outdated
| auto copy_from = (*stack[1]).toTensor(); | ||
| auto index = (*stack[2]).toInt(); | ||
|
|
||
| ET_CHECK_MSG(index >= 0, "Index must be non-negative"); |
Contributor
There was a problem hiding this comment.
Do non fatal checks by setting the error state in the KernelRuntimeContext and then returning. I think theres a macro to do that if you poke around the op libs
Contributor
Author
There was a problem hiding this comment.
Updated all the ET_CHECK_MSG to ET_KERNEL_CHECK_MSG
lucylq
added a commit
to lucylq/executorch-1
that referenced
this pull request
Nov 5, 2025
Summary: The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: `ET_CHECK_MSG(index >= 0, "Index must be non-negative");` and `ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds");`. These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. Additionally, reviewers may want to consider testing the patch with various inputs, including negative `index` values, `index` values that exceed the bounds of `copy_to`, and valid `index` values, to ensure that the patch correctly prevents the write-heap-buffer-overflow crash. Here is the commit message: ``` Fix write-heap-buffer-overflow crash in et_copy_index The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: ```cpp ET_CHECK_MSG(index >= 0, "Index must be non-negative"); ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds"); ``` These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. ``` NOTE: This diff is entirely auto-generated by LLM-based patch generator. Reviewer should carefully examine this diff as Lionhead does not guarrantee the correctnesss of the patch beyond fixing the crash and passing existing tests. Please commandeer this diff and revise as needed. Our bot does not respond to comments or revision requests (yet). Differential Revision: D80399111
lucylq
force-pushed
the
export-D80399111
branch
from
1816e48 to
941bf1d
Compare
lucylq
added a commit
to lucylq/executorch-1
that referenced
this pull request
Nov 5, 2025
Summary: The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: `ET_CHECK_MSG(index >= 0, "Index must be non-negative");` and `ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds");`. These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. Additionally, reviewers may want to consider testing the patch with various inputs, including negative `index` values, `index` values that exceed the bounds of `copy_to`, and valid `index` values, to ensure that the patch correctly prevents the write-heap-buffer-overflow crash. Here is the commit message: ``` Fix write-heap-buffer-overflow crash in et_copy_index The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: ```cpp ET_CHECK_MSG(index >= 0, "Index must be non-negative"); ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds"); ``` These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. ``` NOTE: This diff is entirely auto-generated by LLM-based patch generator. Reviewer should carefully examine this diff as Lionhead does not guarrantee the correctnesss of the patch beyond fixing the crash and passing existing tests. Please commandeer this diff and revise as needed. Our bot does not respond to comments or revision requests (yet). Differential Revision: D80399111
lucylq
force-pushed
the
export-D80399111
branch
from
941bf1d to
5640ebe
Compare
lucylq
added a commit
to lucylq/executorch-1
that referenced
this pull request
Nov 5, 2025
Summary: 1. Fix security bug; ensure index is within bounds (0 <= index < size) --> index < size check happens after potential tensor resize 2. Convert ET_CHECK_MSG --> ET_KERNEL_CHECK_MSG for error reporting --- The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: `ET_CHECK_MSG(index >= 0, "Index must be non-negative");` and `ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds");`. These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. Additionally, reviewers may want to consider testing the patch with various inputs, including negative `index` values, `index` values that exceed the bounds of `copy_to`, and valid `index` values, to ensure that the patch correctly prevents the write-heap-buffer-overflow crash. Here is the commit message: ``` Fix write-heap-buffer-overflow crash in et_copy_index The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: ```cpp ET_CHECK_MSG(index >= 0, "Index must be non-negative"); ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds"); ``` These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. ``` NOTE: This diff is entirely auto-generated by LLM-based patch generator. Reviewer should carefully examine this diff as Lionhead does not guarrantee the correctnesss of the patch beyond fixing the crash and passing existing tests. Please commandeer this diff and revise as needed. Our bot does not respond to comments or revision requests (yet). Differential Revision: D80399111
lucylq
force-pushed
the
export-D80399111
branch
from
5640ebe to
428cc06
Compare
lucylq
added a commit
to lucylq/executorch-1
that referenced
this pull request
Nov 5, 2025
Summary: 1. Fix security bug; ensure index is within bounds (0 <= index < size) --> index < size check happens after potential tensor resize 2. Convert ET_CHECK_MSG --> ET_KERNEL_CHECK_MSG for error reporting --- The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: `ET_CHECK_MSG(index >= 0, "Index must be non-negative");` and `ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds");`. These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. Additionally, reviewers may want to consider testing the patch with various inputs, including negative `index` values, `index` values that exceed the bounds of `copy_to`, and valid `index` values, to ensure that the patch correctly prevents the write-heap-buffer-overflow crash. Here is the commit message: ``` Fix write-heap-buffer-overflow crash in et_copy_index The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: ```cpp ET_CHECK_MSG(index >= 0, "Index must be non-negative"); ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds"); ``` These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. ``` NOTE: This diff is entirely auto-generated by LLM-based patch generator. Reviewer should carefully examine this diff as Lionhead does not guarrantee the correctnesss of the patch beyond fixing the crash and passing existing tests. Please commandeer this diff and revise as needed. Our bot does not respond to comments or revision requests (yet). Differential Revision: D80399111
lucylq
force-pushed
the
export-D80399111
branch
from
428cc06 to
867aa97
Compare
JacobSzwejbka
approved these changes
Nov 11, 2025
lucylq
added a commit
to lucylq/executorch-1
that referenced
this pull request
Nov 11, 2025
Summary: 1. Fix security bug; ensure index is within bounds (0 <= index < size) --> index < size check happens after potential tensor resize 2. Convert ET_CHECK_MSG --> ET_KERNEL_CHECK_MSG for error reporting --- The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: `ET_CHECK_MSG(index >= 0, "Index must be non-negative");` and `ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds");`. These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. Additionally, reviewers may want to consider testing the patch with various inputs, including negative `index` values, `index` values that exceed the bounds of `copy_to`, and valid `index` values, to ensure that the patch correctly prevents the write-heap-buffer-overflow crash. Here is the commit message: ``` Fix write-heap-buffer-overflow crash in et_copy_index The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: ```cpp ET_CHECK_MSG(index >= 0, "Index must be non-negative"); ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds"); ``` These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. ``` NOTE: This diff is entirely auto-generated by LLM-based patch generator. Reviewer should carefully examine this diff as Lionhead does not guarrantee the correctnesss of the patch beyond fixing the crash and passing existing tests. Please commandeer this diff and revise as needed. Our bot does not respond to comments or revision requests (yet). Reviewed By: JacobSzwejbka Differential Revision: D80399111
lucylq
force-pushed
the
export-D80399111
branch
from
867aa97 to
3c89b67
Compare
Summary: 1. Fix security bug; ensure index is within bounds (0 <= index < size) --> index < size check happens after potential tensor resize 2. Convert ET_CHECK_MSG --> ET_KERNEL_CHECK_MSG for error reporting --- The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: `ET_CHECK_MSG(index >= 0, "Index must be non-negative");` and `ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds");`. These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. Additionally, reviewers may want to consider testing the patch with various inputs, including negative `index` values, `index` values that exceed the bounds of `copy_to`, and valid `index` values, to ensure that the patch correctly prevents the write-heap-buffer-overflow crash. Here is the commit message: ``` Fix write-heap-buffer-overflow crash in et_copy_index The crash is a write-heap-buffer-overflow that occurs in the `et_copy_index` function. The root cause is the lack of proper validation of the `index` argument, which can lead to an out-of-bounds write when `index` is negative or exceeds the bounds of the `copy_to` tensor. The patch fixes the crash by adding two checks: ```cpp ET_CHECK_MSG(index >= 0, "Index must be non-negative"); ET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds"); ``` These checks ensure that `index` is within the valid range for the `copy_to` tensor, preventing the out-of-bounds write. Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when `index` is equal to `copy_to.sizes()[0] - 1`. Reviewers should also check that the patch does not alter the existing functionality of the `et_copy_index` function and that it is consistent with the surrounding code. ``` NOTE: This diff is entirely auto-generated by LLM-based patch generator. Reviewer should carefully examine this diff as Lionhead does not guarrantee the correctnesss of the patch beyond fixing the crash and passing existing tests. Please commandeer this diff and revise as needed. Our bot does not respond to comments or revision requests (yet). Reviewed By: JacobSzwejbka Differential Revision: D80399111
lucylq
force-pushed
the
export-D80399111
branch
from
3c89b67 to
1768563
Compare
Contributor
Author
|
@pytorchbot cherry-pick --onto release/1.0 -c critical |
Collaborator
Cherry picking #15605The cherry pick PR is at #15782 and it is recommended to link a critical cherry pick PR with an issue. The following tracker issues are updated: Details for Dev Infra teamRaised by workflow job |
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.
Summary:
--> index < size check happens after potential tensor resize
The crash is a write-heap-buffer-overflow that occurs in the
et_copy_indexfunction. The root cause is the lack of proper validation of theindexargument, which can lead to an out-of-bounds write whenindexis negative or exceeds the bounds of thecopy_totensor.The patch fixes the crash by adding two checks:
ET_CHECK_MSG(index >= 0, "Index must be non-negative");andET_CHECK_MSG(index < copy_to.sizes()[0], "Index out of bounds");. These checks ensure thatindexis within the valid range for thecopy_totensor, preventing the out-of-bounds write.Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when
indexis equal tocopy_to.sizes()[0] - 1. Reviewers should also check that the patch does not alter the existing functionality of theet_copy_indexfunction and that it is consistent with the surrounding code.Additionally, reviewers may want to consider testing the patch with various inputs, including negative
indexvalues,indexvalues that exceed the bounds ofcopy_to, and validindexvalues, to ensure that the patch correctly prevents the write-heap-buffer-overflow crash.Here is the commit message:
These checks ensure that
indexis within the valid range for thecopy_totensor, preventing the out-of-bounds write.Other considerations that reviewers should take into account when validating the patch include verifying that the added checks do not introduce any performance regressions and that they correctly handle edge cases, such as when
indexis equal tocopy_to.sizes()[0] - 1. Reviewers should also check that the patch does not alter the existing functionality of theet_copy_indexfunction and that it is consistent with the surrounding code.