Skip to content

Commit 58cdff7

Browse files
vstinnervstinner
authored
gh-156939: Detect PyBytesWriter buffer overflow earlier (#157385)
Check the canary byte in all PyBytesWriter methods, not only in PyBytesWriter_Finish(). Add a discard() method to the _testcapi wrapper.
1 parent 2cd6d4b commit 58cdff7

3 files changed

Lines changed: 92 additions & 18 deletions

File tree

‎Lib/test/test_capi/test_bytes.py‎

Lines changed: 36 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -591,24 +591,42 @@ def test_canary_byte(self):
591591

592592
# Test small buffer and large buffer
593593
for size in (0, self.SMALL_BUFFER, self.LARGE_BUFFER):
594-
with self.subTest(size=size):
595-
code = textwrap.dedent(f"""
596-
from test.support import SuppressCrashReport
597-
import _testcapi
598-
size = {size}
599-
# Add an extra '#' byte to trigger a buffer overflow
600-
data = b'x' * size + b'#'
601-
use_bytearray = {use_bytearray}
602-
writer = _testcapi.PyBytesWriter(size, use_bytearray)
603-
with SuppressCrashReport():
604-
writer.write(0, data, check=False)
605-
writer.finish()
606-
""")
607-
proc = assert_python_failure('-c', code)
608-
self.assertIn(b'Buffer overflow detected in PyBytesWriter',
609-
proc.err)
610-
self.assertIn(f'at position {size}'.encode(),
611-
proc.err)
594+
for operation in (
595+
'writer.get_data()',
596+
'writer.get_size()',
597+
f'writer.resize({size} * 2)',
598+
f'writer.grow({size})',
599+
'writer.discard()',
600+
'writer.finish()',
601+
):
602+
with self.subTest(size=size, operation=operation):
603+
code = textwrap.dedent(f"""
604+
from test.support import SuppressCrashReport
605+
import os
606+
import _testcapi
607+
size = {size}
608+
# Add an extra '#' byte to trigger a buffer overflow
609+
data = b'x' * size + b'#'
610+
use_bytearray = {use_bytearray}
611+
writer = _testcapi.PyBytesWriter(size, use_bytearray)
612+
with SuppressCrashReport():
613+
writer.write(0, data, check=False)
614+
try:
615+
{operation}
616+
except:
617+
# Ignore all exceptions
618+
pass
619+
# If we reached this line, the operation didn't
620+
# detect the overflow. Exit immediatetly without
621+
# calling the writer destructor since it can detect
622+
# the overflow.
623+
os._exit(0)
624+
""")
625+
proc = assert_python_failure('-c', code)
626+
self.assertIn(b'Buffer overflow detected in PyBytesWriter',
627+
proc.err)
628+
self.assertIn(f'at position {size}'.encode(),
629+
proc.err)
612630

613631
@unittest.skipUnless(support.Py_DEBUG, 'need debug build')
614632
def test_get_data_canary(self):

‎Modules/_testcapi/bytes.c‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,20 @@ writer_finish_with_size(PyObject *self_raw, PyObject *args)
315315
}
316316

317317

318+
static PyObject*
319+
writer_discard(PyObject *self_raw, PyObject *Py_UNUSED(args))
320+
{
321+
WriterObject *self = (WriterObject *)self_raw;
322+
if (writer_check(self) < 0) {
323+
return NULL;
324+
}
325+
326+
PyBytesWriter_Discard(self->writer);
327+
self->writer = NULL;
328+
Py_RETURN_NONE;
329+
}
330+
331+
318332
static PyMethodDef writer_methods[] = {
319333
{"write", _PyCFunction_CAST(writer_write), METH_VARARGS | METH_KEYWORDS},
320334
{"write_bytes", _PyCFunction_CAST(writer_write_bytes), METH_VARARGS},
@@ -325,6 +339,7 @@ static PyMethodDef writer_methods[] = {
325339
{"get_size", _PyCFunction_CAST(writer_get_size), METH_NOARGS},
326340
{"finish", _PyCFunction_CAST(writer_finish), METH_NOARGS},
327341
{"finish_with_size", _PyCFunction_CAST(writer_finish_with_size), METH_VARARGS},
342+
{"discard", _PyCFunction_CAST(writer_discard), METH_VARARGS},
328343
{NULL, NULL} /* sentinel */
329344
};
330345

‎Objects/bytesobject.c‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3696,6 +3696,10 @@ byteswriter_resize(PyBytesWriter *writer, Py_ssize_t size, int resize)
36963696
if (writer->obj != NULL) {
36973697
if (writer->use_bytearray) {
36983698
if (PyByteArray_Resize(writer->obj, size)) {
3699+
#ifdef Py_DEBUG
3700+
// bytearray can override the canary byte on error
3701+
byteswriter_write_canary_byte(writer);
3702+
#endif
36993703
return -1;
37003704
}
37013705
}
@@ -3770,6 +3774,11 @@ byteswriter_create(Py_ssize_t size, int use_bytearray)
37703774

37713775
if (size >= 1) {
37723776
if (byteswriter_resize(writer, size, 0) < 0) {
3777+
#ifdef Py_DEBUG
3778+
// Write the canary byte so byteswriter_check_canary_byte()
3779+
// doesn't fail in PyBytesWriter_Discard()
3780+
byteswriter_write_canary_byte(writer);
3781+
#endif
37733782
PyBytesWriter_Discard(writer);
37743783
return NULL;
37753784
}
@@ -3803,6 +3812,10 @@ PyBytesWriter_Discard(PyBytesWriter *writer)
38033812
return;
38043813
}
38053814

3815+
#ifdef Py_DEBUG
3816+
byteswriter_check_canary_byte(writer);
3817+
#endif
3818+
38063819
Py_XDECREF(writer->obj);
38073820
_Py_FREELIST_FREE(bytes_writers, writer, PyMem_Free);
38083821
}
@@ -3875,6 +3888,14 @@ PyBytesWriter_FinishWithSize(PyBytesWriter *writer, Py_ssize_t size)
38753888
// The function returns single byte singleton if size equals 1
38763889
result = PyBytes_FromStringAndSize(writer->small_buffer, size);
38773890
}
3891+
3892+
#ifdef Py_DEBUG
3893+
// Reset the writer, so byteswriter_check_canary_byte() doesn't fail
3894+
// in PyBytesWriter_Discard().
3895+
writer->size = 0;
3896+
byteswriter_write_canary_byte(writer);
3897+
#endif
3898+
38783899
PyBytesWriter_Discard(writer);
38793900
return result;
38803901

@@ -3901,20 +3922,32 @@ PyBytesWriter_FinishWithPointer(PyBytesWriter *writer, void *buf)
39013922
void*
39023923
PyBytesWriter_GetData(PyBytesWriter *writer)
39033924
{
3925+
#ifdef Py_DEBUG
3926+
byteswriter_check_canary_byte(writer);
3927+
#endif
3928+
39043929
return byteswriter_data(writer);
39053930
}
39063931

39073932

39083933
Py_ssize_t
39093934
PyBytesWriter_GetSize(PyBytesWriter *writer)
39103935
{
3936+
#ifdef Py_DEBUG
3937+
byteswriter_check_canary_byte(writer);
3938+
#endif
3939+
39113940
return _PyBytesWriter_GetSize(writer);
39123941
}
39133942

39143943

39153944
int
39163945
PyBytesWriter_Resize(PyBytesWriter *writer, Py_ssize_t new_size)
39173946
{
3947+
#ifdef Py_DEBUG
3948+
byteswriter_check_canary_byte(writer);
3949+
#endif
3950+
39183951
if (new_size < 0) {
39193952
PyErr_SetString(PyExc_ValueError, "size must be >= 0");
39203953
return -1;
@@ -3950,6 +3983,10 @@ _PyBytesWriter_ResizeAndUpdatePointer(PyBytesWriter *writer, Py_ssize_t size,
39503983
int
39513984
PyBytesWriter_Grow(PyBytesWriter *writer, Py_ssize_t grow)
39523985
{
3986+
#ifdef Py_DEBUG
3987+
byteswriter_check_canary_byte(writer);
3988+
#endif
3989+
39533990
if (grow == 0) {
39543991
// Nothing to do
39553992
return 0;
@@ -4042,6 +4079,10 @@ PyBytesWriter_Format(PyBytesWriter *writer, const char *format, ...)
40424079
static Py_ssize_t
40434080
_PyBytesWriter_ResizeToAllocated(PyBytesWriter *writer)
40444081
{
4082+
#ifdef Py_DEBUG
4083+
byteswriter_check_canary_byte(writer);
4084+
#endif
4085+
40454086
Py_ssize_t allocated = byteswriter_allocated(writer);
40464087
writer->size = allocated;
40474088
#ifdef Py_DEBUG

0 commit comments

Comments
 (0)