Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Include/internal/pycore_bytesobject.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,13 @@ extern int _PyBytes_ResizeKeepOnError(PyObject **pv, Py_ssize_t newsize);
extern int _PyBytes_IsMutable(PyObject *obj);
#endif

#ifdef Py_DEBUG
extern void _PyBytes_CheckOverflow(
PyObject *op,
void *addr,
const char *type_name);
#endif

/* --- PyBytesWriter ------------------------------------------------------ */

struct PyBytesWriter {
Expand Down
23 changes: 23 additions & 0 deletions Lib/test/test_capi/test_bytearray.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import sys
import textwrap
import unittest
from test import support
from test.support import import_helper
from test.support.script_helper import assert_python_failure

_testlimitedcapi = import_helper.import_module('_testlimitedcapi')
from _testcapi import PY_SSIZE_T_MIN, PY_SSIZE_T_MAX
Expand Down Expand Up @@ -172,6 +175,26 @@ def test_resize(self):
# CRASHES resize(object(), 0)
# CRASHES resize(NULL, 0)

@unittest.skipUnless(support.Py_DEBUG, 'need debug build (Py_DEBUG)')
def test_detect_overflow(self):
# Test detection of buffer overflow
size = 123 # bytes
overflow = 1 # bytes
code = textwrap.dedent(f'''
from test.support import SuppressCrashReport
import _testcapi

size = {size}
overflow = {overflow}
with SuppressCrashReport():
# Trigger a buffer overflow in a new bytearray
ba = _testcapi.bytearray_overflow(size, overflow)
ba = None
''')
proc = assert_python_failure('-c', code)
self.assertIn(b'Buffer overflow detected in bytearray object', proc.err)
self.assertIn(f'at position {size}'.encode(), proc.err)


if __name__ == "__main__":
unittest.main()
20 changes: 20 additions & 0 deletions Lib/test/test_capi/test_bytes.py
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,26 @@ def test_join(self):
with self.assertRaises(SystemError):
bytes_join(b'', NULL)

@unittest.skipUnless(support.Py_DEBUG, 'need debug build (Py_DEBUG)')
def test_detect_overflow(self):
# Test detection of buffer overflow
size = 123 # bytes
overflow = 1 # bytes
code = textwrap.dedent(f'''
from test.support import SuppressCrashReport
import _testcapi

size = {size}
overflow = {overflow}
with SuppressCrashReport():
# Trigger a buffer overflow in a new bytes
ba = _testcapi.bytes_overflow(size, overflow)
ba = None
''')
proc = assert_python_failure('-c', code)
self.assertIn(b'Buffer overflow detected in bytes object', proc.err)
self.assertIn(f'at position {size}'.encode(), proc.err)


def get_data_canary(writer):
size = writer.get_size() + 1
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
When Python is built in debug mode, :class:`bytes` and :class:`bytearray`
destructors now check if the trailing null byte has been overridden to detect
buffer overflow. Patch by Victor Stinner.
41 changes: 41 additions & 0 deletions Modules/_testcapi/bytes.c
Original file line number Diff line number Diff line change
Expand Up @@ -528,13 +528,54 @@ test_byteswriter_ptr(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
}


static PyObject *
bytes_overflow(PyObject *Py_UNUSED(module), PyObject *args)
{
Py_ssize_t alloc, overflow = 1;
if (!PyArg_ParseTuple(args, "n|n", &alloc, &overflow))
return NULL;

PyObject *bytes = PyObject_CallFunction((PyObject*)&PyBytes_Type, "n", alloc);
if (bytes == NULL) {
return NULL;
}

char *data = PyBytes_AS_STRING(bytes);
Py_ssize_t size = PyBytes_GET_SIZE(bytes);
memset(data, 'x', size);
memset(data + size, '#', overflow); // Buffer overflow!
return bytes;
}


static PyObject *
bytearray_overflow(PyObject *Py_UNUSED(module), PyObject *args)
{
Py_ssize_t alloc, overflow = 1;
if (!PyArg_ParseTuple(args, "n|n", &alloc, &overflow))
return NULL;

PyObject *bytearray = PyObject_CallFunction((PyObject*)&PyByteArray_Type, "n", alloc);
if (bytearray == NULL) {
return NULL;
}

char *data = PyByteArray_AS_STRING(bytearray);
Py_ssize_t size = PyByteArray_GET_SIZE(bytearray);
memset(data + size, '#', overflow); // Buffer overflow!
return bytearray;
}


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},
{"bytes_overflow", bytes_overflow, METH_VARARGS},
{"bytearray_overflow", bytearray_overflow, METH_VARARGS},
{NULL},
};

Expand Down
2 changes: 2 additions & 0 deletions Modules/_testcapi/mem.c
Original file line number Diff line number Diff line change
Expand Up @@ -448,13 +448,15 @@ test_pyobject_new(PyObject *self, PyObject *Py_UNUSED(ignored))
if (obj == NULL) {
goto alloc_failed;
}
memset(PyBytes_AS_STRING(obj), 0, 3 + 1); // +1 for the null byte
Py_DECREF(obj);

// PyObject_NEW_VAR()
obj = PyObject_NEW_VAR(PyObject, var_type, 3);
if (obj == NULL) {
goto alloc_failed;
}
memset(PyBytes_AS_STRING(obj), 0, 3 + 1); // +1 for the null byte
Py_DECREF(obj);

Py_RETURN_NONE;
Expand Down
6 changes: 6 additions & 0 deletions Objects/bytearrayobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -1272,6 +1272,12 @@ static void
bytearray_dealloc(PyObject *op)
{
PyByteArrayObject *self = _PyByteArray_CAST(op);
#ifdef Py_DEBUG
if (self->ob_bytes_object != NULL) {
Comment thread
vstinner marked this conversation as resolved.
_PyBytes_CheckOverflow(self->ob_bytes_object, op, "bytearray");
}
#endif

if (self->ob_exports > 0) {
PyErr_SetString(PyExc_SystemError,
"deallocated bytearray object has exported buffers");
Expand Down
88 changes: 66 additions & 22 deletions Objects/bytesobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -3191,12 +3191,42 @@ bytes_iteritem(PyObject *obj, Py_ssize_t index)
return (_PyObjectIndexPair) { .object = l, .index = index + 1 };
}

#ifdef Py_DEBUG
void
_PyBytes_CheckOverflow(PyObject *self, void *addr, const char *type_name)
{
// Make sure that the trailing null byte was not modified
char *data = PyBytes_AS_STRING(self);
Py_ssize_t size = PyBytes_GET_SIZE(self);
if (data[size] != '\0') {
_Py_FatalErrorFormat(__func__,
"Buffer overflow detected in %s object %p "
"at position %zd",
type_name, addr, size);
}
}


static void
bytes_dealloc(PyObject *op)
{
PyBytesObject *self = _PyBytes_CAST(op);
_PyBytes_CheckOverflow(op, op, "bytes");
Py_TYPE(self)->tp_free((PyObject *)self);
}
#endif


PyTypeObject PyBytes_Type = {
PyVarObject_HEAD_INIT(&PyType_Type, 0)
"bytes",
PyBytesObject_SIZE,
sizeof(char),
#ifdef Py_DEBUG
bytes_dealloc, /* tp_dealloc */
#else
0, /* tp_dealloc */
#endif
0, /* tp_vectorcall_offset */
0, /* tp_getattr */
0, /* tp_setattr */
Expand Down Expand Up @@ -3665,6 +3695,18 @@ byteswriter_write_canary_byte(PyBytesWriter *writer)
unsigned char *data = (unsigned char*)byteswriter_data(writer);
data[writer->size] = PyBytesWriter_CANARY_BYTE;
}


static void
byteswriter_reset_trailing_byte(PyBytesWriter *writer)
{
// PyBytesWriter writes non-zero canary byte as the last byte.
// bytes/bytearray expects the last byte to be a null byte.
// Reset the last byte to null for bytes/bytearray.
Py_ssize_t allocated = byteswriter_allocated(writer);
char *data = byteswriter_data(writer);
data[allocated] = '\0';
}
#endif


Expand Down Expand Up @@ -3814,6 +3856,9 @@ PyBytesWriter_Discard(PyBytesWriter *writer)

#ifdef Py_DEBUG
byteswriter_check_canary_byte(writer);
if (writer->obj != NULL) {
byteswriter_reset_trailing_byte(writer);
}
#endif

Py_XDECREF(writer->obj);
Expand All @@ -3838,23 +3883,19 @@ PyBytesWriter_FinishWithSize(PyBytesWriter *writer, Py_ssize_t size)
}

#ifdef Py_DEBUG
// Check for buffer overflow
byteswriter_check_canary_byte(writer);

if (writer->obj != NULL) {
// byteswriter_write_canary_byte() can override the trailing NUL byte.
// So reset the trailing NUL byte to NUL.
Py_ssize_t allocated = byteswriter_allocated(writer);
char *data = byteswriter_data(writer);
data[allocated] = '\0';
}
#endif

PyObject *result;
if (size == 0) {
result = bytes_get_empty();
}
else if (writer->obj != NULL) {
// Truncate the bytes/bytearray object if needed
#ifdef Py_DEBUG
byteswriter_reset_trailing_byte(writer);
#endif

if (writer->use_bytearray) {
if (size != PyByteArray_GET_SIZE(writer->obj)) {
if (PyByteArray_Resize(writer->obj, size)) {
Expand All @@ -3868,25 +3909,28 @@ PyBytesWriter_FinishWithSize(PyBytesWriter *writer, Py_ssize_t size)
goto error;
}
}

if (size == 1) {
// Get the single byte singleton
unsigned char ch = PyBytes_AS_STRING(writer->obj)[0];
PyObject *op = (PyObject*)CHARACTER(ch);
assert(_Py_IsImmortal(op));
Py_SETREF(writer->obj, op);
}
}

result = writer->obj;
writer->obj = NULL;

if (size == 1 && !writer->use_bytearray) {
// Get the single byte singleton
unsigned char ch = PyBytes_AS_STRING(result)[0];
PyObject *op = (PyObject*)CHARACTER(ch);
assert(_Py_IsImmortal(op));
Py_SETREF(result, op);
}
}
else if (writer->use_bytearray) {
result = PyByteArray_FromStringAndSize(writer->small_buffer, size);
}
else {
// The function returns single byte singleton if size equals 1
result = PyBytes_FromStringAndSize(writer->small_buffer, size);
// Create an object from the small buffer
if (writer->use_bytearray) {
result = PyByteArray_FromStringAndSize(writer->small_buffer, size);
}
else {
// The function returns single byte singleton if size equals 1
result = PyBytes_FromStringAndSize(writer->small_buffer, size);
}
}

#ifdef Py_DEBUG
Expand Down
Loading