gh-153419: Fix several issues around bytearray __init__#153498
gh-153419: Fix several issues around bytearray __init__#153498stestagg wants to merge 6 commits into
Conversation
Introduce a bytearray_new function to ensure that
ob_bytes_object is always set on a bytearray.
Resizing a bytearray to 0 length explicitly sets
the ob_bytes_object to the empty constant immortal.
Add a check in the 'bytearray init from string'
fast path to ensure there are no active exports.
This fixes asserts/crashes on the following:
- bytearray(1).__init__()
- bytearray().__new__(bytearray).append(1)
- a = bytearray(); b = memoryview(a); a.__init__('x', 'ascii')
This reverts commit c961d61.
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.
|
OK, rewritten based on @cmaloney's comments on the issue. Some of the changes are a bit non-obvious, for example initializing As it now intentionally tolerates uninitialized We have to check There may be some other |
| self.assertRaises(BufferError, ba.hex, S(b':')) | ||
|
|
||
|
|
||
| class ByteArrayInitialization1Test(unittest.TestCase): |
There was a problem hiding this comment.
The new test cases should be added to ByteArrayTest / that is specific enough.
There was a problem hiding this comment.
OK, fair enough, I've done this, I also folded a bunch of them together, I figured that the utility of the test is to catch a crash so just running the ops in order in one test should be more than enough.
| # verify a set of code paths that have historically crashed or asserted | ||
| # (see gh-153419). | ||
|
|
||
| def _check(self, stmt, expected): |
There was a problem hiding this comment.
Pushing all these tests through a subprocess is weird, we shoudl be able to test bytearray much more directly.
There was a problem hiding this comment.
Yes, Thanks! I came at this from a context of some other changes that did this sort of test this way for other reasons, and just went with it! Rewritten as above
| @@ -0,0 +1,4 @@ | |||
| Fix several ``bytearray`` crashes caused by calling, or not calling, | |||
There was a problem hiding this comment.
For this as well as comments they're really verbose at the moment.
For the NEWS file please use rst / Sphinx formatting: https://www.sphinx-doc.org/en/master/index.html. In particular references to classes, functions, and exceptions linking to the right objects is very useful.
There was a problem hiding this comment.
Again, sorry about this, I've rewritten the blurb, hope it's better.
| PyObject *it; | ||
| PyObject *(*iternext)(PyObject *); | ||
|
|
||
| /* First __init__; set ob_bytes_object so ob_bytes is always non-null. */ |
There was a problem hiding this comment.
Please keep the NULL check here rather than moving it inside the resize code. This as written I suspect will also fail some of the Sanitizers, ob_exports is unset until the NULL code sets it.
There was a problem hiding this comment.
The issue I have with this is that it's kinda misleading and redundant. There's 0 guarantee that this function gets called, so it looks like it's a reliable initialiser without actually being so. I can put back the code here, but we'd have to keep it in resize too, as it's easy to hit the resize call without having init called.
This is partly why I went a bit comment heavy, because reasoning about this is quite hard.
There was a problem hiding this comment.
I ran asan as you suggested , and it all checks out, I think the thing is either we accept that it's valid for ob_bytes_object to be NULL sometimes, and just handle that correctly when needed, or we have to make ob_bytes_object never be null, and the only way to do that is with tp_new. (without tricks like PyObject_New which the docs say to avoid using)
As for ob_export, it is zero initialized by default, if it's non-zero then we can't call init anyway, and you get the exception, so I can't see how it would ever need to be reinitialized here. This is different from ..StringAndSize, where the object is allocated with PyObject_New which doesn't zero-initialize the struct.
| {stmt} | ||
| print(list(a)) | ||
| """) | ||
| rc, out, err = assert_python_ok('-c', code) |
There was a problem hiding this comment.
Why do we need to run this in script format, it adds additional overhead? Tests don't need to fail nicely, a crash is a crash.
Format blurb correctly, and reduce the chatter (There is a user-visible behaviour change, but I guess it's so minor it's not worth mentioning)
Introduce a bytearray_new function to ensure that
ob_bytes_object is always set on a bytearray.
Resizing a bytearray to 0 length explicitly sets
the ob_bytes_object to the empty constant immortal.
Add a check in the 'bytearray init from string'
fast path to ensure there are no active exports.
This fixes asserts/crashes on the following:
bytearray(1).__init__()bytearray().__new__(bytearray).append(1)a = bytearray(); b = memoryview(a); a.__init__('x', 'ascii')I realise that the explicit setting of bytes_object to
Py_CONSTANT_EMPTY_BYTESis not strictly necessary, as this is also done by the_PyBytes_Resizetoo, but this seemed to mark the intent of what's happening more clearly, given we rely and assert on that exact behaviour.