From 33f8c2cd0af842c3dcf2f20c2b07cf93c04cbb37 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 9 Sep 2026 14:59:57 -0400 Subject: [PATCH] gh-129813: Check size in PyBytesWriter_FinishWithSize() (#157226) Reject size larger than the allocated size. Check also negative size in PyBytesWriter_FinishWithSize() to always raise ValueError. Previously, the function raised SystemError or ValueError depending on the code path. Replace _PyBytesWriter_GetAllocated() optimization with _PyBytesWriter_ResizeToAllocated() to update the writer size to its allocated size. test_capi: only run the 3 "example" tests in BytesWriterTest. There is no need to rerun the same tests in ByteArrayWriterTest. (cherry picked from commit 4e530b82f37ec89e3b54bc5d0a1612c643bff1e0) --- Lib/test/test_capi/test_bytes.py | 49 +++++++++++-- Modules/_testcapi/bytes.c | 117 ++++++++++++++++++++++++++++++- Objects/bytesobject.c | 52 +++++++++----- 3 files changed, 191 insertions(+), 27 deletions(-) diff --git a/Lib/test/test_capi/test_bytes.py b/Lib/test/test_capi/test_bytes.py index eee201b3a75bbd..3d8270ddcf6244 100644 --- a/Lib/test/test_capi/test_bytes.py +++ b/Lib/test/test_capi/test_bytes.py @@ -316,11 +316,11 @@ def test_join(self): bytes_join(b'', NULL) -class BytesWriterTest(unittest.TestCase): - RESULT_TYPE = bytes +class BaseWriterTest: + RESULT_TYPE = NotImplementedError def create_writer(self, alloc=0, string=b''): - return _testcapi.PyBytesWriter(alloc, string, 0) + raise NotImplementedError def bytes_equal(self, result, expected): # Similar to assertEqual(), but check also that the result type @@ -340,14 +340,20 @@ def test_create(self): def test_finish_with_size(self): # Test PyBytesWriter_FinishWithSize() - writer = self.create_writer(10, b'abc') + writer = self.create_writer(10, b'abcdef') self.assertEqual(writer.get_size(), 10) self.bytes_equal(writer.finish_with_size(3), b'abc') + # Error if the size is negative writer = self.create_writer(3, b'abc') - with self.assertRaises(SystemError): + with self.assertRaises(ValueError): writer.finish_with_size(-3) + # Error if the requested size is larger than the allocated size + writer = self.create_writer(3, b'abc') + with self.assertRaises(ValueError): + writer.finish_with_size(4) + def test_write_bytes(self): # Test PyBytesWriter_WriteBytes() writer = self.create_writer() @@ -420,6 +426,36 @@ def test_format_s_memory_error(self): writer.write_bytes(b'after', -1) self.bytes_equal(writer.finish(), b'after') + +class BytesWriterTest(BaseWriterTest, unittest.TestCase): + RESULT_TYPE = bytes + + def create_writer(self, alloc=0, string=b''): + # Test PyBytesWriter_Create() + return _testcapi.PyBytesWriter(alloc, string, 0) + + # Only PyBytesWriter_Create() returns singletons + def test_singletons(self): + empty = b'' + singletons = {ch: bytes((ch,)) for ch in range(256)} + small_buffer = _testcapi.PyBytesWriter_small_buffer + + writer = self.create_writer() + self.assertIs(writer.finish(), empty) + + # Test writer larger than small_buffer + writer = self.create_writer() + unused_text = b'x' * (small_buffer * 2) + writer.write_bytes(unused_text, len(unused_text)) + self.assertIs(writer.finish_with_size(0), empty) + + for ch in range(256): + text = bytes((ch,)) + + writer = self.create_writer() + writer.write_bytes(text, 1) + self.assertIs(writer.finish(), singletons[ch]) + def test_example_abc(self): self.assertEqual(_testcapi.byteswriter_abc(), b'abc') @@ -430,10 +466,11 @@ def test_example_highlevel(self): self.assertEqual(_testcapi.byteswriter_highlevel(), b'Hello World!') -class ByteArrayWriterTest(BytesWriterTest): +class ByteArrayWriterTest(BaseWriterTest, unittest.TestCase): RESULT_TYPE = bytearray def create_writer(self, alloc=0, string=b''): + # Test private _PyBytesWriter_CreateByteArray() return _testcapi.PyBytesWriter(alloc, string, 1) diff --git a/Modules/_testcapi/bytes.c b/Modules/_testcapi/bytes.c index 23b065aba869cd..735d8d67ca14c8 100644 --- a/Modules/_testcapi/bytes.c +++ b/Modules/_testcapi/bytes.c @@ -4,6 +4,8 @@ #include "parts.h" #include "util.h" +#include // offsetof() + #include "pycore_bytesobject.h" // _PyBytesWriter_CreateByteArray() @@ -149,9 +151,9 @@ writer_write_bytes(PyObject *self_raw, PyObject *args) return NULL; } - char *bytes; - Py_ssize_t size; - if (!PyArg_ParseTuple(args, "yn", &bytes, &size)) { + const char *bytes; + Py_ssize_t unused_size, size; + if (!PyArg_ParseTuple(args, "y#n", &bytes, &unused_size, &size)) { return NULL; } @@ -373,12 +375,114 @@ byteswriter_highlevel(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args)) } +static size_t +pybyteswriter_small_buffer_size(void) +{ + return offsetof(PyBytesWriter, obj); +} + + +// Test the "Pointer" API of PyBytesWriter +static PyObject * +test_byteswriter_ptr(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args)) +{ + // Test PyBytesWriter_FinishWithPointer(): create the string "abc" + PyBytesWriter *writer = PyBytesWriter_Create(3); + if (writer == NULL) { + return NULL; + } + char *str = PyBytesWriter_GetData(writer); + memcpy(str, "abc", 3); + str += 3; + PyObject *result = PyBytesWriter_FinishWithPointer(writer, str); + if (result == NULL) { + return NULL; + } + assert(PyBytes_GET_SIZE(result) == 3); + assert(memcmp(PyBytes_AS_STRING(result), "abc", 3) == 0); + Py_DECREF(result); + + // Test PyBytesWriter_GrowAndUpdatePointer(). + // Start by using the small buffer, and then resize to use a bytes object. + writer = PyBytesWriter_Create(0); + if (writer == NULL) { + return NULL; + } + str = PyBytesWriter_GetData(writer); + + str = PyBytesWriter_GrowAndUpdatePointer(writer, 100, str); + if (str == NULL) { + PyBytesWriter_Discard(writer); + return NULL; + } + memset(str, 'x', 100); + str += 100; + + // make sure that the test switchs to a bytes object + assert((100 + 200) > pybyteswriter_small_buffer_size()); + char *old_str = str; + str = PyBytesWriter_GrowAndUpdatePointer(writer, 200, str); + if (str == NULL) { + PyBytesWriter_Discard(writer); + return NULL; + } + // make sure that we moved from the small buffer to a bytes object + assert(str != old_str); + memset(str, 'y', 200); + str += 200; + + result = PyBytesWriter_FinishWithPointer(writer, str); + if (result == NULL) { + return NULL; + } + assert(PyBytes_GET_SIZE(result) == 300); + str = PyBytes_AS_STRING(result); + for (Py_ssize_t i=0; i < 100; i++) { + assert(str[i] == 'x'); + } + for (Py_ssize_t i=0; i < 200; i++) { + assert(str[100 + i] == 'y'); + } + Py_DECREF(result); + + // Check that PyBytesWriter_FinishWithPointer() rejects pointer + // after the buffer end (create a string larger than the allocated size) + writer = PyBytesWriter_Create(3); + if (writer == NULL) { + return NULL; + } + str = PyBytesWriter_GetData(writer); + memcpy(str, "abc", 3); + str += 4; // off-by-one bug on purpose + result = PyBytesWriter_FinishWithPointer(writer, str); + assert(result == NULL); + assert(PyErr_ExceptionMatches(PyExc_ValueError)); + PyErr_Clear(); + + // Check that PyBytesWriter_FinishWithPointer() rejects pointer + // before the buffer start (negative size) + writer = PyBytesWriter_Create(3); + if (writer == NULL) { + return NULL; + } + str = PyBytesWriter_GetData(writer); + str--; // bug on purpose: go before the buffer start + result = PyBytesWriter_FinishWithPointer(writer, str); + assert(result == NULL); + assert(PyErr_ExceptionMatches(PyExc_ValueError)); + PyErr_Clear(); + + Py_RETURN_NONE; +} + + static PyMethodDef test_methods[] = { {"bytes_resize", bytes_resize, METH_VARARGS}, {"bytes_join", bytes_join, METH_VARARGS}, {"byteswriter_abc", byteswriter_abc, METH_NOARGS}, {"byteswriter_resize", byteswriter_resize, METH_NOARGS}, {"byteswriter_highlevel", byteswriter_highlevel, METH_NOARGS}, + {"test_byteswriter_ptr", test_byteswriter_ptr, METH_NOARGS}, {NULL}, }; @@ -399,5 +503,12 @@ _PyTestCapi_Init_Bytes(PyObject *m) } Py_DECREF(writer_type); + // PyBytesWriter.obj is the second member, small_buffer is the first member + long size = (long)pybyteswriter_small_buffer_size(); + if (PyModule_AddIntConstant(m, "PyBytesWriter_small_buffer", size) < 0) { + Py_DECREF(writer_type); + return -1; + } + return 0; } diff --git a/Objects/bytesobject.c b/Objects/bytesobject.c index 0a8b63ea1dd104..7d8d22b32843f0 100644 --- a/Objects/bytesobject.c +++ b/Objects/bytesobject.c @@ -32,7 +32,7 @@ class bytes "PyBytesObject *" "&PyBytes_Type" /* Forward declaration */ static void* _PyBytesWriter_ResizeAndUpdatePointer(PyBytesWriter *writer, Py_ssize_t size, void *data); -static Py_ssize_t _PyBytesWriter_GetAllocated(PyBytesWriter *writer); +static Py_ssize_t _PyBytesWriter_ResizeToAllocated(PyBytesWriter *writer); #define CHARACTERS _Py_SINGLETON(bytes_characters) @@ -2982,8 +2982,8 @@ _PyBytes_FromList(PyObject *x) if (writer == NULL) { return NULL; } + size = _PyBytesWriter_ResizeToAllocated(writer); char *str = PyBytesWriter_GetData(writer); - size = _PyBytesWriter_GetAllocated(writer); for (Py_ssize_t i = 0; i < PyList_GET_SIZE(x); i++) { PyObject *item = _PyList_GetItemRef((PyListObject *)x, i); @@ -3006,7 +3006,9 @@ _PyBytes_FromList(PyObject *x) if (str == NULL) { goto error; } - size = _PyBytesWriter_GetAllocated(writer); + + // Set the writer size to its allocated size + size = _PyBytesWriter_ResizeToAllocated(writer); } *str++ = (char) value; } @@ -3064,8 +3066,8 @@ _PyBytes_FromIterator(PyObject *it, PyObject *x) if (writer == NULL) { return NULL; } + size = _PyBytesWriter_ResizeToAllocated(writer); char *str = PyBytesWriter_GetData(writer); - size = _PyBytesWriter_GetAllocated(writer); /* Run the iterator to exhaustion */ for (i = 0; ; i++) { @@ -3099,7 +3101,9 @@ _PyBytes_FromIterator(PyObject *it, PyObject *x) if (str == NULL) { goto error; } - size = _PyBytesWriter_GetAllocated(writer); + + // Set the writer size to its allocated size + size = _PyBytesWriter_ResizeToAllocated(writer); } *str++ = (char) value; } @@ -3734,6 +3738,19 @@ PyBytesWriter_Discard(PyBytesWriter *writer) PyObject* PyBytesWriter_FinishWithSize(PyBytesWriter *writer, Py_ssize_t size) { + // Check for negative size here to raise ValueError in all cases, rather + // than having a different exception depending on the code path. For + // example, _PyBytes_Resize() raises SystemError on negative size. + if (size < 0) { + PyErr_Format(PyExc_ValueError, "size must be positive"); + goto error; + } + + if (size > writer->size) { + PyErr_SetString(PyExc_ValueError, "size larger than allocated size"); + goto error; + } + PyObject *result; if (size == 0 && !writer->use_bytearray) { result = bytes_get_empty(); @@ -3781,12 +3798,6 @@ PyObject* PyBytesWriter_FinishWithPointer(PyBytesWriter *writer, void *buf) { Py_ssize_t size = (char*)buf - byteswriter_data(writer); - if (size < 0 || size > byteswriter_allocated(writer)) { - PyBytesWriter_Discard(writer); - PyErr_SetString(PyExc_ValueError, "invalid end pointer"); - return NULL; - } - return PyBytesWriter_FinishWithSize(writer, size); } @@ -3805,13 +3816,6 @@ PyBytesWriter_GetSize(PyBytesWriter *writer) } -static Py_ssize_t -_PyBytesWriter_GetAllocated(PyBytesWriter *writer) -{ - return byteswriter_allocated(writer); -} - - int PyBytesWriter_Resize(PyBytesWriter *writer, Py_ssize_t size) { @@ -3918,3 +3922,15 @@ PyBytesWriter_Format(PyBytesWriter *writer, const char *format, ...) Py_ssize_t size = buf - byteswriter_data(writer); return PyBytesWriter_Resize(writer, size); } + + +// Resize the writer to its allocated size. +// Return the new size. +// The function cannot fail. +static Py_ssize_t +_PyBytesWriter_ResizeToAllocated(PyBytesWriter *writer) +{ + Py_ssize_t allocated = byteswriter_allocated(writer); + writer->size = allocated; + return allocated; +}