Skip to content

check for overflow in calculate_nbytes #11217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 4, 2025

Conversation

JacobSzwejbka
Copy link
Contributor

Summary: Got a fuzzer error around overflow here

Differential Revision: D75544079

Copy link

pytorch-bot bot commented May 29, 2025

🔗 Helpful Links

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

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

❌ 1 Cancelled Job

As of commit b4b35e2 with merge base 5ef38d3 (image):

CANCELLED JOB - The following job was cancelled. Please retry:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels May 29, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75544079

JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request May 29, 2025
Summary:

Got a fuzzer error around overflow here

Differential Revision: D75544079
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75544079

JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request May 29, 2025
Summary:
Pull Request resolved: pytorch#11217

Got a fuzzer error around overflow here

Differential Revision: D75544079
JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request May 29, 2025
Summary:

Got a fuzzer error around overflow here

Differential Revision: D75544079
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75544079

JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request May 29, 2025
Summary:
Pull Request resolved: pytorch#11217

Got a fuzzer error around overflow here

Differential Revision: D75544079
JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request May 29, 2025
Summary:

Got a fuzzer error around overflow here

Differential Revision: D75544079
Copy link
Contributor

@lucylq lucylq left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! Could you also make the same changes to tensor_layout?

Result<size_t> calculate_nbytes(

@facebook-github-bot
Copy link
Contributor

@JacobSzwejbka has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@JacobSzwejbka JacobSzwejbka added the release notes: none Do not include this in the release notes label May 30, 2025
JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request May 30, 2025
Summary:
Got a fuzzer error around overflow here


Reviewed By: lucylq

Differential Revision: D75544079

Pulled By: JacobSzwejbka
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75544079

JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request May 30, 2025
Summary:
Got a fuzzer error around overflow here


Reviewed By: lucylq

Differential Revision: D75544079

Pulled By: JacobSzwejbka
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75544079

JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request May 30, 2025
Summary:
Got a fuzzer error around overflow here


Reviewed By: lucylq

Differential Revision: D75544079

Pulled By: JacobSzwejbka
JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request May 30, 2025
Summary:
Got a fuzzer error around overflow here


Reviewed By: lucylq

Differential Revision: D75544079

Pulled By: JacobSzwejbka
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75544079

JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request Jun 2, 2025
Summary:
Got a fuzzer error around overflow here

Pull Request resolved: pytorch#11217

Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Reviewed By: lucylq

Differential Revision: D75544079

Pulled By: JacobSzwejbka
JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request Jun 3, 2025
Summary:
Got a fuzzer error around overflow here


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Reviewed By: lucylq

Differential Revision: D75544079

Pulled By: JacobSzwejbka
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75544079

JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request Jun 3, 2025
Summary:
Got a fuzzer error around overflow here


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Reviewed By: lucylq

Differential Revision: D75544079

Pulled By: JacobSzwejbka
JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request Jun 3, 2025
Summary:
Got a fuzzer error around overflow here


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Reviewed By: lucylq

Differential Revision: D75544079

Pulled By: JacobSzwejbka
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75544079

1 similar comment
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75544079

JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request Jun 3, 2025
Summary:
Got a fuzzer error around overflow here

Pull Request resolved: pytorch#11217

Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Reviewed By: lucylq

Differential Revision: D75544079

Pulled By: JacobSzwejbka
Comment on lines +63 to +64
// Check for overflow
ET_CHECK(sizes[i] == 0 || n / sizes[i] == prev_n);
Copy link
Contributor

Choose a reason for hiding this comment

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

is it necessary to perform a division here? could we check n >= prev_n instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah. yes...

Copy link
Contributor Author

@JacobSzwejbka JacobSzwejbka Jun 3, 2025

Choose a reason for hiding this comment

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

Ah ok no it doesnt work. I can give a counter example.

To get the numbers smaller lets say Im accumulating in uint16_t and Im taking the product of int8_t::max

n: 127 prev_n: 1

n: 16129 prev_n: 127

n: 16767 prev_n: 16129 (overflowed at 65535 from logical value 2048383)

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm. I'm tempted to ask if we should just import c10/util/safe_numerics.h (which, note to self, requires llvmMathExtra.h) instead of doing division then. it is annoying that it only has 64-bit safe_multiplies functions, but we could add a size_t-based one (in executorch for now if you don't want to wait for pin bump, followed by adding it in pytorch, getting a pin bump, and deleting the executorch one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request Jun 3, 2025
Summary:
Got a fuzzer error around overflow here


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Reviewed By: lucylq

Differential Revision: D75544079

Pulled By: JacobSzwejbka
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75544079

JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request Jun 3, 2025
Summary:
Got a fuzzer error around overflow here


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Reviewed By: lucylq

Differential Revision: D75544079

Pulled By: JacobSzwejbka
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75544079

JacobSzwejbka added a commit to JacobSzwejbka/executorch-1 that referenced this pull request Jun 3, 2025
Summary:
Got a fuzzer error around overflow here


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Reviewed By: lucylq

Differential Revision: D75544079

Pulled By: JacobSzwejbka
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75544079

Summary:
Got a fuzzer error around overflow here


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Reviewed By: lucylq

Differential Revision: D75544079

Pulled By: JacobSzwejbka
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D75544079

@lucylq lucylq merged commit 93b1a0c into pytorch:main Jun 4, 2025
96 of 98 checks passed
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 release notes: none Do not include this in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants