Skip to content

Commit f96b647

Browse files
committed
gh-156939: Detect buffer overflow in bytes and bytearray
When Python is built in debug mode, bytes an bytearray destructors now check if the trailing null byte has been overridden to detect overflow. Add bytes_dealloc() to implement the check.
1 parent e5fbabb commit f96b647

7 files changed

Lines changed: 132 additions & 9 deletions

File tree

Lib/test/test_capi/test_bytearray.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
import sys
2+
import textwrap
23
import unittest
4+
from test import support
35
from test.support import import_helper
6+
from test.support.script_helper import assert_python_failure
47

58
_testlimitedcapi = import_helper.import_module('_testlimitedcapi')
69
from _testcapi import PY_SSIZE_T_MIN, PY_SSIZE_T_MAX
@@ -172,6 +175,24 @@ def test_resize(self):
172175
# CRASHES resize(object(), 0)
173176
# CRASHES resize(NULL, 0)
174177

178+
@unittest.skipUnless(support.Py_DEBUG, 'need debug build (Py_DEBUG)')
179+
def test_detect_overflow(self):
180+
# Test detection of buffer overflow
181+
size = 123
182+
code = textwrap.dedent(f'''
183+
from test.support import SuppressCrashReport
184+
import _testcapi
185+
186+
size = {size}
187+
with SuppressCrashReport():
188+
# Trigger a buffer overflow in a new bytearray
189+
ba = _testcapi.bytearray_overflow(size)
190+
ba = None
191+
''')
192+
proc = assert_python_failure('-c', code)
193+
self.assertIn(b'Buffer overflow detected in bytearray object', proc.err)
194+
self.assertIn(f'at position {size}'.encode(), proc.err)
195+
175196

176197
if __name__ == "__main__":
177198
unittest.main()

Lib/test/test_capi/test_bytes.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,24 @@ def test_join(self):
317317
with self.assertRaises(SystemError):
318318
bytes_join(b'', NULL)
319319

320+
@unittest.skipUnless(support.Py_DEBUG, 'need debug build (Py_DEBUG)')
321+
def test_detect_overflow(self):
322+
# Test detection of buffer overflow
323+
size = 123
324+
code = textwrap.dedent(f'''
325+
from test.support import SuppressCrashReport
326+
import _testcapi
327+
328+
size = {size}
329+
with SuppressCrashReport():
330+
# Trigger a buffer overflow in a new bytes
331+
ba = _testcapi.bytes_overflow(size)
332+
ba = None
333+
''')
334+
proc = assert_python_failure('-c', code)
335+
self.assertIn(b'Buffer overflow detected in bytes object', proc.err)
336+
self.assertIn(f'at position {size}'.encode(), proc.err)
337+
320338

321339
def get_data_canary(writer):
322340
size = writer.get_size() + 1
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
When Python is built in debug mode, :class:`bytes` and :class:`bytearray`
2+
destructors now check if the trailing null byte has been overridden to detect
3+
buffer overflow. Patch by Victor Stinner.

Modules/_testcapi/bytes.c

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -528,13 +528,46 @@ test_byteswriter_ptr(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
528528
}
529529

530530

531+
static PyObject *
532+
bytes_overflow(PyObject *Py_UNUSED(module), PyObject *arg)
533+
{
534+
PyObject *bytes = PyObject_CallOneArg((PyObject*)&PyBytes_Type, arg);
535+
if (bytes == NULL) {
536+
return NULL;
537+
}
538+
539+
char *data = PyBytes_AS_STRING(bytes);
540+
Py_ssize_t size = PyBytes_GET_SIZE(bytes);
541+
memset(data, 'x', size);
542+
data[size] = '#'; // Buffer overflow!
543+
return bytes;
544+
}
545+
546+
547+
static PyObject *
548+
bytearray_overflow(PyObject *Py_UNUSED(module), PyObject *arg)
549+
{
550+
PyObject *bytearray = PyObject_CallOneArg((PyObject*)&PyByteArray_Type, arg);
551+
if (bytearray == NULL) {
552+
return NULL;
553+
}
554+
555+
char *data = PyByteArray_AS_STRING(bytearray);
556+
Py_ssize_t size = PyByteArray_GET_SIZE(bytearray);
557+
data[size] = '#'; // Buffer overflow!
558+
return bytearray;
559+
}
560+
561+
531562
static PyMethodDef test_methods[] = {
532563
{"bytes_resize", bytes_resize, METH_VARARGS},
533564
{"bytes_join", bytes_join, METH_VARARGS},
534565
{"byteswriter_abc", byteswriter_abc, METH_NOARGS},
535566
{"byteswriter_resize", byteswriter_resize, METH_NOARGS},
536567
{"byteswriter_highlevel", byteswriter_highlevel, METH_NOARGS},
537568
{"test_byteswriter_ptr", test_byteswriter_ptr, METH_NOARGS},
569+
{"bytes_overflow", bytes_overflow, METH_O},
570+
{"bytearray_overflow", bytearray_overflow, METH_O},
538571
{NULL},
539572
};
540573

Modules/_testcapi/mem.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -448,13 +448,15 @@ test_pyobject_new(PyObject *self, PyObject *Py_UNUSED(ignored))
448448
if (obj == NULL) {
449449
goto alloc_failed;
450450
}
451+
memset(PyBytes_AS_STRING(obj), 0, 3 + 1); // +1 for the null byte
451452
Py_DECREF(obj);
452453

453454
// PyObject_NEW_VAR()
454455
obj = PyObject_NEW_VAR(PyObject, var_type, 3);
455456
if (obj == NULL) {
456457
goto alloc_failed;
457458
}
459+
memset(PyBytes_AS_STRING(obj), 0, 3 + 1); // +1 for the null byte
458460
Py_DECREF(obj);
459461

460462
Py_RETURN_NONE;

Objects/bytearrayobject.c

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1272,6 +1272,21 @@ static void
12721272
bytearray_dealloc(PyObject *op)
12731273
{
12741274
PyByteArrayObject *self = _PyByteArray_CAST(op);
1275+
1276+
#ifdef Py_DEBUG
1277+
// Make sure that the trailing null byte was not modified
1278+
if (self->ob_bytes_object != NULL) {
1279+
char *data = PyByteArray_AS_STRING(self);
1280+
Py_ssize_t size = PyByteArray_GET_SIZE(self);
1281+
if (data[size] != '\0') {
1282+
_Py_FatalErrorFormat(__func__,
1283+
"Buffer overflow detected in bytearray "
1284+
"object %p at position %zd",
1285+
self, size);
1286+
}
1287+
}
1288+
#endif
1289+
12751290
if (self->ob_exports > 0) {
12761291
PyErr_SetString(PyExc_SystemError,
12771292
"deallocated bytearray object has exported buffers");

Objects/bytesobject.c

Lines changed: 40 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3191,12 +3191,36 @@ bytes_iteritem(PyObject *obj, Py_ssize_t index)
31913191
return (_PyObjectIndexPair) { .object = l, .index = index + 1 };
31923192
}
31933193

3194+
#ifdef Py_DEBUG
3195+
static void
3196+
bytes_dealloc(PyObject *op)
3197+
{
3198+
// Make sure that the trailing null byte was not modified
3199+
PyBytesObject *self = _PyBytes_CAST(op);
3200+
char *data = PyBytes_AS_STRING(self);
3201+
Py_ssize_t size = PyBytes_GET_SIZE(self);
3202+
if (data[size] != '\0') {
3203+
_Py_FatalErrorFormat(__func__,
3204+
"Buffer overflow detected in bytes object %p "
3205+
"at position %zd",
3206+
self, size);
3207+
}
3208+
3209+
Py_TYPE(self)->tp_free((PyObject *)self);
3210+
}
3211+
#endif
3212+
3213+
31943214
PyTypeObject PyBytes_Type = {
31953215
PyVarObject_HEAD_INIT(&PyType_Type, 0)
31963216
"bytes",
31973217
PyBytesObject_SIZE,
31983218
sizeof(char),
3219+
#ifdef Py_DEBUG
3220+
bytes_dealloc, /* tp_dealloc */
3221+
#else
31993222
0, /* tp_dealloc */
3223+
#endif
32003224
0, /* tp_vectorcall_offset */
32013225
0, /* tp_getattr */
32023226
0, /* tp_setattr */
@@ -3665,6 +3689,20 @@ byteswriter_write_canary_byte(PyBytesWriter *writer)
36653689
unsigned char *data = (unsigned char*)byteswriter_data(writer);
36663690
data[writer->size] = PyBytesWriter_CANARY_BYTE;
36673691
}
3692+
3693+
3694+
static void
3695+
byteswriter_reset_trailing_byte(PyBytesWriter *writer)
3696+
{
3697+
if (writer->obj != NULL) {
3698+
// PyBytesArray writes non-zero canary byte as the last byte.
3699+
// bytes/bytearray expects the last byte to be a null byte.
3700+
// Reset the last byte to null for bytes/bytearray.
3701+
Py_ssize_t allocated = byteswriter_allocated(writer);
3702+
char *data = byteswriter_data(writer);
3703+
data[allocated] = '\0';
3704+
}
3705+
}
36683706
#endif
36693707

36703708

@@ -3814,6 +3852,7 @@ PyBytesWriter_Discard(PyBytesWriter *writer)
38143852

38153853
#ifdef Py_DEBUG
38163854
byteswriter_check_canary_byte(writer);
3855+
byteswriter_reset_trailing_byte(writer);
38173856
#endif
38183857

38193858
Py_XDECREF(writer->obj);
@@ -3838,16 +3877,8 @@ PyBytesWriter_FinishWithSize(PyBytesWriter *writer, Py_ssize_t size)
38383877
}
38393878

38403879
#ifdef Py_DEBUG
3841-
// Check for buffer overflow
38423880
byteswriter_check_canary_byte(writer);
3843-
3844-
if (writer->obj != NULL) {
3845-
// byteswriter_write_canary_byte() can override the trailing NUL byte.
3846-
// So reset the trailing NUL byte to NUL.
3847-
Py_ssize_t allocated = byteswriter_allocated(writer);
3848-
char *data = byteswriter_data(writer);
3849-
data[allocated] = '\0';
3850-
}
3881+
byteswriter_reset_trailing_byte(writer);
38513882
#endif
38523883

38533884
PyObject *result;

0 commit comments

Comments
 (0)