gh-156995: Fix bytearray.take_bytes() corrupting shared single-byte bytes objects

[StanFromIreland]

• Issue: bytearray.take_bytes() can corrupt shared single-byte bytes objects #156995

[cmaloney]

Double checking against implementation this change looks correct.

I think should add an assert as a safety net to help catch this bug into bytearray_reinit_from_bytes which is called every time self->ob_bytes_object is set to re-init.

bytearray_reinit_from_bytes(PyByteArrayObject *self, Py_ssize_t size,
                            Py_ssize_t alloc)
{
    /* Only the empty bytes may be immortal. */
    assert((alloc == 0) == _Py_IsImmortal(self->ob_bytes_object));

That safety net in free-threaded builds finds an additional case that can happen. _PyBytes_Resize uses _PyObject_IsUniquelyReferenced which under free threading includes that the object is owned by the current thread. If another thread swapped out the underlying bytes the unquiely reference returns false. if it is a downsize that calls PyBytes_FromStringAndSize which passes non-NULL to its first parameter...

Not sure the best way to fix that... I'd lean that _PyBytes_Resize should never return an immutable object for size >= 1 as that can lead to a really subtle only under free-threading bug...

[StanFromIreland]

Thanks for the review @cmaloney, and nice catch! I've applied both of your suggestions.

[cmaloney]

cc: @vstinner would be good to get more eyes on the _PyBytes_Resize part in particular as this would be nice to get into 3.15

[StanFromIreland]

I agree we should backport, I worry this can be quite nasty, and it's not an unrealistic scenario.

[vstinner]

The overall change LGTM, but I would prefer to revert the _PyBytes_Resize() change from this PR.

[vstinner]

Please ignore my previous comment: the _PyBytes_Resize() change is needed by this fix. test_bytes fails (in debug mode) without this change.

I suggest to complete the _PyBytes_Resize() C API to check that the result is a fresh object:

diff --git a/Lib/test/test_capi/test_bytes.py b/Lib/test/test_capi/test_bytes.py
index 38cda931e7d..590e118c04e 100644
--- a/Lib/test/test_capi/test_bytes.py
+++ b/Lib/test/test_capi/test_bytes.py
@@ -1,3 +1,4 @@
+import sys
 import unittest
 from test.support import import_helper
 
@@ -231,26 +232,41 @@ def test_decodeescape(self):
 
     def test_resize(self):
         """Test _PyBytes_Resize()"""
-        resize = _testcapi.bytes_resize
+        _resize = _testcapi.bytes_resize
+
+        def resize(obj, size, new):
+            result = _resize(obj, size, new)
+            if 1 <= len(result):
+                if new or size != len(obj):
+                    # gh-156995: Make sure that the result is a fresh object.
+                    # Previously, _PyBytes_Resize(&obj, 1) returned a singleton
+                    # if _PyObject_IsUniquelyReferenced() is false.
+                    self.assertEqual(sys.getrefcount(result), 1)
+                    self.assertFalse(sys._is_immortal(result))
+            else:
+                # check that the result is the empty bytes string singleton
+                self.assertTrue(sys._is_immortal(result))
+            return result
 
         for new in True, False:
-            self.assertEqual(resize(b'abc', 0, new), b'')
-            self.assertEqual(resize(b'abc', 1, new), b'a')
-            self.assertEqual(resize(b'abc', 2, new), b'ab')
-            self.assertEqual(resize(b'abc', 3, new), b'abc')
-            b = resize(b'abc', 4, new)
-            self.assertEqual(len(b), 4)
-            self.assertEqual(b[:3], b'abc')
-
-            self.assertEqual(resize(b'a', 0, new), b'')
-            self.assertEqual(resize(b'a', 1, new), b'a')
-            b = resize(b'a', 2, new)
-            self.assertEqual(len(b), 2)
-            self.assertEqual(b[:1], b'a')
-
-            self.assertEqual(resize(b'', 0, new), b'')
-            self.assertEqual(len(resize(b'', 1, new)), 1)
-            self.assertEqual(len(resize(b'', 2, new)), 2)
+            with self.subTest(new=new):
+                self.assertEqual(resize(b'abc', 0, new), b'')
+                self.assertEqual(resize(b'abc', 1, new), b'a')
+                self.assertEqual(resize(b'abc', 2, new), b'ab')
+                self.assertEqual(resize(b'abc', 3, new), b'abc')
+                b = resize(b'abc', 4, new)
+                self.assertEqual(len(b), 4)
+                self.assertEqual(b[:3], b'abc')
+
+                self.assertEqual(resize(b'a', 0, new), b'')
+                self.assertEqual(resize(b'a', 1, new), b'a')
+                b = resize(b'a', 2, new)
+                self.assertEqual(len(b), 2)
+                self.assertEqual(b[:1], b'a')
+
+                self.assertEqual(resize(b'', 0, new), b'')
+                self.assertEqual(len(resize(b'', 1, new)), 1)
+                self.assertEqual(len(resize(b'', 2, new)), 2)
 
         self.assertRaises(SystemError, resize, b'abc', -1, False)
         self.assertRaises(SystemError, resize, bytearray(b'abc'), 3, False)

[vstinner]

LGTM. Nice fix. Modified code is well tested by new tests.

[miss-islington-app]

Thanks @StanFromIreland for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.15.
🐍🍒⛏🤖

[bedevere-app]

GH-157260 is a backport of this pull request to the 3.15 branch.