int.from_bytes crashes if byteorder is a string subclass

[sobolevn]

Here's a minimal reproduction:

» ./python.exe
Python 3.12.0a1+ (heads/main:bded5edd9a, Oct 27 2022, 23:29:21) [Clang 11.0.0 (clang-1100.0.33.16)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class SubStr(str): ...
... 
>>> int.from_bytes(b"0", SubStr("big"))
Assertion failed: (PyUnicode_CheckExact(str1)), function _PyUnicode_Equal, file Objects/unicodeobject.c, line 10447.
[1]    18466 abort      ./python.exe

I think this happes because of these lines:

cpython/Objects/unicodeobject.c

Lines 10444 to 10453 in bded5ed

	int
	_PyUnicode_Equal(PyObject *str1, PyObject *str2)
	{
	assert(PyUnicode_CheckExact(str1));
	assert(PyUnicode_CheckExact(str2));
	if (str1 == str2) {
	return 1;
	}
	return unicode_compare_eq(str1, str2);
	}

_PyUnicode_Equal uses assert(PyUnicode_CheckExact(...)), while many function (including int_from_bytes_impl) use PyUnicode_Check() or just parse str objects from args.

cpython/Objects/longobject.c

Lines 6167 to 6170 in bded5ed

	else if (_PyUnicode_Equal(byteorder, &_Py_ID(little)))
	little_endian = 1;
	else if (_PyUnicode_Equal(byteorder, &_Py_ID(big)))
	little_endian = 0;

Probably other functions that use _PyUnicode_Equal are also affected.

I would like to raise a question: shouldn't it be assert(PyUnicode_Check(...)) in _PyUnicode_Equal?

I would like to send a PR with the fix!

[sobolevn]

CC @sweeneyde as blame owner

[sweeneyde]

I would like to raise a question: shouldn't it be assert(PyUnicode_Check(...)) in _PyUnicode_Equal?

_PyUnicode_Equal was originally added as a part of 03768c4, for use in COMPARE_OP_STR_JUMP opcode, where it would have had the incorrect behavior for string subclasses, which could have __eq__ overridden.

So it looks like that made the switch from _PyUnicode_EqualToASCIIId to _PyUnicode_Equal not an exact replacement.

Replacing the assertion seems reasonable to me, since COMPARE_OP_STR_JUMP checks exact types already, but I would want to double-check that that is equivalent to _PyUnicode_EqualToASCIIId.

[sweeneyde]

Related: faster-cpython/ideas#486 would add back in a hash check for these cases, which would restore the hash check that was in _PyUnicode_EqualToASCIIId.

[sweeneyde]

There also seems to be a discrepancy between whether or not _PyUnicode_Equal can give an error. I don't think it ever can now, and it seems the lines you mention assume it doesn't, but there's this comment:

cpython/Include/cpython/unicodeobject.h

Lines 948 to 949 in 3e07f82

	/* Equality check. Returns -1 on failure. */
	PyAPI_FUNC(int) _PyUnicode_Equal(PyObject *, PyObject *);

and this error check:

cpython/Python/ceval.c

Lines 3422 to 3425 in 3e07f82

	int res = _PyUnicode_Equal(left, right);
	if (res < 0) {
	goto error;
	}

At the time it was added, error checks were needed because PyUnicode_READY could fail. But since f9c9354, PyUnicode_READY is no longer necessary, and the check cannot fail.

I think it should be safe to delete the "Returns -1 on failure" comment and remove the NULL check in ceval.c.

[sobolevn]

Other places that crash because of this:

• int.to_bytes: the same as OP
• type_repr:

» ./python.exe       
Python 3.12.0a1+ (heads/main:bded5edd9a, Oct 27 2022, 23:29:21) [Clang 11.0.0 (clang-1100.0.33.16)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class A: ...
... 
>>> class SubStr(str): ...
... 
>>> A.__module__ = SubStr('ex')
>>> A
Assertion failed: (PyUnicode_CheckExact(str1)), function _PyUnicode_Equal, file Objects/unicodeobject.c, line 10447.
[1]    21783 abort      ./python.exe

• type_new_visit_slots and type_new_copy_slots

>>> class SubStr(str): ...
... 
>>> class A:
...    __slots__ = (SubStr('a'),)
... 
Assertion failed: (PyUnicode_CheckExact(str1)), function _PyUnicode_Equal, file Objects/unicodeobject.c, line 10447.
[1]    21853 abort      ./python.exe

• super_getattro
• Probably write_unraisable_exc_file, but I cannot reproduce it
• print_exception_message

[sweeneyde]

Thanks for finding and fixing this!

I went ahead and opened #98879 about the error handling.