-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
bpo-37502: handle default parameter for buffers argument of pickle.loads #14593
bpo-37502: handle default parameter for buffers argument of pickle.loads #14593
Conversation
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
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.
@mmohrhard Thanks for the contribution! For verification of the bug described in https://bugs.python.org/issue37502, I ran pickle.loads(pickle.dumps(1, protocol=pickle.HIGHEST_PROTOCOL), buffers=None)
in the interpreter after installing the latest version of python:master
:
To test this PR, I installed the branch mmohrhard:buffer-default-in-pickle-loads
and ran the same code as above in the interpreter:
As can be seen from the screenshots above, TypeError
is no longer raised when buffers is set to None
.
Edit: I would recommend replacing the news blurb to be a bit more descriptive, to specify the exception.
Currently it is:
Fix error when buffer argument of pickle.loads is None
I would recommend changing the phrasing to:
pickle.loads() no longer raises TypeError when the buffers argument is set to None
a411622
to
9ac140a
Compare
@aeros167 Thanks. I have updated the patch set with your suggest text for the news blurb and squashed the commits. Please let me know if there is anything else that I should do. |
@@ -1653,7 +1653,7 @@ _Unpickler_SetInputEncoding(UnpicklerObject *self, | |||
static int | |||
_Unpickler_SetBuffers(UnpicklerObject *self, PyObject *buffers) | |||
{ | |||
if (buffers == NULL) { | |||
if (buffers == NULL || buffers == Py_None) { |
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.
Propagating down 2 special values meaning "no buffers" is ugly. Can we
normalize the value when we parse the arguments, so the rest of the code
have exactly one value for "no buffers"?
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.
Thanks for spotting this. This looks good to me.
Thanks @mmohrhard for the PR, and @pitrou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
GH-14948 is a backport of this pull request to the 3.8 branch. |
…ads correctly (pythonGH-14593) (cherry picked from commit 898318b) Co-authored-by: Markus Mohrhard <markus.mohrhard@googlemail.com>
https://bugs.python.org/issue37502