Skip to content

Custom str subclass as keyword argument leads to System Error #94938

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

Closed
jeff5 opened this issue Jul 17, 2022 · 5 comments
Closed

Custom str subclass as keyword argument leads to System Error #94938

jeff5 opened this issue Jul 17, 2022 · 5 comments
Assignees
Labels
3.10 only security fixes 3.11 only security fixes 3.12 only security fixes topic-argument-clinic type-bug An unexpected behavior, bug, or error

Comments

@jeff5
Copy link
Contributor

jeff5 commented Jul 17, 2022

When arguments are given by keyword using a dictionary, CPython requires that the key be of type str, or a sub-class. If a custom str subclass defines a badly-behaved __eq__, and is used that way, a SystemError (null argument) can result, which is not expected in the interpreter.

This report stems from a discussion on discuss.python.org about the types of keyword that ought to be allowed -- it's not code I actually want to run.

Reproducer

# kwargsbomb.py Use of a str subclass as a keyword

class S(str):
    def __eq__(self, other):
        print(other)
        return True
    def __hash__(self):
        return 42

u = "my.domain.name"

a = u.split(**{'sep':'.'})     # ok
print(a)

b = u.split(**{S('sep'):'.'})  # ok
print(b)

c = u.split(**{S('xxx'):'.'})  # fails
print(c)

Output

['my', 'domain', 'name']
['my', 'domain', 'name']
sep
Traceback (most recent call last):
  File " ... \kwargsbomb.py", line 18, in <module>
    c = u.split(**{S('xxx'):'.'})  # fails
        ^^^^^^^^^^^^^^^^^^^^^^^^^
SystemError: null argument to internal routine

Environment

  • CPython versions: 3.10.2 3.11.0b4 (in Idle and at the prompt)
  • Operating system and architecture: Windows 10 64-bit
@jeff5 jeff5 added the type-bug An unexpected behavior, bug, or error label Jul 17, 2022
@JelleZijlstra
Copy link
Member

I'm not sure this should be supported. The discuss.python.org discussion was focused on strings that aren't valid identifiers, not on subclasses of str.

Restricting kwargs to only str may allow some optimizations and saves us from possible subtle bugs (what if a str subclass overrides __hash__). In addition, there's no known use case for using str subclasses.

I suggest we:

  • Document that kwargs must be exactly of type str
  • Fix the code so that attempting to use a str subclass raises TypeError instead of SystemError

@serhiy-storchaka
Copy link
Member

The user should not be able to provoke a SystemError from pure Python code. This exception is reserved for misuse of the C API. There is a bug somewhere in _PyArg_UnpackKeywords(). In other examples it can lead to crash, memory corruption, or just silent produce incorrect result.

@serhiy-storchaka serhiy-storchaka self-assigned this Jul 18, 2022
@serhiy-storchaka serhiy-storchaka added 3.11 only security fixes 3.10 only security fixes topic-argument-clinic 3.12 only security fixes labels Jul 18, 2022
@jeff5
Copy link
Contributor Author

jeff5 commented Jul 19, 2022

The discuss.python.org discussion was focused on strings that aren't valid identifiers, not on subclasses of str.

It starts with the question why integer keys are not allowed. When I looked where this is enforced, I was interested to find that it accepted sub-classes of str. So I tried it, idly, and idle restarted (in 3.8).

Restricting kwargs to only str may allow some optimizations and saves us from possible subtle bugs (what if a str subclass overrides __hash__).

I agree, and favour that, but I cannot say for certain no-one is relying on it. Also @serhiy-storchaka is right that something is wrong elsewhere, and may go wrong in other circumstances, which this daft trick just serves to discover.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue 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.
@serhiy-storchaka
Copy link
Member

Thank you for your report @jeff5. It was an interesting bug.

serhiy-storchaka added a commit that referenced this issue Jul 28, 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 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.
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 28, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 30, 2022
(cherry picked from commit 0956b6d)

Co-authored-by: Serhiy Storchaka <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 30, 2022
(cherry picked from commit 0956b6d)

Co-authored-by: Serhiy Storchaka <[email protected]>
miss-islington added a commit that referenced this issue Jul 30, 2022
(cherry picked from commit 0956b6d)

Co-authored-by: Serhiy Storchaka <[email protected]>
miss-islington added a commit that referenced this issue Jul 30, 2022
(cherry picked from commit 0956b6d)

Co-authored-by: Serhiy Storchaka <[email protected]>
@merwok
Copy link
Member

merwok commented Aug 29, 2022

See also #96397

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.10 only security fixes 3.11 only security fixes 3.12 only security fixes topic-argument-clinic type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants