[pytorch] Fix pushLong() issue in pickler. #28057
Conversation
LONG4 length should be a 4-byte integer, per: https://github.com/python/cpython/blob/2.7/Lib/pickletools.py#L677 Document also seems to indicate that it should be signed. Also, support on the unpickle side. Differential Revision: [D17934174](https://our.internmc.facebook.com/intern/diff/D17934174/) [ghstack-poisoned]
facebook-github-bot
added
the
oncall: jit
LONG4 length should be a 4-byte integer, per: https://github.com/python/cpython/blob/2.7/Lib/pickletools.py#L677 Document also seems to indicate that it should be signed. Also, support on the unpickle side. Differential Revision: [D17934174](https://our.internmc.facebook.com/intern/diff/D17934174/) ghstack-source-id: 91957163 Pull Request resolved: #28057
zdevito
left a comment
zdevito
left a comment
There was a problem hiding this comment.
LONG4 is suppose to first read 4 bytes to determine the size of the data to read and then read that many bytes. I am not sure what our implementation does (either before or after the patch). LONG1 looks more correct, though even that only supports numbers that are precisely 8 bytes. @driazati for what is suppose to happen here. I think we should just delete LONG4 handling, and assert in LONG1 handling that length == 8 on write (currently we only do it on read).
|
Thanks for the feedback. |
|
|
|
In any case, closing this specific PR. |
LONG4 length should be a 4-byte integer, per: https://github.com/python/cpython/blob/2.7/Lib/pickletools.py#L677 Document also seems to indicate that it should be signed. Also, support on the unpickle side. Differential Revision: [D17934174](https://our.internmc.facebook.com/intern/diff/D17934174/) [ghstack-poisoned]
jjlilley
changed the title
jjlilley
changed the title
For pushLong() in Pickler, it looks like we only use for a single use case, with a 10-byte value. We were handling > 256 bytes incorrectly, by using a LONG4 opcode (expecting 4-byte length), but pushing 8 bytes. We could harden this handling, but rather than improve codepaths that we never expect to use, this change simply removes the incorrect codepath and adds and assert. Differential Revision: [D17934174](https://our.internmc.facebook.com/intern/diff/D17934174/) [ghstack-poisoned]
Pull Request resolved: #28057 LONG4 length should be a 4-byte integer, per: https://github.com/python/cpython/blob/2.7/Lib/pickletools.py#L677 Document also seems to indicate that it should be signed. Also, support on the unpickle side. ghstack-source-id: 92038641 Differential Revision: [D17934174](https://our.internmc.facebook.com/intern/diff/D17934174/)
That sounds reasonable, uploading a version that simply removes the incorrect codepath and asserts that the size fits in 1 byte. |
torch/csrc/jit/pickler.cpp
Outdated
| } | ||
| TORCH_INTERNAL_ASSERT( | ||
| size <= std::numeric_limits<uint8_t>::max(), | ||
| "Cannot pickle a long with a size larger than 256 bytes"); |
There was a problem hiding this comment.
This should say Cannot pickle a long with a size larger than 1 byte
There was a problem hiding this comment.
I can change this message, but given that data.size() can be up to 256, saying that the long's size can't be larger than 1 byte doesn't seem accurate.
I could say the more-awkward-but-accurate:
"Cannot pickle a long with the size of size is larger than 1 byte"
Or, is there less-awkward-but-also-accurate preferred version of the assert text?
There was a problem hiding this comment.
Maybe just Cannot pickle a long larger than 255 bytes (0 is a valid size, 256 is not)
For pushLong() in Pickler, it looks like we only use for a single use case, with a 10-byte value. We were handling > 256 bytes incorrectly, by using a LONG4 opcode (expecting 4-byte length), but pushing 8 bytes. We could harden this handling, but rather than improve codepaths that we never expect to use, this change simply removes the incorrect codepath and adds and assert. Differential Revision: [D17934174](https://our.internmc.facebook.com/intern/diff/D17934174/) [ghstack-poisoned]
Pull Request resolved: #28057 For pushLong() in Pickler, it looks like we only use for a single use case, with a 10-byte value. We were handling > 256 bytes incorrectly, by using a LONG4 opcode (expecting 4-byte length), but pushing 8 bytes. We could harden this handling, but rather than improve codepaths that we never expect to use, this change simply removes the incorrect codepath and adds and assert. ghstack-source-id: 92048325 Differential Revision: [D17934174](https://our.internmc.facebook.com/intern/diff/D17934174/)
|
This pull request has been merged in ff00e8c. |
Summary: Pull Request resolved: pytorch#28057 For pushLong() in Pickler, it looks like we only use for a single use case, with a 10-byte value. We were handling > 256 bytes incorrectly, by using a LONG4 opcode (expecting 4-byte length), but pushing 8 bytes. We could harden this handling, but rather than improve codepaths that we never expect to use, this change simply removes the incorrect codepath and adds and assert. ghstack-source-id: 92048325 Test Plan: buck test mode/dev-nosan caffe2/test/... Differential Revision: D17934174 fbshipit-source-id: ecc1ca37dbcc87151fc5bf2ffb6b05dff91d3667
Stack from ghstack:
For pushLong() in Pickler, it looks like we only use for a single use case, with a 10-byte value.
We were handling > 256 bytes incorrectly, by using a LONG4 opcode (expecting 4-byte length), but pushing 8 bytes. We could harden this handling, but rather than improve codepaths that we never expect to use, this change simply removes the incorrect codepath and adds and assert.
Differential Revision: D17934174