-
-
Notifications
You must be signed in to change notification settings - Fork 35k
gh-153419: Fix several issues around bytearray __init__ #153498
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c961d61
b54ca0f
314253c
98888e0
f5ea967
c57fe6b
b03da26
689a52d
9ee14d4
6f73d57
0d64e2b
ecd5941
072bc04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -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); | ||||||||
|
|
||||||||
| /* All computations are done unsigned to avoid integer overflows | ||||||||
| (see issue #22335). */ | ||||||||
| size_t alloc = (size_t) obj->ob_alloc; | ||||||||
|
|
@@ -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. */ | ||||||||
|
|
@@ -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__ | ||||||||
|
|
||||||||
|
|
@@ -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. */ | ||||||||
|
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. */ | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was probably a badly worded comment, as per your previous, the The benefit of putting it here is that the processing of the first argument ( I'm open to suggestions here :) |
||||||||
| if (!_canresize(self)) { | ||||||||
|
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); | ||||||||
|
|
||||||||
|
|
@@ -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); | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not 100% sure that _PyBytes_Resize() always set
Suggested change
|
||||||||
| bytearray_reinit_from_bytes(self, 0, 0); | ||||||||
| Py_DECREF(remaining); | ||||||||
| return NULL; | ||||||||
| } | ||||||||
|
|
@@ -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, | ||||||||
| }; | ||||||||
|
|
||||||||
There was a problem hiding this comment.
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.