FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-156939: Detect buffer overflow in PyBytesWriter in debug mode (#15… · python/cpython@12a1de1 · GitHub

  1. python
  2. cpython

Repository navigation

Commit 12a1de1

Browse files
vstinner
authored
gh-156939: Detect buffer overflow in PyBytesWriter in debug mode (#156943)
Reserve one byte in PyBytesWriter used as a canary byte: set it to a special value. PyBytesWriter_Finish() checks if the canary byte has been overriden to detect a buffer overflow. Add tests for this feature. Update buffer overflow check in fcntl: allocate extra guard bytes in the writer and then truncate these bytes.
1 parent 1a703ab commit 12a1de1

6 files changed

Lines changed: 230 additions & 59 deletions

File tree

‎Lib/test/test_capi/test_bytes.py‎

Lines changed: 120 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import sys
2+
import textwrap
23
import unittest
34
from test import support
45
from test.support import import_helper
6+
from test.support.script_helper import assert_python_failure
57

68
_testlimitedcapi = import_helper.import_module('_testlimitedcapi')
79
_testcapi = import_helper.import_module('_testcapi')
@@ -316,12 +318,18 @@ def test_join(self):
316318
bytes_join(b'', NULL)
317319

318320

321+
def get_data_canary(writer):
322+
size = writer.get_size() + 1
323+
return writer.get_data(size)
324+
325+
319326
class BaseWriterTest:
320327
RESULT_TYPE = NotImplementedError
321328
SMALL_BUFFER = 11 # bytes
322329
assert SMALL_BUFFER < _testcapi.PyBytesWriter_small_buffer
323330
LARGE_BUFFER = _testcapi.PyBytesWriter_small_buffer + 17 # bytes
324331
NEW_BYTE = b'\xff'
332+
CANARY_BYTE = b'\xdd'
325333

326334
def create_writer(self, alloc=0, string=b''):
327335
raise NotImplementedError
@@ -344,6 +352,7 @@ def test_get_data(self):
344352
# Test PyBytesWriter_GetData()
345353
writer = self.create_writer(6)
346354
NEW_BYTE = self.NEW_BYTE
355+
CANARY_BYTE = self.CANARY_BYTE
347356
self.assertEqual(writer.get_data(), NEW_BYTE * 6)
348357
writer.write(0, b'abc')
349358
self.assertEqual(writer.get_data(), b'abc' + NEW_BYTE * 3)
@@ -357,7 +366,7 @@ def test_get_data(self):
357366
writer.write(0, b's' * small)
358367
self.assertEqual(writer.get_data(), b's' * small)
359368
writer.resize(large)
360-
self.assertEqual(writer.get_data(), b's' * small + NEW_BYTE * (large - small))
369+
self.assertEqual(writer.get_data(), b's' * small + CANARY_BYTE + NEW_BYTE * (large - small - 1))
361370
writer.write(small, b'L' * (large - small))
362371
self.assertEqual(writer.get_data(), b's' * small + b'L' * (large - small))
363372

@@ -443,6 +452,47 @@ def test_resize(self):
443452
writer.resize(_testcapi.PY_SSIZE_T_MAX)
444453
self.assertEqual(writer.finish(), b'x' * size)
445454

455+
@unittest.skipUnless(support.Py_DEBUG, 'need debug build')
456+
def test_resize_canary(self):
457+
CANARY_BYTE = self.CANARY_BYTE
458+
for size in (self.SMALL_BUFFER, self.LARGE_BUFFER):
459+
with self.subTest(size=size):
460+
# Truncate the last byte
461+
data = b'x' * size
462+
writer = self.create_writer(size)
463+
writer.write(0, data)
464+
self.assertEqual(get_data_canary(writer), data + CANARY_BYTE)
465+
writer.resize(size - 1)
466+
self.assertEqual(get_data_canary(writer), data[:-1] + CANARY_BYTE)
467+
self.assertEqual(writer.finish(), data[:-1])
468+
469+
# Make the buffer empty
470+
writer = self.create_writer(size)
471+
writer.write(0, data)
472+
writer.resize(0)
473+
self.assertEqual(writer.get_data(), b'')
474+
self.assertEqual(writer.finish(), b'')
475+
476+
@support.nomemtest
477+
def test_resize_error(self):
478+
# Test PyBytesWriter_Resize() error
479+
init = b'x' * self.LARGE_BUFFER
480+
writer = self.create_writer(len(init))
481+
writer.write(0, init)
482+
size = len(init) + 100
483+
try:
484+
with self.assertRaises(MemoryError):
485+
_testcapi.set_nomemory(0)
486+
writer.resize(size)
487+
finally:
488+
_testcapi.remove_mem_hooks()
489+
suffix = b'still working'
490+
writer.write_bytes(suffix, -1)
491+
self.assertEqual(writer.finish(), init + suffix)
492+
493+
# Note: PyBytesWriter_Resize() leaves the buffer unchanged (no resize)
494+
# if the new size is smaller than the allocated size
495+
446496
def test_grow(self):
447497
# Test PyBytesWriter_Grow()
448498
writer = self.create_writer(0)
@@ -461,24 +511,6 @@ def test_grow(self):
461511
writer.grow(0) # noop
462512
self.assertEqual(writer.finish(), b'number=123')
463513

464-
for size in (self.SMALL_BUFFER, self.LARGE_BUFFER):
465-
with self.subTest(size=size):
466-
# Truncate the last byte
467-
data = b'x' * size
468-
writer = self.create_writer(size)
469-
writer.write(0, data)
470-
self.assertEqual(writer.get_data(), data)
471-
writer.grow(-1)
472-
self.assertEqual(writer.get_data(), data[:-1])
473-
self.assertEqual(writer.finish(), data[:-1])
474-
475-
# Make the buffer empty
476-
writer = self.create_writer(size)
477-
writer.write(0, data)
478-
writer.grow(-size)
479-
self.assertEqual(writer.get_data(), b'')
480-
self.assertEqual(writer.finish(), b'')
481-
482514
# Switch from small buffer to large buffer
483515
writer = self.create_writer()
484516
small, large = self.SMALL_BUFFER, self.LARGE_BUFFER
@@ -500,25 +532,45 @@ def test_grow(self):
500532
writer.grow(_testcapi.PY_SSIZE_T_MAX)
501533
self.assertEqual(writer.finish(), b'x' * size)
502534

535+
@unittest.skipUnless(support.Py_DEBUG, 'need debug build')
536+
def test_grow_canary(self):
537+
CANARY_BYTE = self.CANARY_BYTE
538+
for size in (self.SMALL_BUFFER, self.LARGE_BUFFER):
539+
with self.subTest(size=size):
540+
# Truncate the last byte
541+
data = b'x' * size
542+
writer = self.create_writer(size)
543+
writer.write(0, data)
544+
self.assertEqual(get_data_canary(writer), data + CANARY_BYTE)
545+
writer.grow(-1)
546+
self.assertEqual(get_data_canary(writer), data[:-1] + CANARY_BYTE)
547+
self.assertEqual(writer.finish(), data[:-1])
548+
549+
# Make the buffer empty
550+
writer = self.create_writer(size)
551+
writer.write(0, data)
552+
writer.grow(-size)
553+
self.assertEqual(writer.get_data(), b'')
554+
self.assertEqual(writer.finish(), b'')
555+
503556
@support.nomemtest
504-
def test_resize_error(self):
505-
# Test PyBytesWriter_Resize() error
557+
def test_grow_error(self):
558+
# Test PyBytesWriter_Grow() error
506559
init = b'x' * self.LARGE_BUFFER
507560
writer = self.create_writer(len(init))
508561
writer.write(0, init)
509-
size = len(init) + 100
510562
try:
511563
with self.assertRaises(MemoryError):
512564
_testcapi.set_nomemory(0)
513-
writer.resize(size)
565+
writer.grow(100)
514566
finally:
515567
_testcapi.remove_mem_hooks()
516568
suffix = b'still working'
517569
writer.write_bytes(suffix, -1)
518570
self.assertEqual(writer.finish(), init + suffix)
519571

520-
# Note: PyBytesWriter_Resize() leaves the buffer unchanged (no resize)
521-
# if the new size is smaller than the allocated size
572+
# Note: PyBytesWriter_Grow() leaves the buffer unchanged (no resize)
573+
# if grow is negative.
522574

523575
def test_format_i(self):
524576
# Test PyBytesWriter_Format()
@@ -531,6 +583,49 @@ def test_format_i(self):
531583
writer.format_i(b'y=%i', 456)
532584
self.assertEqual(writer.finish(), b'x=123, y=456')
533585

586+
@unittest.skipUnless(support.Py_DEBUG, 'need a Python debug build')
587+
def test_canary_byte(self):
588+
small_buffer = _testcapi.PyBytesWriter_small_buffer
589+
large_size = small_buffer * 10
590+
use_bytearray = (self.RESULT_TYPE == bytearray)
591+
592+
# Test small buffer and large buffer
593+
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)
612+
613+
@unittest.skipUnless(support.Py_DEBUG, 'need debug build')
614+
def test_get_data_canary(self):
615+
# Test PyBytesWriter_GetData()
616+
NEW_BYTE = self.NEW_BYTE
617+
CANARY_BYTE = self.CANARY_BYTE
618+
619+
writer = self.create_writer(6)
620+
self.assertEqual(get_data_canary(writer),
621+
NEW_BYTE * 6 + CANARY_BYTE)
622+
writer.write(0, b'abc')
623+
self.assertEqual(get_data_canary(writer),
624+
b'abc' + NEW_BYTE * 3 + CANARY_BYTE)
625+
writer.write(3, b'123')
626+
self.assertEqual(get_data_canary(writer),
627+
b'abc123' + CANARY_BYTE)
628+
534629

535630
class BytesWriterTest(BaseWriterTest, unittest.TestCase):
536631
RESULT_TYPE = bytes

‎Misc/NEWS.d/next/C_API/2026-09-04-16-41-07.gh-issue-156939.bKaQuE.rst‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
When Python is built in debug mode, :c:type:`PyBytesWriter` now detects
2+
buffer overflow. Patch by Victor Stinner.

‎Modules/_testcapi/bytes.c‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -135,22 +135,29 @@ writer_check(WriterObject *self)
135135

136136

137137
static PyObject*
138-
writer_write(PyObject *self_raw, PyObject *args)
138+
writer_write(PyObject *self_raw, PyObject *args, PyObject *kwargs)
139139
{
140140
WriterObject *self = (WriterObject *)self_raw;
141141
if (writer_check(self) < 0) {
142142
return NULL;
143143
}
144144

145+
static char *kwlist[] = {"pos", "str", "check", NULL};
145146
Py_ssize_t pos, size;
146147
char *str;
147-
if (!PyArg_ParseTuple(args, "ny#", &pos, &str, &size)) {
148+
int check = 1;
149+
if (!PyArg_ParseTupleAndKeywords(args, kwargs,
150+
"ny#|i", kwlist,
151+
&pos, &str, &size, &check)) {
148152
return NULL;
149153
}
150154

151-
if (pos < 0 || (pos + size) > PyBytesWriter_GetSize(self->writer)) {
152-
PyErr_SetString(PyExc_ValueError, "invalid position or size");
153-
return NULL;
155+
// Use check=0 to trigger a buffer overflow for example
156+
if (check) {
157+
if (pos < 0 || (pos + size) > PyBytesWriter_GetSize(self->writer)) {
158+
PyErr_SetString(PyExc_ValueError, "invalid position or size");
159+
return NULL;
160+
}
154161
}
155162

156163
char *data = PyBytesWriter_GetData(self->writer);
@@ -168,7 +175,7 @@ writer_write_bytes(PyObject *self_raw, PyObject *args)
168175
return NULL;
169176
}
170177

171-
char *bytes;
178+
const char *bytes;
172179
Py_ssize_t unused_size, size;
173180
if (!PyArg_ParseTuple(args, "y#n", &bytes, &unused_size, &size)) {
174181
return NULL;
@@ -245,15 +252,19 @@ writer_grow(PyObject *self_raw, PyObject *args)
245252

246253

247254
static PyObject*
248-
writer_get_data(PyObject *self_raw, PyObject *Py_UNUSED(args))
255+
writer_get_data(PyObject *self_raw, PyObject *args)
249256
{
250257
WriterObject *self = (WriterObject *)self_raw;
251258
if (writer_check(self) < 0) {
252259
return NULL;
253260
}
254261

255-
const char *data = PyBytesWriter_GetData(self->writer);
256262
Py_ssize_t size = PyBytesWriter_GetSize(self->writer);
263+
if (!PyArg_ParseTuple(args, "|n", &size)) {
264+
return NULL;
265+
}
266+
267+
const char *data = PyBytesWriter_GetData(self->writer);
257268
return PyBytes_FromStringAndSize(data, size);
258269
}
259270

@@ -305,12 +316,12 @@ writer_finish_with_size(PyObject *self_raw, PyObject *args)
305316

306317

307318
static PyMethodDef writer_methods[] = {
308-
{"write", _PyCFunction_CAST(writer_write), METH_VARARGS},
319+
{"write", _PyCFunction_CAST(writer_write), METH_VARARGS | METH_KEYWORDS},
309320
{"write_bytes", _PyCFunction_CAST(writer_write_bytes), METH_VARARGS},
310321
{"format_i", _PyCFunction_CAST(writer_format_i), METH_VARARGS},
311322
{"resize", _PyCFunction_CAST(writer_resize), METH_VARARGS},
312323
{"grow", _PyCFunction_CAST(writer_grow), METH_VARARGS},
313-
{"get_data", _PyCFunction_CAST(writer_get_data), METH_NOARGS},
324+
{"get_data", _PyCFunction_CAST(writer_get_data), METH_VARARGS},
314325
{"get_size", _PyCFunction_CAST(writer_get_size), METH_NOARGS},
315326
{"finish", _PyCFunction_CAST(writer_finish), METH_NOARGS},
316327
{"finish_with_size", _PyCFunction_CAST(writer_finish_with_size), METH_VARARGS},

‎Modules/fcntlmodule.c‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -121,13 +121,14 @@ fcntl_fcntl_impl(PyObject *module, int fd, int code, PyObject *arg)
121121
return PyBytes_FromStringAndSize(buf, len);
122122
}
123123
else {
124-
PyBytesWriter *writer = PyBytesWriter_Create(len);
124+
PyBytesWriter *writer = PyBytesWriter_Create(len + GUARDSZ);
125125
if (writer == NULL) {
126126
PyBuffer_Release(&view);
127127
return NULL;
128128
}
129129
char *ptr = PyBytesWriter_GetData(writer);
130130
memcpy(ptr, view.buf, len);
131+
memcpy(ptr + len, guard, GUARDSZ);
131132
PyBuffer_Release(&view);
132133

133134
do {
@@ -142,7 +143,7 @@ fcntl_fcntl_impl(PyObject *module, int fd, int code, PyObject *arg)
142143
PyBytesWriter_Discard(writer);
143144
return NULL;
144145
}
145-
if (ptr[len] != '\0') {
146+
if (memcmp(ptr + len, guard, GUARDSZ) != 0) {
146147
PyErr_SetString(PyExc_SystemError,
147148
"Memory corruption in fcntl() due to "
148149
"buffer overflow. "
@@ -151,7 +152,8 @@ fcntl_fcntl_impl(PyObject *module, int fd, int code, PyObject *arg)
151152
PyBytesWriter_Discard(writer);
152153
return NULL;
153154
}
154-
return PyBytesWriter_Finish(writer);
155+
// Truncate the trailing guard bytes
156+
return PyBytesWriter_FinishWithSize(writer, len);
155157
}
156158
#undef FCNTL_BUFSZ
157159
}
@@ -316,13 +318,14 @@ fcntl_ioctl_impl(PyObject *module, int fd, unsigned long code, PyObject *arg,
316318
return PyBytes_FromStringAndSize(buf, len);
317319
}
318320
else {
319-
PyBytesWriter *writer = PyBytesWriter_Create(len);
321+
PyBytesWriter *writer = PyBytesWriter_Create(len + GUARDSZ);
320322
if (writer == NULL) {
321323
PyBuffer_Release(&view);
322324
return NULL;
323325
}
324326
char *ptr = PyBytesWriter_GetData(writer);
325327
memcpy(ptr, view.buf, len);
328+
memcpy(ptr + len, guard, GUARDSZ);
326329
PyBuffer_Release(&view);
327330

328331
do {
@@ -337,7 +340,7 @@ fcntl_ioctl_impl(PyObject *module, int fd, unsigned long code, PyObject *arg,
337340
PyBytesWriter_Discard(writer);
338341
return NULL;
339342
}
340-
if (ptr[len] != '\0') {
343+
if (memcmp(ptr + len, guard, GUARDSZ) != 0) {
341344
PyErr_SetString(PyExc_SystemError,
342345
"Memory corruption in ioctl() due to "
343346
"buffer overflow. "
@@ -346,7 +349,8 @@ fcntl_ioctl_impl(PyObject *module, int fd, unsigned long code, PyObject *arg,
346349
PyBytesWriter_Discard(writer);
347350
return NULL;
348351
}
349-
return PyBytesWriter_Finish(writer);
352+
// Truncate the trailing guard bytes
353+
return PyBytesWriter_FinishWithSize(writer, len);
350354
}
351355
#undef IOCTL_BUFSZ
352356
}

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL