Skip to content
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

gh-94938: Fix errror detection of unexpected keyword arguments #94999

Merged

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Jul 19, 2022

When keyword argument name is an instance of a str subclass with
overloaded methods __eq__ and __hash__, the former code could not find
the name of an extraneous keyword argument to report an error, and
_PyArg_UnpackKeywords() returned success without setting the
corresponding cell in linearized arguments array. But since the number
of expected initialized cells is determined as total number of passed
arguments, this lead to reading NULL as keyword parameter value, that
caused SystemError or crash or other undesired behavior.

When keyword argument name is an instance of a str subclass with
overloaded methods __eq__ and __hash__, the former code could not find
the name of an extraneous keyword argument to report an error, and
_PyArg_UnpackKeywords() returned success without setting the
corresponding cell in linearized arguments array. But since the number
of expected initialized cells is determined as total number of passed
arguments, this lead to reading NULL as keyword parameter value, that
caused SysTemError or crash or other undesired behavior.
@serhiy-storchaka serhiy-storchaka merged commit ebad53a into python:main Jul 28, 2022
14 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jul 28, 2022

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒🤖 I'm not a witch! I'm not a witch!

@serhiy-storchaka serhiy-storchaka deleted the error-unexpected-keyword-arg branch Jul 28, 2022
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jul 28, 2022

Sorry @serhiy-storchaka, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker ebad53a4dc1bb591820724a22cef9b8459185b5f 3.11

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jul 28, 2022

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker ebad53a4dc1bb591820724a22cef9b8459185b5f 3.10

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jul 28, 2022

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jul 28, 2022

Sorry @serhiy-storchaka, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker ebad53a4dc1bb591820724a22cef9b8459185b5f 3.11

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 28, 2022
…uments (pythonGH-94999)

When keyword argument name is an instance of a str subclass with
overloaded methods __eq__ and __hash__, the former code could not find
the name of an extraneous keyword argument to report an error, and
_PyArg_UnpackKeywords() returned success without setting the
corresponding cell in the linearized arguments array. But since the number
of expected initialized cells is determined as the total number of passed
arguments, this lead to reading NULL as a keyword parameter value, that
caused SystemError or crash or other undesired behavior.
(cherry picked from commit ebad53a)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jul 28, 2022

GH-95353 is a backport of this pull request to the 3.11 branch.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 28, 2022
…uments (pythonGH-94999)

When keyword argument name is an instance of a str subclass with
overloaded methods __eq__ and __hash__, the former code could not find
the name of an extraneous keyword argument to report an error, and
_PyArg_UnpackKeywords() returned success without setting the
corresponding cell in the linearized arguments array. But since the number
of expected initialized cells is determined as the total number of passed
arguments, this lead to reading NULL as a keyword parameter value, that
caused SystemError or crash or other undesired behavior..
(cherry picked from commit ebad53a)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jul 28, 2022

GH-95354 is a backport of this pull request to the 3.10 branch.

serhiy-storchaka added a commit that referenced this issue Jul 28, 2022
…GH-94999) (GH-95353)

When keyword argument name is an instance of a str subclass with
overloaded methods __eq__ and __hash__, the former code could not find
the name of an extraneous keyword argument to report an error, and
_PyArg_UnpackKeywords() returned success without setting the
corresponding cell in the linearized arguments array. But since the number
of expected initialized cells is determined as the total number of passed
arguments, this lead to reading NULL as a keyword parameter value, that
caused SystemError or crash or other undesired behavior.
(cherry picked from commit ebad53a)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit that referenced this issue Jul 28, 2022
…GH-94999) (GH-95354)

When keyword argument name is an instance of a str subclass with
overloaded methods __eq__ and __hash__, the former code could not find
the name of an extraneous keyword argument to report an error, and
_PyArg_UnpackKeywords() returned success without setting the
corresponding cell in the linearized arguments array. But since the number
of expected initialized cells is determined as the total number of passed
arguments, this lead to reading NULL as a keyword parameter value, that
caused SystemError or crash or other undesired behavior..
(cherry picked from commit ebad53a)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
def __eq__(self, other):
return False
def __hash__(self):
return str.__hash__(self)
Copy link
Member

@zware zware Jul 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a little odd; aren't these latter __eq__ and __hash__ definitions rendering the first ones pointless? Which set was supposed to be in this class?

Copy link
Member Author

@serhiy-storchaka serhiy-storchaka Jul 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. The first definitions are correct, they cause a crash with non-patched code. The latter definitions do not cause any visible effects, Thank you, Zachary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants