-
Notifications
You must be signed in to change notification settings - Fork 607
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
Conversation
🔗 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 JobAs of commit b4b35e2 with merge base 5ef38d3 ( CANCELLED JOB - The following job was cancelled. Please retry:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This pull request was exported from Phabricator. Differential Revision: D75544079 |
Summary: Got a fuzzer error around overflow here Differential Revision: D75544079
434c823
to
5d3972e
Compare
This pull request was exported from Phabricator. Differential Revision: D75544079 |
Summary: Pull Request resolved: pytorch#11217 Got a fuzzer error around overflow here Differential Revision: D75544079
5d3972e
to
32eef53
Compare
Summary: Got a fuzzer error around overflow here Differential Revision: D75544079
32eef53
to
2daf3a0
Compare
This pull request was exported from Phabricator. Differential Revision: D75544079 |
Summary: Pull Request resolved: pytorch#11217 Got a fuzzer error around overflow here Differential Revision: D75544079
2daf3a0
to
9831a5f
Compare
Summary: Got a fuzzer error around overflow here Differential Revision: D75544079
9831a5f
to
7351b90
Compare
There was a problem hiding this 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?
executorch/runtime/core/tensor_layout.cpp
Line 19 in 14c3b31
Result<size_t> calculate_nbytes( |
@JacobSzwejbka has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
3f3b171
to
aca6a6d
Compare
Summary: Got a fuzzer error around overflow here Reviewed By: lucylq Differential Revision: D75544079 Pulled By: JacobSzwejbka
This pull request was exported from Phabricator. Differential Revision: D75544079 |
Summary: Got a fuzzer error around overflow here Reviewed By: lucylq Differential Revision: D75544079 Pulled By: JacobSzwejbka
aca6a6d
to
1c3dcd0
Compare
This pull request was exported from Phabricator. Differential Revision: D75544079 |
Summary: Got a fuzzer error around overflow here Reviewed By: lucylq Differential Revision: D75544079 Pulled By: JacobSzwejbka
1c3dcd0
to
e0b8b5e
Compare
Summary: Got a fuzzer error around overflow here Reviewed By: lucylq Differential Revision: D75544079 Pulled By: JacobSzwejbka
e8c8ad9
to
0ba6715
Compare
This pull request was exported from Phabricator. Differential Revision: D75544079 |
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
0ba6715
to
83e4f30
Compare
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
83e4f30
to
f96f246
Compare
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
f96f246
to
0b65bb0
Compare
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
0b65bb0
to
aecdbbc
Compare
This pull request was exported from Phabricator. Differential Revision: D75544079 |
1 similar comment
This pull request was exported from Phabricator. Differential Revision: D75544079 |
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
aecdbbc
to
83f6030
Compare
// Check for overflow | ||
ET_CHECK(sizes[i] == 0 || n / sizes[i] == prev_n); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah. yes...
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
83f6030
to
bbbf9b3
Compare
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
bbbf9b3
to
656d28e
Compare
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
656d28e
to
179b1f3
Compare
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
179b1f3
to
b4b35e2
Compare
This pull request was exported from Phabricator. Differential Revision: D75544079 |
Summary: Got a fuzzer error around overflow here
Differential Revision: D75544079