Skip to content

Commit 2f471c9

Browse files
vstinnermaurycy
andcommitted
gh-157242: Fix PyBytesWriter_Resize() on MemoryError (#157243)
If PyBytesWriter_Resize() fails, leave the writer unchanged. Add a new internal _PyBytes_ResizeKeepOnError() function similar to _PyBytes_Resize() but leaves the bytes object unchanged on error. Add BaseWriterTest for BytesWriterTest and ByteArrayWriterTest. Add _testcapi.PyBytesWriter_small_buffer constant. Co-authored-by: Maurycy Pawłowski-Wieroński <maurycy@maurycy.com> (cherry picked from commit f5dd52d)
1 parent 0073717 commit 2f471c9

6 files changed

Lines changed: 240 additions & 32 deletions

File tree

‎Include/internal/pycore_bytesobject.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,8 @@ _PyBytes_Repeat(char* dest, Py_ssize_t len_dest,
7373
*/
7474
#define _PyBytesObject_SIZE (offsetof(PyBytesObject, ob_sval) + 1)
7575

76+
extern int _PyBytes_ResizeKeepOnError(PyObject **pv, Py_ssize_t newsize);
77+
7678

7779
// Return the cached hash value, or -1 if not cached yet.
7880
static inline Py_hash_t

‎Lib/test/test_capi/test_bytes.py‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -316,11 +316,11 @@ def test_join(self):
316316
bytes_join(b'', NULL)
317317

318318

319-
class BytesWriterTest(unittest.TestCase):
320-
RESULT_TYPE = bytes
319+
class BaseWriterTest:
320+
RESULT_TYPE = NotImplementedError
321321

322322
def create_writer(self, alloc=0, string=b''):
323-
return _testcapi.PyBytesWriter(alloc, string, 0)
323+
raise NotImplementedError
324324

325325
def bytes_equal(self, result, expected):
326326
# Similar to assertEqual(), but check also that the result type
@@ -420,6 +420,13 @@ def test_format_s_memory_error(self):
420420
writer.write_bytes(b'after', -1)
421421
self.bytes_equal(writer.finish(), b'after')
422422

423+
424+
class BytesWriterTest(BaseWriterTest, unittest.TestCase):
425+
RESULT_TYPE = bytes
426+
427+
def create_writer(self, alloc=0, string=b''):
428+
return _testcapi.PyBytesWriter(alloc, string, 0)
429+
423430
def test_example_abc(self):
424431
self.assertEqual(_testcapi.byteswriter_abc(), b'abc')
425432

@@ -429,8 +436,26 @@ def test_example_resize(self):
429436
def test_example_highlevel(self):
430437
self.assertEqual(_testcapi.byteswriter_highlevel(), b'Hello World!')
431438

432-
433-
class ByteArrayWriterTest(BytesWriterTest):
439+
def test_resize_error(self):
440+
small_buffer = _testcapi.PyBytesWriter_small_buffer
441+
init = b'x' * (small_buffer * 2)
442+
writer = self.create_writer(len(init), init)
443+
size = len(init) + 100
444+
try:
445+
with self.assertRaises(MemoryError):
446+
_testcapi.set_nomemory(0)
447+
writer.resize(size, b'')
448+
finally:
449+
_testcapi.remove_mem_hooks()
450+
suffix = b'still working'
451+
writer.write_bytes(suffix, -1)
452+
self.bytes_equal(writer.finish(), init + suffix)
453+
454+
# Note: PyBytesWriter_Resize() leaves the buffer unchanged (no resize)
455+
# if the new size is smaller than the allocated size
456+
457+
458+
class ByteArrayWriterTest(BaseWriterTest, unittest.TestCase):
434459
RESULT_TYPE = bytearray
435460

436461
def create_writer(self, alloc=0, string=b''):
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Fix :c:func:`PyBytesWriter_Resize` to handle properly memory allocation
2+
failure. Leave the writer unchanged on error, instead of leaving it in an
3+
inconsistent state. Patch by Victor Stinner.

‎Modules/_testcapi/bytes.c‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
#include "parts.h"
55
#include "util.h"
66

7+
#include <stddef.h> // offsetof()
8+
79
#include "pycore_bytesobject.h" // _PyBytesWriter_CreateByteArray()
810

911

@@ -373,6 +375,14 @@ byteswriter_highlevel(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
373375
}
374376

375377

378+
static size_t
379+
pybyteswriter_small_buffer_size(void)
380+
{
381+
// PyBytesWriter.obj is the second member, small_buffer is the first member
382+
return offsetof(PyBytesWriter, obj);
383+
}
384+
385+
376386
static PyMethodDef test_methods[] = {
377387
{"bytes_resize", bytes_resize, METH_VARARGS},
378388
{"bytes_join", bytes_join, METH_VARARGS},
@@ -399,5 +409,10 @@ _PyTestCapi_Init_Bytes(PyObject *m)
399409
}
400410
Py_DECREF(writer_type);
401411

412+
long size = (long)pybyteswriter_small_buffer_size();
413+
if (PyModule_AddIntConstant(m, "PyBytesWriter_small_buffer", size) < 0) {
414+
return -1;
415+
}
416+
402417
return 0;
403418
}

‎Modules/_testcapi/mem.c‎

Lines changed: 139 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -155,18 +155,29 @@ fm_remove_hooks(void)
155155
}
156156
}
157157

158+
static void
159+
fm_set_nomemory(int start, int stop)
160+
{
161+
/* Memory allocation fails after 'start' allocation requests, and until
162+
* 'stop' allocation requests except when 'stop' is negative or equal
163+
* to 0 (default) in which case allocation failures never stop. */
164+
FmData.start = start;
165+
FmData.stop = stop;
166+
FmData.count = 0;
167+
fm_setup_hooks();
168+
}
169+
158170
static PyObject *
159171
set_nomemory(PyObject *self, PyObject *args)
160172
{
161173
/* Memory allocation fails after 'start' allocation requests, and until
162174
* 'stop' allocation requests except when 'stop' is negative or equal
163175
* to 0 (default) in which case allocation failures never stop. */
164-
FmData.count = 0;
165-
FmData.stop = 0;
166-
if (!PyArg_ParseTuple(args, "i|i", &FmData.start, &FmData.stop)) {
176+
int start, stop = 0;
177+
if (!PyArg_ParseTuple(args, "i|i", &start, &stop)) {
167178
return NULL;
168179
}
169-
fm_setup_hooks();
180+
fm_set_nomemory(start, stop);
170181
Py_RETURN_NONE;
171182
}
172183

@@ -731,6 +742,129 @@ tracemalloc_track_race(PyObject *self, PyObject *args)
731742
}
732743

733744

745+
struct bytes_resize_tracer {
746+
PyObject *create;
747+
PyObject *destroy;
748+
};
749+
750+
751+
static int
752+
bytes_resize_tracer(PyObject *obj, PyRefTracerEvent event, void* data)
753+
{
754+
if (event != PyRefTracer_CREATE && event != PyRefTracer_DESTROY) {
755+
return 0;
756+
}
757+
758+
struct bytes_resize_tracer *tracer = (struct bytes_resize_tracer*)data;
759+
if (!PyBytes_Check(obj)) {
760+
return 0;
761+
}
762+
763+
switch (event) {
764+
case PyRefTracer_CREATE:
765+
tracer->create = obj;
766+
break;
767+
case PyRefTracer_DESTROY:
768+
tracer->destroy = obj;
769+
break;
770+
default:
771+
break;
772+
}
773+
return 0;
774+
}
775+
776+
777+
// When _PyBytes_Resize() resizes a bytes object in-place, check that
778+
// PyRefTracer_DESTROY and PyRefTracer_CREATE events are emitted.
779+
// If no_memory is non-zero, inject MemoryError.
780+
static int
781+
check_bytes_resize_tracer(int no_memory)
782+
{
783+
PyObject *bytes = NULL;
784+
PyRefTracer old_tracer = NULL;
785+
void *old_tracer_data = NULL;
786+
int restore_tracer = 0;
787+
788+
bytes = PyBytes_FromString("hello");
789+
if (bytes == NULL) {
790+
goto error;
791+
}
792+
assert(PyUnstable_Object_IsUniquelyReferenced(bytes));
793+
794+
old_tracer = PyRefTracer_GetTracer(&old_tracer_data);
795+
restore_tracer = 1;
796+
797+
struct bytes_resize_tracer tracer = {0};
798+
if (PyRefTracer_SetTracer(bytes_resize_tracer, &tracer) != 0) {
799+
goto error;
800+
}
801+
802+
PyObject *old_bytes = bytes; // borrowed reference
803+
if (no_memory) {
804+
fm_set_nomemory(0, 0);
805+
int res = _PyBytes_Resize(&bytes, 100);
806+
assert(res < 0);
807+
assert(bytes == NULL);
808+
fm_remove_hooks();
809+
810+
assert(PyErr_ExceptionMatches(PyExc_MemoryError));
811+
PyErr_Clear();
812+
}
813+
else {
814+
if (_PyBytes_Resize(&bytes, 100) < 0) {
815+
assert(bytes == NULL);
816+
goto error;
817+
}
818+
}
819+
820+
if (tracer.destroy != old_bytes) {
821+
PyErr_SetString(PyExc_AssertionError, "PyRefTracer_DESTROY not seen");
822+
goto error;
823+
}
824+
825+
int seen_create;
826+
if (no_memory) {
827+
seen_create = (tracer.create == old_bytes);
828+
}
829+
else {
830+
seen_create = (tracer.create == bytes);
831+
}
832+
if (!seen_create) {
833+
PyErr_SetString(PyExc_AssertionError, "PyRefTracer_CREATE not seen");
834+
goto error;
835+
}
836+
837+
Py_CLEAR(bytes);
838+
if (PyRefTracer_SetTracer(old_tracer, old_tracer_data) != 0) {
839+
restore_tracer = 0;
840+
goto error;
841+
}
842+
return 0;
843+
844+
error:
845+
Py_XDECREF(bytes);
846+
if (restore_tracer) {
847+
if (PyRefTracer_SetTracer(old_tracer, old_tracer_data) != 0) {
848+
return -1;
849+
}
850+
}
851+
return -1;
852+
}
853+
854+
855+
static PyObject*
856+
test_bytes_resize_tracer(PyObject *self, PyObject *Py_UNUSED(ignored))
857+
{
858+
if (check_bytes_resize_tracer(0) < 0) {
859+
return NULL;
860+
}
861+
if (check_bytes_resize_tracer(1) < 0) {
862+
return NULL;
863+
}
864+
Py_RETURN_NONE;
865+
}
866+
867+
734868
static PyMethodDef test_methods[] = {
735869
{"pymem_api_misuse", pymem_api_misuse, METH_NOARGS},
736870
{"pymem_buffer_overflow", pymem_buffer_overflow, METH_NOARGS},
@@ -745,6 +879,7 @@ static PyMethodDef test_methods[] = {
745879
{"test_pymem_setrawallocators", test_pymem_setrawallocators, METH_NOARGS},
746880
{"test_pyobject_new", test_pyobject_new, METH_NOARGS},
747881
{"test_pyobject_setallocators", test_pyobject_setallocators, METH_NOARGS},
882+
{"test_bytes_resize_tracer", test_bytes_resize_tracer, METH_NOARGS},
748883

749884
// Tracemalloc tests
750885
{"tracemalloc_track", tracemalloc_track, METH_VARARGS},

0 commit comments

Comments
 (0)