-
Notifications
You must be signed in to change notification settings - Fork 22.4k
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
Add size check before calling stack_.at(dict_pos) in unpickler.cpp #94300
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/94300
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ✅ No FailuresAs of commit cbfc10f: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
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.
Thank you very much for the fix. Do you mind adding unit test to test_jit.py
that attempts to deserialize an hand crafted jit script.
I don't think it is possible to craft a good testcase by hand. In this example, fuzzing harness mainly targets code related to serialization/deserialization of rpc requests/responses. It's quite hard to generate a similar testcase that reaches the problematic code path using, for example, pickled model loading |
@malfet |
@eellison, can we merge this PR that fixes crash in JIT? |
@ezyang, can we merge this approved PR? |
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: This PR is too stale; the last push date was more than 3 days ago. Please rebase and try again. You can rebase and merge by leaving the following comment on this PR: Details for Dev Infra teamRaised by workflow job |
@pytorchbot rebase |
@pytorchbot successfully started a rebase job. Check the current status here |
Successfully rebased |
88ad76f
to
cbfc10f
Compare
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Hi!
I've been fuzzing different pytorch modules, and found a crash inside one of them.
Specifically, I'm talking about a module for unpickling and a function called
Unpickler::readInstruction()
. Running this function with provided crash file results in a crash, which occurs while callingauto dict = stack_.at(dict_pos).toGenericDict();
unpickler.cpp:561. The crash occurs, because the indexdict_pos
is out of bounds (which itself happens because the stack size is 0).Besides this pull-request, there is another one related to unpickler hardening: #84343
All tests were performed on this pytorch version: abc54f93145830b502400faa92bec86e05422fbd
How to reproduce
To reproduce the crash, use provided docker: Dockerfile
Build the container:
docker build -t oss-sydr-fuzz-pytorch-reproduce .
Copy crash file to the current directory:
Run the container:
docker run --privileged --network host -v `pwd`:/homedir --rm -it oss-sydr-fuzz-pytorch-reproduce /bin/bash
And execute the binary:
/message_deserialize_sydr /homedir/crash-042dff5e121580425d9d34d0f293918f3c9fbf1e
After execution completes you will see this error message:
And this stacktrace: