Skip to content

Commit f5dd52d

Browse files
vstinnermaurycy
andauthored
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. Co-authored-by: Maurycy Pawłowski-Wieroński <maurycy@maurycy.com>
1 parent d9565e5 commit f5dd52d

5 files changed

Lines changed: 213 additions & 27 deletions

File tree

Include/internal/pycore_bytesobject.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,8 @@ PyAPI_FUNC(PyObject *) _PyBytes_Repeat(PyObject *self, Py_ssize_t n);
7575
*/
7676
#define _PyBytesObject_SIZE (offsetof(PyBytesObject, ob_sval) + 1)
7777

78+
extern int _PyBytes_ResizeKeepOnError(PyObject **pv, Py_ssize_t newsize);
79+
7880
/* --- PyBytesWriter ------------------------------------------------------ */
7981

8082
struct PyBytesWriter {

Lib/test/test_capi/test_bytes.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,24 @@ def test_example_resize(self):
446446
def test_example_highlevel(self):
447447
self.assertEqual(_testcapi.byteswriter_highlevel(), b'Hello World!')
448448

449+
def test_resize_error(self):
450+
small_buffer = _testcapi.PyBytesWriter_small_buffer
451+
init = b'x' * (small_buffer * 2)
452+
writer = self.create_writer(len(init), init)
453+
size = len(init) + 100
454+
try:
455+
with self.assertRaises(MemoryError):
456+
_testcapi.set_nomemory(0)
457+
writer.resize(size, b'')
458+
finally:
459+
_testcapi.remove_mem_hooks()
460+
suffix = b'still working'
461+
writer.write_bytes(suffix, -1)
462+
self.assertEqual(writer.finish(), self.result_type(init + suffix))
463+
464+
# Note: PyBytesWriter_Resize() leaves the buffer unchanged (no resize)
465+
# if the new size is smaller than the allocated size
466+
449467

450468
class ByteArrayWriterTest(BaseWriterTest, unittest.TestCase):
451469
result_type = bytearray
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/mem.c

Lines changed: 139 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -177,18 +177,29 @@ fm_remove_hooks(void)
177177
}
178178
}
179179

180+
static void
181+
fm_set_nomemory(int start, int stop)
182+
{
183+
/* Memory allocation fails after 'start' allocation requests, and until
184+
* 'stop' allocation requests except when 'stop' is negative or equal
185+
* to 0 (default) in which case allocation failures never stop. */
186+
FmData.start = start;
187+
FmData.stop = stop;
188+
FmData.count = 0;
189+
fm_setup_hooks();
190+
}
191+
180192
static PyObject *
181193
set_nomemory(PyObject *self, PyObject *args)
182194
{
183195
/* Memory allocation fails after 'start' allocation requests, and until
184196
* 'stop' allocation requests except when 'stop' is negative or equal
185197
* to 0 (default) in which case allocation failures never stop. */
186-
FmData.count = 0;
187-
FmData.stop = 0;
188-
if (!PyArg_ParseTuple(args, "i|i", &FmData.start, &FmData.stop)) {
198+
int start, stop = 0;
199+
if (!PyArg_ParseTuple(args, "i|i", &start, &stop)) {
189200
return NULL;
190201
}
191-
fm_setup_hooks();
202+
fm_set_nomemory(start, stop);
192203
Py_RETURN_NONE;
193204
}
194205

@@ -821,6 +832,129 @@ get_process_memory_usage(PyObject *self, PyObject *args)
821832
#endif
822833

823834

835+
struct bytes_resize_tracer {
836+
PyObject *create;
837+
PyObject *destroy;
838+
};
839+
840+
841+
static int
842+
bytes_resize_tracer(PyObject *obj, PyRefTracerEvent event, void* data)
843+
{
844+
if (event != PyRefTracer_CREATE && event != PyRefTracer_DESTROY) {
845+
return 0;
846+
}
847+
848+
struct bytes_resize_tracer *tracer = (struct bytes_resize_tracer*)data;
849+
if (!PyBytes_Check(obj)) {
850+
return 0;
851+
}
852+
853+
switch (event) {
854+
case PyRefTracer_CREATE:
855+
tracer->create = obj;
856+
break;
857+
case PyRefTracer_DESTROY:
858+
tracer->destroy = obj;
859+
break;
860+
default:
861+
break;
862+
}
863+
return 0;
864+
}
865+
866+
867+
// When _PyBytes_Resize() resizes a bytes object in-place, check that
868+
// PyRefTracer_DESTROY and PyRefTracer_CREATE events are emitted.
869+
// If no_memory is non-zero, inject MemoryError.
870+
static int
871+
check_bytes_resize_tracer(int no_memory)
872+
{
873+
PyObject *bytes = NULL;
874+
PyRefTracer old_tracer = NULL;
875+
void *old_tracer_data = NULL;
876+
int restore_tracer = 0;
877+
878+
bytes = PyBytes_FromString("hello");
879+
if (bytes == NULL) {
880+
goto error;
881+
}
882+
assert(PyUnstable_Object_IsUniquelyReferenced(bytes));
883+
884+
old_tracer = PyRefTracer_GetTracer(&old_tracer_data);
885+
restore_tracer = 1;
886+
887+
struct bytes_resize_tracer tracer = {0};
888+
if (PyRefTracer_SetTracer(bytes_resize_tracer, &tracer) != 0) {
889+
goto error;
890+
}
891+
892+
PyObject *old_bytes = bytes; // borrowed reference
893+
if (no_memory) {
894+
fm_set_nomemory(0, 0);
895+
int res = _PyBytes_Resize(&bytes, 100);
896+
assert(res < 0);
897+
assert(bytes == NULL);
898+
fm_remove_hooks();
899+
900+
assert(PyErr_ExceptionMatches(PyExc_MemoryError));
901+
PyErr_Clear();
902+
}
903+
else {
904+
if (_PyBytes_Resize(&bytes, 100) < 0) {
905+
assert(bytes == NULL);
906+
goto error;
907+
}
908+
}
909+
910+
if (tracer.destroy != old_bytes) {
911+
PyErr_SetString(PyExc_AssertionError, "PyRefTracer_DESTROY not seen");
912+
goto error;
913+
}
914+
915+
int seen_create;
916+
if (no_memory) {
917+
seen_create = (tracer.create == old_bytes);
918+
}
919+
else {
920+
seen_create = (tracer.create == bytes);
921+
}
922+
if (!seen_create) {
923+
PyErr_SetString(PyExc_AssertionError, "PyRefTracer_CREATE not seen");
924+
goto error;
925+
}
926+
927+
Py_CLEAR(bytes);
928+
if (PyRefTracer_SetTracer(old_tracer, old_tracer_data) != 0) {
929+
restore_tracer = 0;
930+
goto error;
931+
}
932+
return 0;
933+
934+
error:
935+
Py_XDECREF(bytes);
936+
if (restore_tracer) {
937+
if (PyRefTracer_SetTracer(old_tracer, old_tracer_data) != 0) {
938+
return -1;
939+
}
940+
}
941+
return -1;
942+
}
943+
944+
945+
static PyObject*
946+
test_bytes_resize_tracer(PyObject *self, PyObject *Py_UNUSED(ignored))
947+
{
948+
if (check_bytes_resize_tracer(0) < 0) {
949+
return NULL;
950+
}
951+
if (check_bytes_resize_tracer(1) < 0) {
952+
return NULL;
953+
}
954+
Py_RETURN_NONE;
955+
}
956+
957+
824958
static PyMethodDef test_methods[] = {
825959
{"pymem_api_misuse", pymem_api_misuse, METH_NOARGS},
826960
{"pymem_buffer_overflow", pymem_buffer_overflow, METH_NOARGS},
@@ -838,6 +972,7 @@ static PyMethodDef test_methods[] = {
838972
#if TARGET_OS_OSX || defined(__FreeBSD__)
839973
{"get_process_memory_usage", get_process_memory_usage, METH_VARARGS},
840974
#endif
975+
{"test_bytes_resize_tracer", test_bytes_resize_tracer, METH_NOARGS},
841976

842977
// Tracemalloc tests
843978
{"tracemalloc_track", tracemalloc_track, METH_VARARGS},

Objects/bytesobject.c

Lines changed: 51 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -3344,69 +3344,96 @@ PyBytes_ConcatAndDel(PyObject **pv, PyObject *w)
33443344
does *not* include that), and a trailing \0 byte is stored.
33453345
*/
33463346

3347+
// Similar to _PyBytes_Resize(), but leaves the object unchanged on error.
33473348
int
3348-
_PyBytes_Resize(PyObject **pv, Py_ssize_t newsize)
3349+
_PyBytes_ResizeKeepOnError(PyObject **pv, Py_ssize_t newsize)
33493350
{
3350-
PyObject *v;
3351-
PyBytesObject *sv;
3352-
v = *pv;
3351+
PyObject *v = *pv;
3352+
PyObject *result;
3353+
33533354
if (!PyBytes_Check(v) || newsize < 0) {
3354-
*pv = 0;
3355-
Py_DECREF(v);
33563355
PyErr_BadInternalCall();
33573356
return -1;
33583357
}
3358+
33593359
Py_ssize_t oldsize = PyBytes_GET_SIZE(v);
33603360
if (oldsize == newsize) {
33613361
/* return early if newsize equals to v->ob_size */
33623362
return 0;
33633363
}
3364+
33643365
if (oldsize == 0) {
3365-
*pv = _PyBytes_FromSize(newsize, 0);
3366+
result = _PyBytes_FromSize(newsize, 0);
3367+
if (result == NULL) {
3368+
return -1;
3369+
}
3370+
*pv = result;
33663371
Py_DECREF(v);
3367-
return (*pv == NULL) ? -1 : 0;
3372+
return 0;
33683373
}
3374+
33693375
if (newsize == 0) {
3370-
*pv = bytes_get_empty();
3376+
*pv = bytes_get_empty(); // cannot fail
33713377
Py_DECREF(v);
33723378
return 0;
33733379
}
3380+
33743381
if (!_PyObject_IsUniquelyReferenced(v)) {
33753382
// Allocate and then copy so we don't get a shared immortal
33763383
// one-character singleton!
3377-
*pv = _PyBytes_FromSize(newsize, 0);
3378-
if (*pv) {
3379-
memcpy(PyBytes_AS_STRING(*pv), PyBytes_AS_STRING(v),
3380-
Py_MIN(oldsize, newsize));
3384+
result = _PyBytes_FromSize(newsize, 0);
3385+
if (!result) {
3386+
return -1;
33813387
}
3388+
3389+
memcpy(PyBytes_AS_STRING(result), PyBytes_AS_STRING(v),
3390+
Py_MIN(oldsize, newsize));
3391+
*pv = result;
33823392
Py_DECREF(v);
3383-
return (*pv == NULL) ? -1 : 0;
3393+
return 0;
33843394
}
33853395
assert(v != bytes_get_empty());
33863396

33873397
#ifdef Py_TRACE_REFS
33883398
_Py_ForgetReference(v);
33893399
#endif
33903400
_PyReftracerTrack(v, PyRefTracer_DESTROY);
3391-
*pv = (PyObject *)
3392-
PyObject_Realloc(v, PyBytesObject_SIZE + newsize);
3393-
if (*pv == NULL) {
3394-
#ifdef Py_REF_DEBUG
3395-
_Py_DecRefTotal(_PyThreadState_GET());
3401+
3402+
result = (PyObject *)PyObject_Realloc(v, PyBytesObject_SIZE + newsize);
3403+
if (result == NULL) {
3404+
#ifdef Py_TRACE_REFS
3405+
_Py_AddToAllObjects(v);
33963406
#endif
3397-
PyObject_Free(v);
3407+
_PyReftracerTrack(v, PyRefTracer_CREATE);
3408+
33983409
PyErr_NoMemory();
33993410
return -1;
34003411
}
3401-
_Py_NewReferenceNoTotal(*pv);
3402-
sv = (PyBytesObject *) *pv;
3412+
3413+
*pv = result;
3414+
v = result;
3415+
_Py_NewReferenceNoTotal(v);
3416+
PyBytesObject *sv = (PyBytesObject *)v;
34033417
Py_SET_SIZE(sv, newsize);
34043418
sv->ob_sval[newsize] = '\0';
34053419
set_ob_shash(sv, -1); /* invalidate cached hash value */
34063420
return 0;
34073421
}
34083422

34093423

3424+
int
3425+
_PyBytes_Resize(PyObject **pv, Py_ssize_t newsize)
3426+
{
3427+
int res = _PyBytes_ResizeKeepOnError(pv, newsize);
3428+
if (res < 0) {
3429+
PyObject *v = *pv;
3430+
*pv = NULL;
3431+
Py_DECREF(v);
3432+
}
3433+
return res;
3434+
}
3435+
3436+
34103437
/*********************** Bytes Iterator ****************************/
34113438

34123439
typedef struct {
@@ -3644,7 +3671,8 @@ byteswriter_resize(PyBytesWriter *writer, Py_ssize_t size, int resize)
36443671
}
36453672
}
36463673
else {
3647-
if (_PyBytes_Resize(&writer->obj, size)) {
3674+
if (_PyBytes_ResizeKeepOnError(&writer->obj, size)) {
3675+
assert(writer->obj != NULL);
36483676
return -1;
36493677
}
36503678
}

0 commit comments

Comments
 (0)