From a8c1464f013b06db895d3b1d1ff5987691cf5a3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Wed, 12 Jun 2024 16:58:29 +0200 Subject: [PATCH 01/13] add regression test --- Lib/test/pickletester.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index 93e7dbbd1039342..e2a9772101eae30 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -1866,11 +1866,22 @@ def test_bytearray(self): def test_bytearray_memoization_bug(self): for proto in protocols: - for s in b'', b'xyz', b'xyz'*100: - b = bytearray(s) - p = self.dumps((b, b), proto) - b1, b2 = self.loads(p) - self.assertIs(b1, b2) + for array_type in [bytearray, ZeroCopyBytes, ZeroCopyBytearray]: + for s in b'', b'xyz', b'xyz'*100: + b = array_type(s) + p = self.dumps((b, b), proto) + b1, b2 = self.loads(p) + self.assertIs(b1, b2) + + b1a, b2a = array_type(s), array_type(s) + p = self.dumps((b1a, b2a), proto) + b1b, b2b = self.loads(p) + + self.assertIsNot(b1a, b1b) + self.assert_is_copy(b1a, b1b) + + self.assertIsNot(b2a, b2b) + self.assert_is_copy(b2a, b2b) def test_ints(self): for proto in protocols: From f771a42772e2bdfd0580a80cf7b9ce78618a631c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Wed, 12 Jun 2024 18:11:56 +0200 Subject: [PATCH 02/13] fix pickle's Python implementation of protocol buffer --- Lib/pickle.py | 48 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/Lib/pickle.py b/Lib/pickle.py index 33c97c8c5efb28e..24d0511ae14ce0f 100644 --- a/Lib/pickle.py +++ b/Lib/pickle.py @@ -782,14 +782,9 @@ def save_float(self, obj): self.write(FLOAT + repr(obj).encode("ascii") + b'\n') dispatch[float] = save_float - def save_bytes(self, obj): - if self.proto < 3: - if not obj: # bytes object is empty - self.save_reduce(bytes, (), obj=obj) - else: - self.save_reduce(codecs.encode, - (str(obj, 'latin1'), 'latin1'), obj=obj) - return + def __save_bytes_aux(self, obj): + # helper for writing bytearray objects for protocol >= 3 + assert self.proto >= 3 n = len(obj) if n <= 0xff: self.write(SHORT_BINBYTES + pack("= 5 + assert self.proto >= 5 + n = len(obj) + if n >= self.framer._FRAME_SIZE_TARGET: + self._write_large_bytes(BYTEARRAY8 + pack("= self.framer._FRAME_SIZE_TARGET: - self._write_large_bytes(BYTEARRAY8 + pack(" Date: Wed, 12 Jun 2024 18:13:25 +0200 Subject: [PATCH 03/13] add test coverage --- Lib/test/pickletester.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index e2a9772101eae30..0304bf6feee44e4 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -1845,6 +1845,23 @@ def test_bytes(self): p = self.dumps(s, proto) self.assert_is_copy(s, self.loads(p)) + def test_bytes_memoization(self): + for proto in protocols: + for array_type in [bytes, ZeroCopyBytes]: + for s in b'', b'xyz', b'xyz'*100: + b = array_type(s) + p = self.dumps((b, b), proto) + x, y = self.loads(p) + self.assertIs(x, y) + self.assert_is_copy((b, b), (x, y)) + + b1, b2 = array_type(s), array_type(s) + p = self.dumps((b1, b2), proto) + # Note that (b1, b2) = self.loads(p) might have identical + # components, i.e., b1 is b2, but this is not always the + # case if the content is large (equality still holds). + self.assert_is_copy((b1, b2), self.loads(p)) + def test_bytearray(self): for proto in protocols: for s in b'', b'xyz', b'xyz'*100: @@ -1864,9 +1881,9 @@ def test_bytearray(self): self.assertNotIn(b'bytearray', p) self.assertTrue(opcode_in_pickle(pickle.BYTEARRAY8, p)) - def test_bytearray_memoization_bug(self): + def test_bytearray_memoization(self): for proto in protocols: - for array_type in [bytearray, ZeroCopyBytes, ZeroCopyBytearray]: + for array_type in [bytearray, ZeroCopyBytearray]: for s in b'', b'xyz', b'xyz'*100: b = array_type(s) p = self.dumps((b, b), proto) @@ -1877,6 +1894,8 @@ def test_bytearray_memoization_bug(self): p = self.dumps((b1a, b2a), proto) b1b, b2b = self.loads(p) + # Unlike bytes, bytearray objects are never identical, + # even if they have an empty data or a short data. self.assertIsNot(b1a, b1b) self.assert_is_copy(b1a, b1b) From e2d64f131f4d6ff7595f69d4384c963b5ee25c81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Wed, 12 Jun 2024 18:23:20 +0200 Subject: [PATCH 04/13] blurb --- .../2024-06-12-18-23-15.gh-issue-120380.edtqjq.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-06-12-18-23-15.gh-issue-120380.edtqjq.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-06-12-18-23-15.gh-issue-120380.edtqjq.rst b/Misc/NEWS.d/next/Core and Builtins/2024-06-12-18-23-15.gh-issue-120380.edtqjq.rst new file mode 100644 index 000000000000000..c682a0b7666416f --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-06-12-18-23-15.gh-issue-120380.edtqjq.rst @@ -0,0 +1,3 @@ +Fix Python implementation of :class:`pickle.Pickler` for :class:`bytes` and +:class:`bytearray` objects when using protocol version 5. Patch by Bénédikt +Tran. From 7261a62ff94a01a560a11697d75aa23d65128d37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Wed, 12 Jun 2024 18:32:34 +0200 Subject: [PATCH 05/13] fix typo --- Lib/pickle.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/pickle.py b/Lib/pickle.py index 24d0511ae14ce0f..975e967c6f0bf0e 100644 --- a/Lib/pickle.py +++ b/Lib/pickle.py @@ -783,7 +783,7 @@ def save_float(self, obj): dispatch[float] = save_float def __save_bytes_aux(self, obj): - # helper for writing bytearray objects for protocol >= 3 + # helper for writing bytes objects for protocol >= 3 assert self.proto >= 3 n = len(obj) if n <= 0xff: From caa7bab41f4c6758325affc943f734b40f415392 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Thu, 13 Jun 2024 10:17:09 +0200 Subject: [PATCH 06/13] fixup! variable usage --- Lib/pickle.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/pickle.py b/Lib/pickle.py index 975e967c6f0bf0e..8a256af68d81e74 100644 --- a/Lib/pickle.py +++ b/Lib/pickle.py @@ -853,7 +853,7 @@ def save_picklebuffer(self, obj): if in_memo: self.__save_bytearray_aux(buf) else: - self.save_bytearray(m.tobytes()) + self.save_bytearray(buf) else: # Write data out-of-band self.write(NEXT_BUFFER) From 13c969dd06c70d9f8680c8db0f1a901e96d40f6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Thu, 13 Jun 2024 10:17:23 +0200 Subject: [PATCH 07/13] fix test_pyclbr for double-underscore methods --- Lib/test/test_pyclbr.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_pyclbr.py b/Lib/test/test_pyclbr.py index 0c12a3085b12af0..ec5058e65f66b92 100644 --- a/Lib/test/test_pyclbr.py +++ b/Lib/test/test_pyclbr.py @@ -78,7 +78,7 @@ def ismethod(oclass, obj, name): objname = obj.__name__ if objname.startswith("__") and not objname.endswith("__"): - objname = "_%s%s" % (oclass.__name__, objname) + objname = "_%s%s" % (oclass.__name__.lstrip('_'), objname) return objname == name # Make sure the toplevel functions and classes are the same. @@ -115,8 +115,8 @@ def ismethod(oclass, obj, name): actualMethods.append(m) foundMethods = [] for m in value.methods.keys(): - if m[:2] == '__' and m[-2:] != '__': - foundMethods.append('_'+name+m) + if m.startswith('__') and not m.endswith('__'): + foundMethods.append(f"_{name.lstrip('_')}{m}") else: foundMethods.append(m) From 8b981b6e63fb9b4a63e1cd0a2a1fba71c132f69f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Thu, 13 Jun 2024 13:40:16 +0200 Subject: [PATCH 08/13] fixup cases when the class name is a repetition of '_' --- Lib/test/test_pyclbr.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/Lib/test/test_pyclbr.py b/Lib/test/test_pyclbr.py index ec5058e65f66b92..1282ba8ff64df18 100644 --- a/Lib/test/test_pyclbr.py +++ b/Lib/test/test_pyclbr.py @@ -78,7 +78,8 @@ def ismethod(oclass, obj, name): objname = obj.__name__ if objname.startswith("__") and not objname.endswith("__"): - objname = "_%s%s" % (oclass.__name__.lstrip('_'), objname) + if stripped_typename := oclass.__name__.lstrip('_'): + objname = f"_{stripped_typename}{objname}" return objname == name # Make sure the toplevel functions and classes are the same. @@ -113,12 +114,16 @@ def ismethod(oclass, obj, name): continue if ismethod(py_item, getattr(py_item, m), m): actualMethods.append(m) - foundMethods = [] - for m in value.methods.keys(): - if m.startswith('__') and not m.endswith('__'): - foundMethods.append(f"_{name.lstrip('_')}{m}") - else: - foundMethods.append(m) + + if stripped_typename := name.lstrip('_'): + foundMethods = [] + for m in value.methods.keys(): + if m.startswith('__') and not m.endswith('__'): + foundMethods.append(f"_{stripped_typename}{m}") + else: + foundMethods.append(m) + else: + foundMethods = list(value.methods.keys()) try: self.assertListEq(foundMethods, actualMethods, ignore) From 6c2e97525d50269f2f3e23fb88aa1a907639bfe5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Thu, 13 Jun 2024 13:50:23 +0200 Subject: [PATCH 09/13] update comment --- Lib/test/pickletester.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index 0304bf6feee44e4..adf8457e42ffd83 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -1891,11 +1891,14 @@ def test_bytearray_memoization(self): self.assertIs(b1, b2) b1a, b2a = array_type(s), array_type(s) + # Unlike bytes, equal but independent bytearray objects are + # never identical. + self.assertIsNot(b1a, b2a) + p = self.dumps((b1a, b2a), proto) b1b, b2b = self.loads(p) + self.assertIsNot(b1b, b2b) - # Unlike bytes, bytearray objects are never identical, - # even if they have an empty data or a short data. self.assertIsNot(b1a, b1b) self.assert_is_copy(b1a, b1b) From 45e09757d608eb54b5f7b428b58ebb3174a28cdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Tue, 18 Jun 2024 14:27:16 +0200 Subject: [PATCH 10/13] refactor names and comments --- Lib/pickle.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Lib/pickle.py b/Lib/pickle.py index 8a256af68d81e74..d719ceb7a0b8e88 100644 --- a/Lib/pickle.py +++ b/Lib/pickle.py @@ -782,8 +782,9 @@ def save_float(self, obj): self.write(FLOAT + repr(obj).encode("ascii") + b'\n') dispatch[float] = save_float - def __save_bytes_aux(self, obj): + def _save_bytes_no_memo(self, obj): # helper for writing bytes objects for protocol >= 3 + # without memoizing them assert self.proto >= 3 n = len(obj) if n <= 0xff: @@ -803,12 +804,13 @@ def save_bytes(self, obj): self.save_reduce(codecs.encode, (str(obj, 'latin1'), 'latin1'), obj=obj) return - self.__save_bytes_aux(obj) + self._save_bytes_no_memo(obj) self.memoize(obj) dispatch[bytes] = save_bytes - def __save_bytearray_aux(self, obj): + def _save_bytearray_no_memo(self, obj): # helper for writing bytearray objects for protocol >= 5 + # without memoizing them assert self.proto >= 5 n = len(obj) if n >= self.framer._FRAME_SIZE_TARGET: @@ -823,7 +825,7 @@ def save_bytearray(self, obj): else: self.save_reduce(bytearray, (bytes(obj),), obj=obj) return - self.__save_bytearray_aux(obj) + self._save_bytearray_no_memo(obj) self.memoize(obj) dispatch[bytearray] = save_bytearray @@ -846,12 +848,12 @@ def save_picklebuffer(self, obj): in_memo = id(buf) in self.memo if m.readonly: if in_memo: - self.__save_bytes_aux(buf) + self._save_bytes_no_memo(buf) else: self.save_bytes(buf) else: if in_memo: - self.__save_bytearray_aux(buf) + self._save_bytearray_no_memo(buf) else: self.save_bytearray(buf) else: From 7cb89eacaff9c8a2faf20d36463abe7221b03cac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Tue, 18 Jun 2024 14:41:24 +0200 Subject: [PATCH 11/13] improve debugging of tests --- Lib/test/pickletester.py | 66 +++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index adf8457e42ffd83..9922591ce7114af 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -1849,18 +1849,20 @@ def test_bytes_memoization(self): for proto in protocols: for array_type in [bytes, ZeroCopyBytes]: for s in b'', b'xyz', b'xyz'*100: - b = array_type(s) - p = self.dumps((b, b), proto) - x, y = self.loads(p) - self.assertIs(x, y) - self.assert_is_copy((b, b), (x, y)) - - b1, b2 = array_type(s), array_type(s) - p = self.dumps((b1, b2), proto) - # Note that (b1, b2) = self.loads(p) might have identical - # components, i.e., b1 is b2, but this is not always the - # case if the content is large (equality still holds). - self.assert_is_copy((b1, b2), self.loads(p)) + with self.subTest(proto=proto, array_type=array_type, s=s, independent=False): + b = array_type(s) + p = self.dumps((b, b), proto) + x, y = self.loads(p) + self.assertIs(x, y) + self.assert_is_copy((b, b), (x, y)) + + with self.subTest(proto=proto, array_type=array_type, s=s, independent=True): + b1, b2 = array_type(s), array_type(s) + p = self.dumps((b1, b2), proto) + # Note that (b1, b2) = self.loads(p) might have identical + # components, i.e., b1 is b2, but this is not always the + # case if the content is large (equality still holds). + self.assert_is_copy((b1, b2), self.loads(p)) def test_bytearray(self): for proto in protocols: @@ -1885,25 +1887,27 @@ def test_bytearray_memoization(self): for proto in protocols: for array_type in [bytearray, ZeroCopyBytearray]: for s in b'', b'xyz', b'xyz'*100: - b = array_type(s) - p = self.dumps((b, b), proto) - b1, b2 = self.loads(p) - self.assertIs(b1, b2) - - b1a, b2a = array_type(s), array_type(s) - # Unlike bytes, equal but independent bytearray objects are - # never identical. - self.assertIsNot(b1a, b2a) - - p = self.dumps((b1a, b2a), proto) - b1b, b2b = self.loads(p) - self.assertIsNot(b1b, b2b) - - self.assertIsNot(b1a, b1b) - self.assert_is_copy(b1a, b1b) - - self.assertIsNot(b2a, b2b) - self.assert_is_copy(b2a, b2b) + with self.subTest(proto=proto, array_type=array_type, s=s, independent=False): + b = array_type(s) + p = self.dumps((b, b), proto) + b1, b2 = self.loads(p) + self.assertIs(b1, b2) + + with self.subTest(proto=proto, array_type=array_type, s=s, independent=True): + b1a, b2a = array_type(s), array_type(s) + # Unlike bytes, equal but independent bytearray objects are + # never identical. + self.assertIsNot(b1a, b2a) + + p = self.dumps((b1a, b2a), proto) + b1b, b2b = self.loads(p) + self.assertIsNot(b1b, b2b) + + self.assertIsNot(b1a, b1b) + self.assert_is_copy(b1a, b1b) + + self.assertIsNot(b2a, b2b) + self.assert_is_copy(b2a, b2b) def test_ints(self): for proto in protocols: From 47ee6377df8d439c64455a78cde069e1965ce972 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Tue, 18 Jun 2024 14:43:22 +0200 Subject: [PATCH 12/13] Revert "fixup cases when the class name is a repetition of '_'" This reverts commit 236efc0360a8e1ccfb8890845c2e75944e836179. --- Lib/test/test_pyclbr.py | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/Lib/test/test_pyclbr.py b/Lib/test/test_pyclbr.py index 1282ba8ff64df18..ec5058e65f66b92 100644 --- a/Lib/test/test_pyclbr.py +++ b/Lib/test/test_pyclbr.py @@ -78,8 +78,7 @@ def ismethod(oclass, obj, name): objname = obj.__name__ if objname.startswith("__") and not objname.endswith("__"): - if stripped_typename := oclass.__name__.lstrip('_'): - objname = f"_{stripped_typename}{objname}" + objname = "_%s%s" % (oclass.__name__.lstrip('_'), objname) return objname == name # Make sure the toplevel functions and classes are the same. @@ -114,16 +113,12 @@ def ismethod(oclass, obj, name): continue if ismethod(py_item, getattr(py_item, m), m): actualMethods.append(m) - - if stripped_typename := name.lstrip('_'): - foundMethods = [] - for m in value.methods.keys(): - if m.startswith('__') and not m.endswith('__'): - foundMethods.append(f"_{stripped_typename}{m}") - else: - foundMethods.append(m) - else: - foundMethods = list(value.methods.keys()) + foundMethods = [] + for m in value.methods.keys(): + if m.startswith('__') and not m.endswith('__'): + foundMethods.append(f"_{name.lstrip('_')}{m}") + else: + foundMethods.append(m) try: self.assertListEq(foundMethods, actualMethods, ignore) From 118b541364c1ca3a02ad1cb1b78629ff5caf7e16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Tue, 18 Jun 2024 14:46:50 +0200 Subject: [PATCH 13/13] Revert "fix test_pyclbr for double-underscore methods" This reverts commit 13c969dd06c70d9f8680c8db0f1a901e96d40f6e. --- Lib/test/test_pyclbr.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_pyclbr.py b/Lib/test/test_pyclbr.py index ec5058e65f66b92..0c12a3085b12af0 100644 --- a/Lib/test/test_pyclbr.py +++ b/Lib/test/test_pyclbr.py @@ -78,7 +78,7 @@ def ismethod(oclass, obj, name): objname = obj.__name__ if objname.startswith("__") and not objname.endswith("__"): - objname = "_%s%s" % (oclass.__name__.lstrip('_'), objname) + objname = "_%s%s" % (oclass.__name__, objname) return objname == name # Make sure the toplevel functions and classes are the same. @@ -115,8 +115,8 @@ def ismethod(oclass, obj, name): actualMethods.append(m) foundMethods = [] for m in value.methods.keys(): - if m.startswith('__') and not m.endswith('__'): - foundMethods.append(f"_{name.lstrip('_')}{m}") + if m[:2] == '__' and m[-2:] != '__': + foundMethods.append('_'+name+m) else: foundMethods.append(m)