Rewrite canary byte test in Python

* Add test_get_data_canary()
* Update test_get_data()

Author: vstinner

Lib/test/test_capi/test_bytes.py

@@ -324,6 +324,7 @@ class BaseWriterTest:
     assert SMALL_BUFFER < _testcapi.PyBytesWriter_small_buffer
     LARGE_BUFFER = _testcapi.PyBytesWriter_small_buffer + 17  # bytes
     NEW_BYTE = b'\xff'
+    CANARY_BYTE = b'\xdd'
 
     def create_writer(self, alloc=0, string=b''):
         raise NotImplementedError
@@ -346,6 +347,7 @@ def test_get_data(self):
         # Test PyBytesWriter_GetData()
         writer = self.create_writer(6)
         NEW_BYTE = self.NEW_BYTE
+        CANARY_BYTE = self.CANARY_BYTE
         self.assertEqual(writer.get_data(), NEW_BYTE * 6)
         writer.write(0, b'abc')
         self.assertEqual(writer.get_data(), b'abc' + NEW_BYTE * 3)
@@ -359,7 +361,7 @@ def test_get_data(self):
         writer.write(0, b's' * small)
         self.assertEqual(writer.get_data(), b's' * small)
         writer.resize(large)
-        self.assertEqual(writer.get_data(), b's' * small + NEW_BYTE * (large - small))
+        self.assertEqual(writer.get_data(), b's' * small + CANARY_BYTE + NEW_BYTE * (large - small - 1))
         writer.write(small, b'L' * (large - small))
         self.assertEqual(writer.get_data(), b's' * small + b'L' * (large - small))
 
@@ -515,6 +517,51 @@ def test_format_i(self):
         writer.format_i(b'y=%i', 456)
         self.assertEqual(writer.finish(), b'x=123, y=456')
 
+    @unittest.skipUnless(support.Py_DEBUG, 'need a Python debug build')
+    def test_canary_byte(self):
+        small_buffer = _testcapi.PyBytesWriter_small_buffer
+        large_size = small_buffer * 10
+        use_bytearray = (self.RESULT_TYPE == bytearray)
+
+        # Test small buffer and large buffer
+        for size in (0, self.SMALL_BUFFER, self.LARGE_BUFFER):
+            with self.subTest(size=size):
+                code = textwrap.dedent(f"""
+                    from test.support import SuppressCrashReport
+                    import _testcapi
+                    size = {size}
+                    # Add an extra '#' byte to trigger a buffer overflow
+                    data = b'x' * size + b'#'
+                    use_bytearray = {use_bytearray}
+                    writer = _testcapi.PyBytesWriter(size, use_bytearray)
+                    with SuppressCrashReport():
+                        writer.write(0, data, check=False)
+                        writer.finish()
+                """)
+                proc = assert_python_failure('-c', code)
+                self.assertIn(b'Buffer overflow detected in PyBytesWriter',
+                              proc.err)
+                self.assertIn(f'at position {size}'.encode(),
+                              proc.err)
+
+    @unittest.skipUnless(support.Py_DEBUG, 'need Py_DEBUG')
+    def test_get_data_canary(self):
+        # Test PyBytesWriter_GetData()
+        NEW_BYTE = self.NEW_BYTE
+        CANARY_BYTE = self.CANARY_BYTE
+        canary_byte_size = len(CANARY_BYTE)
+
+        def get_data_canary():
+            size = writer.get_size() + canary_byte_size
+            return writer.get_data(size)
+
+        writer = self.create_writer(6)
+        self.assertEqual(get_data_canary(), NEW_BYTE * 6 + CANARY_BYTE)
+        writer.write(0, b'abc')
+        self.assertEqual(get_data_canary(), b'abc' + NEW_BYTE * 3 + CANARY_BYTE)
+        writer.write(3, b'123')
+        self.assertEqual(get_data_canary(), b'abc123' + CANARY_BYTE)
+
 
 class BytesWriterTest(BaseWriterTest, unittest.TestCase):
     RESULT_TYPE = bytes
@@ -567,28 +614,6 @@ def test_example_resize(self):
     def test_example_highlevel(self):
         self.assertEqual(_testcapi.byteswriter_highlevel(), b'Hello World!')
 
-    @unittest.skipUnless(support.Py_DEBUG, 'need a Python debug build')
-    def test_canary_byte(self):
-        small_buffer = _testcapi.PyBytesWriter_small_buffer
-        large_size = small_buffer * 10
-
-        # Test small buffer and large buffer
-        for size in (0, 3, large_size):
-            with self.subTest(size=size):
-                code = textwrap.dedent(f"""
-                    from test.support import SuppressCrashReport
-                    import _testcapi
-                    size = {size}
-                    data = b'x' * size
-                    with SuppressCrashReport():
-                        _testcapi.byteswriter_test_canary_byte(data)
-                """)
-                proc = assert_python_failure('-c', code)
-                self.assertIn(b'Buffer overflow detected in PyBytesWriter',
-                              proc.err)
-                self.assertIn(f'at position {size}'.encode(),
-                              proc.err)
-
 
 class ByteArrayWriterTest(BaseWriterTest, unittest.TestCase):
     RESULT_TYPE = bytearray

Modules/_testcapi/bytes.c

@@ -135,22 +135,29 @@ writer_check(WriterObject *self)
 
 
 static PyObject*
-writer_write(PyObject *self_raw, PyObject *args)
+writer_write(PyObject *self_raw, PyObject *args, PyObject *kwargs)
 {
     WriterObject *self = (WriterObject *)self_raw;
     if (writer_check(self) < 0) {
         return NULL;
     }
 
+    static char *kwlist[] = {"pos", "str", "check", NULL};
     Py_ssize_t pos, size;
     char *str;
-    if (!PyArg_ParseTuple(args, "ny#", &pos, &str, &size)) {
+    int check = 1;
+    if (!PyArg_ParseTupleAndKeywords(args, kwargs,
+                                     "ny#|i", kwlist,
+                                     &pos, &str, &size, &check)) {
         return NULL;
     }
 
-    if (pos < 0 || (pos + size) > PyBytesWriter_GetSize(self->writer)) {
-        PyErr_SetString(PyExc_ValueError, "invalid position or size");
-        return NULL;
+    // Use check=0 to trigger a buffer overflow for example
+    if (check) {
+        if (pos < 0 || (pos + size) > PyBytesWriter_GetSize(self->writer)) {
+            PyErr_SetString(PyExc_ValueError, "invalid position or size");
+            return NULL;
+        }
     }
 
     char *data = PyBytesWriter_GetData(self->writer);
@@ -245,15 +252,19 @@ writer_grow(PyObject *self_raw, PyObject *args)
 
 
 static PyObject*
-writer_get_data(PyObject *self_raw, PyObject *Py_UNUSED(args))
+writer_get_data(PyObject *self_raw, PyObject *args)
 {
     WriterObject *self = (WriterObject *)self_raw;
     if (writer_check(self) < 0) {
         return NULL;
     }
 
-    const char *data = PyBytesWriter_GetData(self->writer);
     Py_ssize_t size = PyBytesWriter_GetSize(self->writer);
+    if (!PyArg_ParseTuple(args, "|n", &size)) {
+        return NULL;
+    }
+
+    const char *data = PyBytesWriter_GetData(self->writer);
     return PyBytes_FromStringAndSize(data, size);
 }
 
@@ -305,12 +316,12 @@ writer_finish_with_size(PyObject *self_raw, PyObject *args)
 
 
 static PyMethodDef writer_methods[] = {
-    {"write", _PyCFunction_CAST(writer_write), METH_VARARGS},
+    {"write", _PyCFunction_CAST(writer_write), METH_VARARGS | METH_KEYWORDS},
     {"write_bytes", _PyCFunction_CAST(writer_write_bytes), METH_VARARGS},
     {"format_i", _PyCFunction_CAST(writer_format_i), METH_VARARGS},
     {"resize", _PyCFunction_CAST(writer_resize), METH_VARARGS},
     {"grow", _PyCFunction_CAST(writer_grow), METH_VARARGS},
-    {"get_data", _PyCFunction_CAST(writer_get_data), METH_NOARGS},
+    {"get_data", _PyCFunction_CAST(writer_get_data), METH_VARARGS},
     {"get_size", _PyCFunction_CAST(writer_get_size), METH_NOARGS},
     {"finish", _PyCFunction_CAST(writer_finish), METH_NOARGS},
     {"finish_with_size", _PyCFunction_CAST(writer_finish_with_size), METH_VARARGS},
@@ -502,41 +513,13 @@ test_byteswriter_ptr(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
 }
 
 
-// Trigger a buffer overflow on purpose to test the canary byte feature
-// which detects buffer overflow
-static PyObject *
-byteswriter_test_canary_byte(PyObject *Py_UNUSED(module), PyObject *args)
-{
-    const char *str;
-    Py_ssize_t len;
-    if (!PyArg_ParseTuple(args, "s#", &str, &len)) {
-        return NULL;
-    }
-
-    PyBytesWriter *writer = PyBytesWriter_Create(len);
-    if (writer == NULL) {
-        return NULL;
-    }
-
-    char *data = PyBytesWriter_GetData(writer);
-    if (len) {
-        memcpy(data, str, len);
-    }
-    data[len] = '#';  // Overflow!
-
-    // In debug mode, PyBytesWriter_Finish() checks for buffer overflow
-    return PyBytesWriter_Finish(writer);
-}
-
-
 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},
-    {"byteswriter_test_canary_byte", byteswriter_test_canary_byte, METH_VARARGS},
     {NULL},
 };