gh-146059: Call fast_save_leave() in pickle save_frozenset() by vstinner · Pull Request #146173 · python/cpython

vstinner · 2026-03-19T15:17:04Z

Issue: Fatal Python error when using _pickle with many frozenset objects #146059

vstinner

vstinner · 2026-03-19T15:17:41Z

Lib/test/pickletester.py

+                    buf = io.BytesIO()
+                    pickler = self.pickler(buf, protocol=proto)
+                    # Enable fast mode (disables memo, enables cycle detection)
+                    pickler.fast = 1


Would it be also worth it to test non-fast mode? The bug only triggers in fast mode.

serhiy-storchaka

Please add also tests for list, set, dict, tuple and frozendict.

serhiy-storchaka

Also, this mechanism is used to guard against deeply nested structures, so it would be worth to test deeply nested structures.

There are sets of tests for recursive structures. We can use similar tests for limited but deep recursion.

Lib/test/pickletester.py

vstinner · 2026-03-20T11:44:28Z

@serhiy-storchaka: Please review the updated PR. Is it what you expected?

Also, this mechanism is used to guard against deeply nested structures, so it would be worth to test deeply nested structures.

I added deep_nested_struct() tests for the 6 types.

More Pythonic way would be to make tested_type and data parameters and call the function with different arguments.

Good idea, I rewrote tests like that.

vstinner · 2026-03-24T11:14:21Z

Ping @serhiy-storchaka.

miss-islington-app · 2026-03-26T16:35:35Z

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

…ythonGH-146173) Add more pickle tests: test also nested structures. (cherry picked from commit 5c0dcb3e0d817bd8c28e8efcdb97103cd9210989) Co-authored-by: Victor Stinner <[email protected]>

…ythonGH-146173) Add more pickle tests: test also nested structures. (cherry picked from commit 5c0dcb3) Co-authored-by: Victor Stinner <[email protected]>

bedevere-app · 2026-03-26T16:35:50Z

GH-146473 is a backport of this pull request to the 3.14 branch.

bedevere-app · 2026-03-26T16:35:55Z

GH-146474 is a backport of this pull request to the 3.13 branch.

…H-146173) (#146473) gh-146059: Call fast_save_leave() in pickle save_frozenset() (GH-146173) Add more pickle tests: test also nested structures. (cherry picked from commit 5c0dcb3) Co-authored-by: Victor Stinner <[email protected]>

serhiy-storchaka

LGTM. 👍

I only have some minor questions.

serhiy-storchaka · 2026-03-26T17:36:57Z

Lib/test/pickletester.py

+            self.skipTest("need Pickler class")
+
+        data = [create_data(i) for i in range(FAST_NESTING_LIMIT * 2)]
+        data = {"key": data}


Why is this needed?

Ah, it comes from the initial reproducer. But I confirm that I can still reproduce the bug/crash without {"key": data}. I removed fast_save_leave() from save_list(), save_dict() and save_frozenset(), and the test fails or crash.

serhiy-storchaka · 2026-03-26T17:37:49Z

Lib/test/pickletester.py

                    self.assertIn(expected, str(e))

+    def fast_save_enter(self, create_data, minprotocol=0):
+        # gh-146059: Check that fast_save() is called when


Did you mean fast_save_leave()?

Oops, you're right.

serhiy-storchaka · 2026-03-26T17:39:31Z

Lib/test/pickletester.py

+        data = seed
+        for i in range(depth):
+            data = create_nested(data)
+        data = {"key": data}


Why is this needed?

serhiy-storchaka · 2026-03-26T17:40:24Z

Lib/test/pickletester.py

+    def test_fast_save_enter_dict(self):
+        self.fast_save_enter(lambda i: {"key": i})
+
+    def deep_nested_struct(self, seed, create_nested,


Why is the seed parameter used? Can't we use the same seed (None) for all tests?

I looked nice to me to have a seed of the same type than the nested structure, but you're right, it makes the test too complex, it's not worth it. Also, I confirm that I can still trigger the bug (when removing fast_save_leave() manually in pickle) with None seed.

vstinner · 2026-03-26T18:30:43Z

LGTM. 👍 I only have some minor questions.

Thanks for your review. I created #146481 to address your review.

…H-146173) (#146474) gh-146059: Call fast_save_leave() in pickle save_frozenset() (GH-146173) Add more pickle tests: test also nested structures. (cherry picked from commit 5c0dcb3) Co-authored-by: Victor Stinner <[email protected]>

pythongh-146059: Call fast_save_leave() in pickle save_frozenset()

c86e3d0

vstinner added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Mar 19, 2026

bedevere-app bot mentioned this pull request Mar 19, 2026

Fatal Python error when using _pickle with many frozenset objects #146059

Closed

bedevere-app bot added the awaiting core review label Mar 19, 2026

vstinner commented Mar 19, 2026

View reviewed changes

vstinner added the skip news label Mar 19, 2026

serhiy-storchaka self-requested a review March 19, 2026 16:01

serhiy-storchaka reviewed Mar 19, 2026

View reviewed changes

Lib/test/pickletester.py Outdated Show resolved Hide resolved

vstinner added 2 commits March 20, 2026 12:37

Add more tests: test also nested structures

f8c93db

test_deep_nested_struct_set() uses K class

d7495d8

vstinner merged commit 5c0dcb3 into python:main Mar 26, 2026
48 checks passed

vstinner deleted the pickle_fast branch March 26, 2026 16:35

bedevere-app bot removed the awaiting core review label Mar 26, 2026

bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Mar 26, 2026

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Mar 26, 2026

serhiy-storchaka reviewed Mar 26, 2026

View reviewed changes

serhiy-storchaka mentioned this pull request Mar 26, 2026

PyDict_DelItem() can crash in some circumstances #146480

Closed

Uh oh!

Conversation

vstinner commented Mar 19, 2026 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Mar 20, 2026

Uh oh!

vstinner commented Mar 24, 2026

Uh oh!

Uh oh!

miss-islington-app bot commented Mar 26, 2026

Uh oh!

bedevere-app bot commented Mar 26, 2026

Uh oh!

bedevere-app bot commented Mar 26, 2026

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Mar 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vstinner commented Mar 19, 2026 •

edited by bedevere-app bot

Loading