bpo-41200: Add pickle.loads fuzz test#21289
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). Recognized GitHub usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
|
@ned-deily OK, I modified the bpo number. What else should I do? |
|
You have to actually open an issue on bugs.python.org and use the number assigned there. |
There was a problem hiding this comment.
I will comment on this PR, but bear in mind I don't know much or anything about our fuzzing infrastructure.
(but I happen to maintain the fuzzing infrastructure on another project)
I'm curious: did you try to run the fuzzer for some time? pickle has never meant to be strengthened against corrupt input, so my intuition is that you may get a steady stream of issues reported.
There's something else: to fuzz efficiently, it is important to start from a corpus of well-known (valid or invalid) input. Is it possible with our fuzzing infrastructure? If so, I would recommend adding a bunch of valid pickles in the seed corpus. Otherwise, the fuzzer will try to produce pickles entirely at random without knowing anything about their structure, and the inputs produced will not be very "interesting".
| if (parsed == NULL && ( | ||
| PyErr_ExceptionMatches(pickle_error) || | ||
| PyErr_ExceptionMatches(pickling_error) || | ||
| PyErr_ExceptionMatches(unpickling_error) |
There was a problem hiding this comment.
Why not simply PyErr_ExceptionMatches(PyExc_Exception)? We don't really care which exact exception is raised.
There was a problem hiding this comment.
other fuzzers in this module also work like this, but if we're going to list exceptions to ignore, we should at least add comments as to why for each or at least each class.
I assume this being a pickle.loads test is really looking for process crashes.
unfortunately... I expect the results of this as a fuzzer to be pretty bad, pickle is explicitly documented as not being suitable for untrusted data. it may execute arbitrary code or crash in that situation. i asked this broader question on the bug.
|
cc @ammaraskar and @ssbr since they seem to be knowledgeable on the topic. |
|
Side question: does it make sense to compile a single fuzzer for all different functionalities (struct, csv, re, pickle...)? Or would it be better to have separate fuzzers? |
|
Thanks for the ping Antoine. I think long term I'd like to move the fuzzers to their own files since their initialization/fuzzing logic is getting a bit spaghetti-like in this one large file. However, they do need to be compiled into separate binaries for oss-fuzz to pick them up. Since they provide us with free resources with fuzzing, I think it's important to keep our fuzzers focused on security-critical components where bugs are likely to have high impact throughout the Python ecosystem. In terms of this pull request, I initially did not add in a @gpshead Do you have any opinions on this? |
|
@xinali, please sign the CLA. Thank you! |
|
Ok, I have signed the CLA. |
add
pickle.loads(x)fuzz testhttps://bugs.python.org/issue41200
https://bugs.python.org/issue41200