From a51be95f5f1ad4d3946a93563472583e00005ef7 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 28 Jun 2024 10:03:25 +0100 Subject: [PATCH 1/4] gh-121115: Skip __index__ in PyLong_AsNativeBytes by default --- Doc/c-api/long.rst | 14 ++++++++++---- Include/cpython/longobject.h | 7 +++++-- Lib/test/test_capi/test_long.py | 12 ++++++++++-- .../2024-06-28-10-02-58.gh-issue-121115.EeSLfc.rst | 3 +++ Objects/longobject.c | 6 +++++- 5 files changed, 33 insertions(+), 9 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-06-28-10-02-58.gh-issue-121115.EeSLfc.rst diff --git a/Doc/c-api/long.rst b/Doc/c-api/long.rst index a0e111af5996d70..606b0fc8b921e3c 100644 --- a/Doc/c-api/long.rst +++ b/Doc/c-api/long.rst @@ -405,14 +405,13 @@ distinguished from a number. Use :c:func:`PyErr_Occurred` to disambiguate. Passing zero to *n_bytes* will return the size of a buffer that would be large enough to hold the value. This may be larger than technically - necessary, but not unreasonably so. + necessary, but not unreasonably so. If *n_bytes=0*, *buffer* may be + ``NULL``. .. note:: Passing *n_bytes=0* to this function is not an accurate way to determine - the bit length of a value. - - If *n_bytes=0*, *buffer* may be ``NULL``. + the bit length of the value. To get at the entire Python value of an unknown size, the function can be called twice: first to determine the buffer size, then to fill it:: @@ -462,6 +461,7 @@ distinguished from a number. Use :c:func:`PyErr_Occurred` to disambiguate. .. c:macro:: Py_ASNATIVEBYTES_NATIVE_ENDIAN ``3`` .. c:macro:: Py_ASNATIVEBYTES_UNSIGNED_BUFFER ``4`` .. c:macro:: Py_ASNATIVEBYTES_REJECT_NEGATIVE ``8`` + .. c:macro:: Py_ASNATIVEBYTES_ALLOW_INDEX ``16`` ============================================= ====== Specifying ``Py_ASNATIVEBYTES_NATIVE_ENDIAN`` will override any other endian @@ -483,6 +483,12 @@ distinguished from a number. Use :c:func:`PyErr_Occurred` to disambiguate. provided there is enough space for at least one sign bit, regardless of whether ``Py_ASNATIVEBYTES_UNSIGNED_BUFFER`` was specified. + If ``Py_ASNATIVEBYTES_ALLOW_INDEX`` is specified and a non-integer value is + passed, its :meth:`~object.__index__` will be used to convert. This may + result in Python code executing and other threads being allowed to run, which + could cause changes to other objects or values in use. When *flags* is + ``-1``, this option is not set. + .. note:: With the default *flags* (``-1``, or *UNSIGNED_BUFFER* without diff --git a/Include/cpython/longobject.h b/Include/cpython/longobject.h index 19a6722d07734a3..e7e0c3d9764f20e 100644 --- a/Include/cpython/longobject.h +++ b/Include/cpython/longobject.h @@ -10,6 +10,7 @@ PyAPI_FUNC(PyObject*) PyLong_FromUnicodeObject(PyObject *u, int base); #define Py_ASNATIVEBYTES_NATIVE_ENDIAN 3 #define Py_ASNATIVEBYTES_UNSIGNED_BUFFER 4 #define Py_ASNATIVEBYTES_REJECT_NEGATIVE 8 +#define Py_ASNATIVEBYTES_ALLOW_INDEX 16 /* PyLong_AsNativeBytes: Copy the integer value to a native variable. buffer points to the first byte of the variable. @@ -20,8 +21,10 @@ PyAPI_FUNC(PyObject*) PyLong_FromUnicodeObject(PyObject *u, int base); * 2 - native endian * 4 - unsigned destination (e.g. don't reject copying 255 into one byte) * 8 - raise an exception for negative inputs - If flags is -1 (all bits set), native endian is used and value truncation - behaves most like C (allows negative inputs and allow MSB set). + * 16 - call __index__ on non-int types + If flags is -1 (all bits set), native endian is used, value truncation + behaves most like C (allows negative inputs and allow MSB set), and non-int + objects will raise a TypeError. Big endian mode will write the most significant byte into the address directly referenced by buffer; little endian will write the least significant byte into that address. diff --git a/Lib/test/test_capi/test_long.py b/Lib/test/test_capi/test_long.py index 06a29b5a0505b42..00d2947cc979a4f 100644 --- a/Lib/test/test_capi/test_long.py +++ b/Lib/test/test_capi/test_long.py @@ -496,12 +496,14 @@ def test_long_asnativebytes(self): "PyLong_AsNativeBytes(v, , 0, -1)") self.assertEqual(buffer, b"\x5a", "buffer overwritten when it should not have been") - # Also check via the __index__ path - self.assertEqual(expect, asnativebytes(Index(v), buffer, 0, -1), + # Also check via the __index__ path. + # We pass Py_ASNATIVEBYTES_NATIVE_ENDIAN | ALLOW_INDEX + self.assertEqual(expect, asnativebytes(Index(v), buffer, 0, 3 | 16), "PyLong_AsNativeBytes(Index(v), , 0, -1)") self.assertEqual(buffer, b"\x5a", "buffer overwritten when it should not have been") + # Test that we populate n=2 bytes but do not overwrite more. buffer = bytearray(b"\x99"*3) self.assertEqual(2, asnativebytes(4, buffer, 2, 0), # BE @@ -607,6 +609,12 @@ def test_long_asnativebytes(self): with self.assertRaises(ValueError): asnativebytes(-1, buffer, 0, 8) + # Ensure omitting Py_ASNATIVEBYTES_ALLOW_INDEX raises on __index__ value + with self.assertRaises(TypeError): + asnativebytes(Index(1), buffer, 0, -1) + with self.assertRaises(TypeError): + asnativebytes(Index(1), buffer, 0, 3) + # Check a few error conditions. These are validated in code, but are # unspecified in docs, so if we make changes to the implementation, it's # fine to just update these tests rather than preserve the behaviour. diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-06-28-10-02-58.gh-issue-121115.EeSLfc.rst b/Misc/NEWS.d/next/Core and Builtins/2024-06-28-10-02-58.gh-issue-121115.EeSLfc.rst new file mode 100644 index 000000000000000..aaecc873551cc7c --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-06-28-10-02-58.gh-issue-121115.EeSLfc.rst @@ -0,0 +1,3 @@ +:c:func:`PyLong_AsNativeBytes` no longer uses :meth:`~object.__index__` +methods by default. The ``Py_ASNATIVEBYTES_ALLOW_INDEX`` flag has been added +to allow it. diff --git a/Objects/longobject.c b/Objects/longobject.c index 86afec9a4141341..4ca259fb08e8c7b 100644 --- a/Objects/longobject.c +++ b/Objects/longobject.c @@ -1128,13 +1128,17 @@ PyLong_AsNativeBytes(PyObject* vv, void* buffer, Py_ssize_t n, int flags) if (PyLong_Check(vv)) { v = (PyLongObject *)vv; } - else { + else if (flags != -1 && (flags & Py_ASNATIVEBYTES_ALLOW_INDEX)) { v = (PyLongObject *)_PyNumber_Index(vv); if (v == NULL) { return -1; } do_decref = 1; } + else { + PyErr_Format(PyExc_TypeError, "expect int, got %T", vv); + return -1; + } if ((flags != -1 && (flags & Py_ASNATIVEBYTES_REJECT_NEGATIVE)) && _PyLong_IsNegative(v)) { From 520aec33f8720b740c1612f6ce9a0577835e9a5c Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 28 Jun 2024 10:04:41 +0100 Subject: [PATCH 2/4] But what does it raise? --- Doc/c-api/long.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Doc/c-api/long.rst b/Doc/c-api/long.rst index 606b0fc8b921e3c..1964dd4d8c1387e 100644 --- a/Doc/c-api/long.rst +++ b/Doc/c-api/long.rst @@ -487,7 +487,8 @@ distinguished from a number. Use :c:func:`PyErr_Occurred` to disambiguate. passed, its :meth:`~object.__index__` will be used to convert. This may result in Python code executing and other threads being allowed to run, which could cause changes to other objects or values in use. When *flags* is - ``-1``, this option is not set. + ``-1``, this option is not set, and non-integer values will raise + :exc:`TypeError`. .. note:: From ae2b8e5b7d238e6677ac8390d88db1af4619a89e Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 28 Jun 2024 10:06:40 +0100 Subject: [PATCH 3/4] Doc tweak --- Doc/c-api/long.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/c-api/long.rst b/Doc/c-api/long.rst index 1964dd4d8c1387e..42162914c0aec81 100644 --- a/Doc/c-api/long.rst +++ b/Doc/c-api/long.rst @@ -484,7 +484,7 @@ distinguished from a number. Use :c:func:`PyErr_Occurred` to disambiguate. whether ``Py_ASNATIVEBYTES_UNSIGNED_BUFFER`` was specified. If ``Py_ASNATIVEBYTES_ALLOW_INDEX`` is specified and a non-integer value is - passed, its :meth:`~object.__index__` will be used to convert. This may + passed, its :meth:`~object.__index__` method will be called first. This may result in Python code executing and other threads being allowed to run, which could cause changes to other objects or values in use. When *flags* is ``-1``, this option is not set, and non-integer values will raise From f08d113d8097f6c27b5ff1ad212d1c5ca7893304 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 28 Jun 2024 10:15:26 +0100 Subject: [PATCH 4/4] Update Lib/test/test_capi/test_long.py Co-authored-by: Nice Zombies --- Lib/test/test_capi/test_long.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/test/test_capi/test_long.py b/Lib/test/test_capi/test_long.py index 00d2947cc979a4f..7e8d571ae234d1f 100644 --- a/Lib/test/test_capi/test_long.py +++ b/Lib/test/test_capi/test_long.py @@ -503,7 +503,6 @@ def test_long_asnativebytes(self): self.assertEqual(buffer, b"\x5a", "buffer overwritten when it should not have been") - # Test that we populate n=2 bytes but do not overwrite more. buffer = bytearray(b"\x99"*3) self.assertEqual(2, asnativebytes(4, buffer, 2, 0), # BE