Skip to content

Add safe_numel()#19074

Merged
meta-codesync[bot] merged 13 commits intomainfrom
gh/lucylq/154/head
Apr 25, 2026
Merged

Add safe_numel()#19074
meta-codesync[bot] merged 13 commits intomainfrom
gh/lucylq/154/head

Conversation

@lucylq
Copy link
Copy Markdown
Contributor

@lucylq lucylq commented Apr 23, 2026

Stack from ghstack (oldest at bottom):

  • safe_numel(sizes, dim): returns Result; use this where possible

Currently, we have compute_numel, which silently overflows.

Authored with Claude.

Differential Revision: D102070375

Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)

[ghstack-poisoned]
@lucylq lucylq requested a review from JacobSzwejbka as a code owner April 23, 2026 17:38
@pytorch-bot
Copy link
Copy Markdown

pytorch-bot Bot commented Apr 23, 2026

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19074

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

⏳ 1 Pending, 2 Unrelated Failures

As of commit ae06ce8 with merge base eef7921 (image):

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 meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 23, 2026
@github-actions
Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Copy link
Copy Markdown
Contributor

@JacobSzwejbka JacobSzwejbka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please swap to just using ET_CHECK(safe_numel()) like we talked about)

Github Executorch added 4 commits April 24, 2026 10:56
…w()"

Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)

[ghstack-poisoned]
Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)

[ghstack-poisoned]
…w()"

Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)

[ghstack-poisoned]
Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)

[ghstack-poisoned]
@lucylq lucylq changed the title Add safe_numel() and compute_numel_overflow() Add safe_numel() Apr 24, 2026
Github Executorch added 2 commits April 24, 2026 13:33
Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)

[ghstack-poisoned]
Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)

[ghstack-poisoned]
@lucylq lucylq changed the base branch from gh/lucylq/154/base to main April 24, 2026 20:33
Github Executorch added 4 commits April 24, 2026 13:47
Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)

[ghstack-poisoned]
Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)

[ghstack-poisoned]
Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)

[ghstack-poisoned]
Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)

[ghstack-poisoned]
lucylq pushed a commit that referenced this pull request Apr 24, 2026
Pull Request resolved: #19074

Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.
ghstack-source-id: 372628309
@exported-using-ghexport

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)
Github Executorch added 2 commits April 24, 2026 15:17
Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)

[ghstack-poisoned]
Introduces two overflow-checked numel helpers as experimental API additions.

- safe_numel(sizes, dim): returns Result; use this where possible
- compute_numel_overflow(sizes, dim): aborts on overflow; use this in ctors and other places that cannot return result. Long-term change is to migrate fully to safe_numel.

Currently, we have compute_numel, which silently overflows.




Authored with Claude.

Differential Revision: [D102070375](https://our.internmc.facebook.com/intern/diff/D102070375/)

[ghstack-poisoned]
Copilot AI review requested due to automatic review settings April 24, 2026 22:17
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new overflow-checked safe_numel() helper to compute tensor element counts without silent integer overflow, and wires it into both portable (executor) and ATen-mode type shims so callers can begin migrating to checked numel computations.

Changes:

  • Introduce safe_numel() returning Result<ssize_t> for portable tensors (TensorImpl::SizesType).
  • Add an equivalent safe_numel() inline helper in exec_aten.h for ATen mode.
  • Update exec_aten Buck target deps to export core runtime headers needed by the new API surface.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

File Description
runtime/core/portable_type/tensor_impl.h Declares safe_numel() in the portable tensor API surface and re-exports it via torch::executor aliases.
runtime/core/portable_type/tensor_impl.cpp Implements overflow-checked safe_numel() for portable tensors using c10::mul_overflows.
runtime/core/exec_aten/targets.bzl Adds exported deps so exec_aten.h can include error.h / result.h and related core headers.
runtime/core/exec_aten/exec_aten.h Adds an ATen-mode inline safe_numel() returning Result<ssize_t>.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread runtime/core/exec_aten/exec_aten.h
Comment on lines +62 to +68
ssize_t next_numel;
ET_CHECK_OR_RETURN_ERROR(
!c10::mul_overflows(numel, static_cast<ssize_t>(sizes[i]), &next_numel),
InvalidArgument,
"Overflow computing numel at dimension %zd",
i);
numel = next_numel;
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safe_numel() uses c10::mul_overflows() with signed ssize_t. In our portable_type c10 implementation, the MSVC fallback path computes volatile T tmp = a * b for signed T, which can itself trigger signed-overflow UB when the product doesn’t fit. This undermines the overflow checking and can be exploitable for attacker-controlled shapes on MSVC builds. Prefer an overflow check that avoids signed multiplication (e.g., since sizes are validated non-negative, use a size_t/uint64_t accumulator with if (a > SSIZE_MAX / b) bounds checking, or use an unsigned safe-multiply helper and then range-check back to ssize_t).

Copilot uses AI. Check for mistakes.
Comment on lines +50 to +55
ET_CHECK_OR_RETURN_ERROR(
dim == 0 || sizes != nullptr,
InvalidArgument,
"Sizes must be provided for non-scalar tensors");
ssize_t numel = 1;
for (const auto i : c10::irange(dim)) {
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safe_numel() doesn’t reject negative dim. With c10::irange(dim), a negative dim produces an empty range, so the function returns 1 instead of an error. Since this helper is intended for validating potentially malformed/untrusted shape metadata, it should return InvalidArgument when dim < 0 (before the loop).

Copilot uses AI. Check for mistakes.
Comment on lines +47 to +71
::executorch::runtime::Result<ssize_t> safe_numel(
const TensorImpl::SizesType* sizes,
ssize_t dim) {
ET_CHECK_OR_RETURN_ERROR(
dim == 0 || sizes != nullptr,
InvalidArgument,
"Sizes must be provided for non-scalar tensors");
ssize_t numel = 1;
for (const auto i : c10::irange(dim)) {
ET_CHECK_OR_RETURN_ERROR(
sizes[i] >= 0,
InvalidArgument,
"Size must be non-negative, got %zd at dimension %zd",
static_cast<ssize_t>(sizes[i]),
i);
ssize_t next_numel;
ET_CHECK_OR_RETURN_ERROR(
!c10::mul_overflows(numel, static_cast<ssize_t>(sizes[i]), &next_numel),
InvalidArgument,
"Overflow computing numel at dimension %zd",
i);
numel = next_numel;
}
return numel;
}
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are existing portable_type tensor_impl unit tests, but safe_numel() introduces new behavior (overflow detection, negative sizes, null sizes for dim>0). Please add focused tests covering: scalar (dim==0, sizes==nullptr), negative dim, negative size, and an overflow case near SSIZE_MAX to ensure the new checks behave as expected.

Copilot uses AI. Check for mistakes.
Comment thread runtime/core/portable_type/tensor_impl.h
Comment thread runtime/core/portable_type/tensor_impl.h
Comment on lines +120 to +125
ET_CHECK_OR_RETURN_ERROR(
dim == 0 || sizes != nullptr,
InvalidArgument,
"Sizes must be provided for non-scalar tensors");
ssize_t numel = 1;
for (ssize_t i = 0; i < dim; i++) {
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exec_aten::safe_numel() doesn’t reject negative dim. For malformed metadata, a negative dim will skip the loop and return 1, which is indistinguishable from a scalar. Consider explicitly returning InvalidArgument when dim < 0 (in addition to the existing sizes!=nullptr check for dim>0).

Copilot uses AI. Check for mistakes.
Comment on lines +126 to +135
ET_CHECK_OR_RETURN_ERROR(
sizes[i] >= 0,
InvalidArgument,
"Size must be non-negative, got %zd at dimension %zd",
static_cast<ssize_t>(sizes[i]),
i);
ssize_t next_numel;
ET_CHECK_OR_RETURN_ERROR(
!c10::mul_overflows(numel, static_cast<ssize_t>(sizes[i]), &next_numel),
InvalidArgument,
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ATen mode, SizesType is int64_t but safe_numel() casts each size to ssize_t before checking overflow. On platforms where ssize_t is narrower than int64_t (or if a size is larger than SSIZE_MAX), this cast is implementation-defined and can truncate/flip sign, potentially letting invalid shapes slip through or producing incorrect results. Add an explicit range check that sizes[i] <= SSIZE_MAX (and >= 0) before converting, or do the multiplication in a wider unsigned type and validate the final result fits in ssize_t.

Copilot uses AI. Check for mistakes.
@meta-codesync meta-codesync Bot merged commit 222711e into main Apr 25, 2026
178 of 182 checks passed
@meta-codesync meta-codesync Bot deleted the gh/lucylq/154/head branch April 25, 2026 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants