bytearray_resize_lock_held always initializes ob_bytes_object if NULL

There are several ways that bytearray_resize_lock_held gets called
and for any of them, ob_bytes_object may be NULL.

This allows us to remove the extra initialization in __init__ as
this can't be guaranteed to be called anyway.

Handle a related issue in take_bytes where in the extreme case
of the bytes resize allocation failing (on a size reduction)
then the bytearray other state fileds could be left inconsistent with
a null ob_bytes_object.

unconditionally call _canresize in __init__ as on
initial creation, this will always be true,
and if there are any exported views, then we just
always fail, even if technically an empty bytearray
doesn't get modified in this case, it's so rare, it's not worth it.

Author: stestagg

Lib/test/test_bytes.py

@@ -25,7 +25,7 @@
 import test.string_tests
 import test.list_tests
 from test.support import bigaddrspacetest, MAX_Py_ssize_t
-from test.support.script_helper import assert_python_failure
+from test.support.script_helper import assert_python_failure, assert_python_ok
 
 
 if sys.flags.bytes_warning:
@@ -2198,6 +2198,85 @@ def __len__(self):
         self.assertRaises(BufferError, ba.hex, S(b':'))
 
 
+class ByteArrayInitialization1Test(unittest.TestCase):
+    # A bytearray created with __new__ so that __init__ is never called
+    # (often as a side-effect of a subclass not calling super().__init__).
+    # Is left with ob_bytes_object == NULL.  It's easy for implementation
+    # code to not realise that ob_bytes_object can be NULL, so these tests
+    # verify a set of code paths that have historically crashed or asserted
+    # (see gh-153419)
+
+    def _check(self, stmt, expected):
+        code = textwrap.dedent(f"""
+            a = bytearray.__new__(bytearray)
+            {stmt}
+            print(list(a))
+        """)
+        rc, out, err = assert_python_ok('-c', code)
+        self.assertEqual(out.decode().strip(), expected)
+
+    def test_append(self):
+        self._check("a.append(1)", "[1]")
+
+    def test_insert(self):
+        self._check("a.insert(0, 1)", "[1]")
+
+    def test_extend_bytes(self):
+        self._check("a.extend(b'x')", "[120]")
+
+    def test_extend_iterable(self):
+        self._check("a.extend([1, 2, 3])", "[1, 2, 3]")
+
+    def test_iadd(self):
+        self._check("a += b'x'", "[120]")
+
+    def test_slice_assign(self):
+        self._check("a[:] = b'xyz'", "[120, 121, 122]")
+
+    def test_resize(self):
+        self._check("a.resize(4)", "[0, 0, 0, 0]")
+
+    def test_init_int(self):
+        self._check("a.__init__(5)", "[0, 0, 0, 0, 0]")
+
+    def test_init_bytes(self):
+        self._check("a.__init__(b'xyz')", "[120, 121, 122]")
+
+    def test_reinit_length1(self):
+        # There is a shortcut taken when resizing, where alloc/2 < newsize.
+        # In this case, the existing buffer is reused, rather than reset
+        # If this happens when newsize == 0 and alloc == 1, then various
+        # code assumptions can be violated.  This test should catch those
+        # in debug builds. (see gh-153419)
+        code = textwrap.dedent("""
+            a = bytearray(1)
+            a.__init__()
+            print(list(a))
+        """)
+        rc, out, err = assert_python_ok('-c', code)
+        self.assertEqual(out.decode().strip(), repr([]))
+
+    def test_take_bytes_all(self):
+        self._check("a.take_bytes()", "[]")
+
+    def test_take_bytes_zero(self):
+        self._check("a.take_bytes(0)", "[]")
+
+    def test_reinit_with_view(self):
+        code = textwrap.dedent("""
+            a = bytearray()
+            v = memoryview(a)
+            try:
+                a.__init__("x", "ascii")
+            except BufferError:
+                print("PASS")
+            else:
+                print("NO EXCEPTION")
+        """)
+        rc, out, err = assert_python_ok('-c', code)
+        self.assertEqual(out.decode().strip(), "PASS")
+
+
 class AssortedBytesTest(unittest.TestCase):
     #
     # Test various combinations of bytes and bytearray

Objects/bytearrayobject.c

@@ -213,6 +213,15 @@ bytearray_resize_lock_held(PyObject *self, Py_ssize_t requested_size)
 {
     _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self);
     PyByteArrayObject *obj = ((PyByteArrayObject *)self);
+
+    /* If ob_bytes_object has not been initialized yet, eagerly initialize
+       it here so the following code can reason about state more easily,
+       and things like pointer comparisons are valid. */
+    if (obj->ob_bytes_object == NULL) {
+        obj->ob_bytes_object = Py_GetConstant(Py_CONSTANT_EMPTY_BYTES);
+        bytearray_reinit_from_bytes(obj, 0, 0);
+    }
+
     /* All computations are done unsigned to avoid integer overflows
        (see issue #22335). */
     size_t alloc = (size_t) obj->ob_alloc;
@@ -236,6 +245,15 @@ bytearray_resize_lock_held(PyObject *self, Py_ssize_t requested_size)
         return -1;
     }
 
+    /* When resizing to 0, always reset to the empty-bytes constant to avoid
+       complexity in the realloc path below (see gh-153419). */
+    if (requested_size == 0) {
+        Py_XSETREF(obj->ob_bytes_object,
+                   Py_GetConstant(Py_CONSTANT_EMPTY_BYTES));
+        bytearray_reinit_from_bytes(obj, 0, 0);
+        return 0;
+    }
+
     if (size + logical_offset <= alloc) {
         /* Current buffer is large enough to host the requested size,
            decide on a strategy. */
@@ -920,20 +938,17 @@ bytearray___init___impl(PyByteArrayObject *self, PyObject *arg,
     PyObject *it;
     PyObject *(*iternext)(PyObject *);
 
-    /* First __init__; set ob_bytes_object so ob_bytes is always non-null. */
-    if (self->ob_bytes_object == NULL) {
-        self->ob_bytes_object = Py_GetConstant(Py_CONSTANT_EMPTY_BYTES);
-        bytearray_reinit_from_bytes(self, 0, 0);
-        self->ob_exports = 0;
+    if (!_canresize(self)) {
+        return -1;
     }
 
-    if (Py_SIZE(self) != 0) {
-        /* Empty previous contents (yes, do this first of all!) */
-        if (PyByteArray_Resize((PyObject *)self, 0) < 0)
-            return -1;
+    /* Empty any previous contents (do this first of all!).
+       Also initializes ob_bytes_object if needed */
+    if (PyByteArray_Resize((PyObject *)self, 0) < 0) {
+        return -1;
     }
 
-    /* Should be caused by first init or the resize to 0. */
+    /* PyByteArray_Resize(,0) should always leave the empty bytes constant */
     assert(self->ob_bytes_object == Py_GetConstantBorrowed(Py_CONSTANT_EMPTY_BYTES));
     assert(self->ob_exports == 0);
 
@@ -1607,6 +1622,10 @@ bytearray_take_bytes_impl(PyByteArrayObject *self, PyObject *n)
     }
 
     if (_PyBytes_Resize(&self->ob_bytes_object, to_take) == -1) {
+        /* _PyBytes_Resize has made ob_bytes_object NULL here
+           we need to ensure that the other byte array fields are consistent. */
+        self->ob_bytes_object = Py_GetConstant(Py_CONSTANT_EMPTY_BYTES);
+        bytearray_reinit_from_bytes(self, 0, 0);
         Py_DECREF(remaining);
         return NULL;
     }