Skip to content

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

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

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 added type-bug An unexpected behavior, bug, or error needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Jul 19, 2022
@serhiy-storchaka serhiy-storchaka merged commit ebad53a into python:main Jul 28, 2022
@miss-islington
Copy link
Contributor

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 July 28, 2022 04:40
@miss-islington
Copy link
Contributor

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

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

@serhiy-storchaka serhiy-storchaka added needs backport to 3.11 only security fixes and removed needs backport to 3.11 only security fixes labels Jul 28, 2022
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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 pull request 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 <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 28, 2022
@bedevere-bot
Copy link

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 pull request 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 <[email protected]>
@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 28, 2022
serhiy-storchaka added a commit that referenced this pull request 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 <[email protected]>
serhiy-storchaka added a commit that referenced this pull request 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 <[email protected]>
def __eq__(self, other):
return False
def __hash__(self):
return str.__hash__(self)
Copy link
Member

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

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.

@serhiy-storchaka serhiy-storchaka removed their assignment Jul 25, 2023
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.

4 participants