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
40 changes: 40 additions & 0 deletions Lib/test/test_bytes.py
Original file line number Diff line number Diff line change
Expand Up @@ -2197,6 +2197,46 @@ def __len__(self):

self.assertRaises(BufferError, ba.hex, S(b':'))

def test_no_init_called(self):
# A bytearray created without calling bytearray.__init__
# should not crash the interpreter (see gh-153419).
def bytearray_new():
return bytearray.__new__(bytearray)

bytearray_new().insert(0, 1)
bytearray_new().extend(b"x")
bytearray_new().extend([1, 2, 3])
bytearray_new().resize(4)
bytearray_new().__init__(5)
bytearray_new().__init__(b"xyz")
bytearray_new().take_bytes()
bytearray_new().take_bytes(0)

a = bytearray_new()
a.append(1)

a = bytearray_new()
a += b"x"

a = bytearray_new()
a[:] = b"xyz"

def test_reinit_length(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)
a = bytearray(1)
a.__init__()
self.assertEqual(a, b"")

def test_reinit_with_view(self):
a = bytearray()
with memoryview(a):
self.assertRaises(BufferError, a.__init__, "x", "ascii")
self.assertEqual(a, b"")


class AssortedBytesTest(unittest.TestCase):
#
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix multiple :class:`bytearray` crashes and reference leaks caused by skipping :meth:`~object.__init__` and broken state setup code.
48 changes: 36 additions & 12 deletions Objects/bytearrayobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,9 @@ bytearray_resize_lock_held(PyObject *self, Py_ssize_t requested_size)
{
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self);
PyByteArrayObject *obj = ((PyByteArrayObject *)self);

assert(obj->ob_bytes_object != NULL);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind to apply this sanity check to _PyBytes_Resize()? The empty bytes string singleton must never be resized.

diff --git a/Objects/bytesobject.c b/Objects/bytesobject.c
index f63185e1428..264ce0d9a10 100644
--- a/Objects/bytesobject.c
+++ b/Objects/bytesobject.c
@@ -3381,6 +3381,7 @@ _PyBytes_Resize(PyObject **pv, Py_ssize_t newsize)
         return (*pv == NULL) ? -1 : 0;
     }
 
+    assert(v != bytes_get_empty());
 #ifdef Py_TRACE_REFS
     _Py_ForgetReference(v);
 #endif


/* All computations are done unsigned to avoid integer overflows
(see issue #22335). */
size_t alloc = (size_t) obj->ob_alloc;
Expand All @@ -236,6 +239,14 @@ bytearray_resize_lock_held(PyObject *self, Py_ssize_t requested_size)
return -1;
}

/* Resize to 0 resets to empty bytes (see issue #153419). */
if (requested_size == 0) {
Py_SETREF(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. */
Expand Down Expand Up @@ -902,6 +913,20 @@ bytearray_ass_subscript(PyObject *op, PyObject *index, PyObject *values)
return ret;
}

static PyObject *
bytearray_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
{
PyObject *op = PyType_GenericNew(type, args, kwds);
if (op == NULL) {
return NULL;
}
PyByteArrayObject *self = _PyByteArray_CAST(op);
self->ob_bytes_object = Py_GetConstant(Py_CONSTANT_EMPTY_BYTES);
bytearray_reinit_from_bytes(self, 0, 0);
self->ob_exports = 0;
return op;
}

/*[clinic input]
bytearray.__init__

Expand All @@ -920,20 +945,16 @@ 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. */
Comment thread
stestagg marked this conversation as resolved.
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;
/* Disallow any __init__ call if the object is not resizable (has exports)
to make the handling of non-null `source` init values simpler. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this quite matches the current state? The non-null is always handled in the tp_new; the resize is just preventing a reallocation which would leave the existing exports and pointers into unowned memory

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was probably a badly worded comment, as per your previous, the _canresize() isn't strictly needed here, because PyByteArray_Resize checks this before doing an actual resize.

The benefit of putting it here is that the processing of the first argument (source in the docs) is a bit easier if we eagerly check _canresize(). The non-null in the comment is referring to that (because when source is null, we don't do anything more than resize()).

I'm open to suggestions here :)

if (!_canresize(self)) {
Comment thread
stestagg marked this conversation as resolved.
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!). */
if (PyByteArray_Resize((PyObject *)self, 0) < 0) {
return -1;
}

/* Should be caused by first init or the resize to 0. */
assert(self->ob_bytes_object == Py_GetConstantBorrowed(Py_CONSTANT_EMPTY_BYTES));
assert(self->ob_exports == 0);

Expand Down Expand Up @@ -1607,6 +1628,9 @@ bytearray_take_bytes_impl(PyByteArrayObject *self, PyObject *n)
}

if (_PyBytes_Resize(&self->ob_bytes_object, to_take) == -1) {
assert(self->ob_bytes_object == NULL);
self->ob_bytes_object = Py_GetConstant(Py_CONSTANT_EMPTY_BYTES);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure that _PyBytes_Resize() always set self->ob_bytes_object to NULL on error. Also, this private function can change in the future. So I would prefer to add this assertion to make sure that there is no refleak:

Suggested change
self->ob_bytes_object = Py_GetConstant(Py_CONSTANT_EMPTY_BYTES);
assert(self->ob_bytes_object == NULL);
self->ob_bytes_object = Py_GetConstant(Py_CONSTANT_EMPTY_BYTES);

bytearray_reinit_from_bytes(self, 0, 0);
Py_DECREF(remaining);
return NULL;
}
Expand Down Expand Up @@ -2937,7 +2961,7 @@ PyTypeObject PyByteArray_Type = {
0, /* tp_dictoffset */
bytearray___init__, /* tp_init */
PyType_GenericAlloc, /* tp_alloc */
PyType_GenericNew, /* tp_new */
bytearray_new, /* tp_new */
PyObject_Free, /* tp_free */
.tp_version_tag = _Py_TYPE_VERSION_BYTEARRAY,
};
Expand Down
1 change: 1 addition & 0 deletions Objects/bytesobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -3380,6 +3380,7 @@ _PyBytes_Resize(PyObject **pv, Py_ssize_t newsize)
Py_DECREF(v);
return (*pv == NULL) ? -1 : 0;
}
assert(v != bytes_get_empty());

#ifdef Py_TRACE_REFS
_Py_ForgetReference(v);
Expand Down
Loading