fix memory overflow #5169
Closed
fix memory overflow #5169
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
Contributor
Summary: X-link: facebookresearch/FBGEMM#2166 In edge cases, the weight input_stride and output_stride could be different. In this case, out will move based on out_stride but copy data with size input_stride. If input_stride is larger than output_stride, the memcpy could overflow. this fix guarantees the safe memcpy, and is backward compatible for cases that input_stride <= output_stride. Differential Revision: D87734295
emlin
force-pushed
the
export-D87734295
branch
from
c3c7ec1 to
4f07e08
Compare
q10
added a commit
to q10/FBGEMM
that referenced
this pull request
Nov 26, 2025
…and input_stride > output_stride (pytorch#5169) Summary: X-link: facebookresearch/FBGEMM#2170 X-link: facebookresearch/FBGEMM#2166 In edge cases, the weight input_stride and output_stride could be different. In this case, out will move based on out_stride but copy data with size input_stride. If input_stride is larger than output_stride, the memcpy could overflow. this fix guarantees the safe memcpy, and is backward compatible for cases that input_stride <= output_stride. ==== The context is S592128, where the predictor crashes every so often bc of bad input being provided to memcpy, which causes memory overflow. The issues show up when nobag=true, OutType=uint8_t, and input_stride > output_stride. From what I can tell, if input_stride and output_stride are not provided to us (i.e. a -1 is passed), we compute it ourselves. But it is possible for users to have users pass in custom-sized tables to us, where input_stride is greater than output_stride. The code already has an assert beforehand to check for this case, but apparently, assert() can be disabled when -DNDEBUG is provided during compilation, which is probably what is being passed in when building the package and why we see the crash. Emma provided the initial patch, which is to copy just the min(input_stride, output_stride) amount of buffer per row. It provides backward compatibility with existing behavior, and just adjusts it so that the corner case doesnt crash the predictor. I have added the unit test to the patch, based on the existing EmbeddingSpMDMTest.basicTest. I initially tried to fit it into EmbeddingSpMDMTest.basicTest, but figured it would be better to have a standalone test bc it is testing a corner case, not adding more coordinates to the test matrix. Reviewed By: emlin, spcyppt Differential Revision: D87734295
q10
added a commit
to q10/FBGEMM
that referenced
this pull request
Nov 26, 2025
…and input_stride > output_stride (pytorch#5169) Summary: X-link: facebookresearch/FBGEMM#2170 X-link: facebookresearch/FBGEMM#2166 In edge cases, the weight input_stride and output_stride could be different. In this case, out will move based on out_stride but copy data with size input_stride. If input_stride is larger than output_stride, the memcpy could overflow. this fix guarantees the safe memcpy, and is backward compatible for cases that input_stride <= output_stride. ==== The context is S592128, where the predictor crashes every so often bc of bad input being provided to memcpy, which causes memory overflow. The issues show up when nobag=true, OutType=uint8_t, and input_stride > output_stride. From what I can tell, if input_stride and output_stride are not provided to us (i.e. a -1 is passed), we compute it ourselves. But it is possible for users to have users pass in custom-sized tables to us, where input_stride is greater than output_stride. The code already has an assert beforehand to check for this case, but apparently, assert() can be disabled when -DNDEBUG is provided during compilation, which is probably what is being passed in when building the package and why we see the crash. Emma provided the initial patch, which is to copy just the min(input_stride, output_stride) amount of buffer per row. It provides backward compatibility with existing behavior, and just adjusts it so that the corner case doesnt crash the predictor. I have added the unit test to the patch, based on the existing EmbeddingSpMDMTest.basicTest. I initially tried to fit it into EmbeddingSpMDMTest.basicTest, but figured it would be better to have a standalone test bc it is testing a corner case, not adding more coordinates to the test matrix. Reviewed By: emlin, spcyppt Differential Revision: D87734295
Contributor
|
This pull request has been merged in c7910c4. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Differential Revision: D87734295