Add out of bounds checks inside irparser.cpp and unpickler.cpp #91401
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/91401
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit ba70472: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
Thanks for the fix! This looks good to me, I'll let CI run |
|
@pytorchbot rebase -s |
|
@pytorchbot successfully started a rebase job. Check the current status here |
|
Successfully rebased |
pytorchmergebot
force-pushed
the
check-size-before-using-containers
branch
from
7497bd8 to
d0cf2ee
Compare
| @@ -501,6 +501,13 @@ void IRParser::parseOperator(Block* b) { | |||
| for (const VarWithType& v : outs) { | |||
| vmap[v.name] = n->outputs()[idx]; | |||
| if (schema && !schema->is_varret()) { | |||
| TORCH_CHECK( | |||
There was a problem hiding this comment.
curious, why do we need this check?
I understand the pop() issue... but doesn't .at() already check for out of bounds? (i.e. this is why you would use .at() instead of [])?
There was a problem hiding this comment.
Yep, that's true.
However, I thought about this issue from the user's point of view. I'd say it's mostly a quality-of-life improvement that helps to clearly show where and why the error occurs. There are many places, where using .at() is enough, however, I think in this context it's beneficial.
|
@pytorchbot rebase -s |
|
@pytorchbot successfully started a rebase job. Check the current status here |
|
Successfully rebased |
pytorchmergebot
force-pushed
the
check-size-before-using-containers
branch
from
d0cf2ee to
b733091
Compare
|
@pytorchbot rebase -s |
|
@pytorchbot successfully started a rebase job. Check the current status here |
|
Successfully rebased |
pytorchmergebot
force-pushed
the
check-size-before-using-containers
branch
from
b733091 to
ba70472
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 few crashes.
Inside unpickler.cpp/irparser.cpp there are a few places, where
.at()and.pop_back()are called before checking target container size. Lack of these checks results in an attempt to access elements oob (in case of.at()), and an actual out-of-bounds access while calling.pop_back()/.pop()on astack_variable.Crash-files:
Crash location:
unpickler.cpp:439(Call to.at(idx)with idx that exceedsmemo_table_size)./message_deserialize_fuzz /homedir/crash-5695ad5b2921127775d4137ee02e23834a0bedc4Crash location:
irparser.cpp:504(Call to.at(idx)with idx that exceedsschema->returns()size)./irparser_fuzz /homedir/crash-779ecab3d637c8c87de21e23dddb9def82a26792Crash location:
unpickler.cpp:451(Call to.pop_back()with emptystack_)./message_deserialize_fuzz /homedir/crash-735acc19c9f39b9bbb5667878af995c9167da37fCrash location:
unpickler.cpp:469(Call to.pop()with emptystack_)./message_deserialize_fuzz /homedir/crash-b552f1a2bbba5eab0f6aeba58475175b18e5b1b9The provided patch adds missing size checks.
How to reproduce
To reproduce the crashes, 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/bashAnd execute fuzz-targets with the given arguments
After execution completes you will see ASAN reports.