Skip to content
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

gh-92930: _pickle.c: Acquire strong references before calling save() #92931

Merged
merged 11 commits into from Jun 11, 2022

Conversation

sweeneyde
Copy link
Member

@sweeneyde sweeneyde commented May 18, 2022

@sweeneyde sweeneyde requested a review from serhiy-storchaka May 18, 2022
@sweeneyde sweeneyde changed the title gh-92930: pickle.c: Acquire strong references after PyDict_Next gh-92930: _pickle.c: Acquire strong references after PyDict_Next May 18, 2022
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

LGTM. There may be a similar issue in batch_list_exact. Do you mind to fix it in the same PR?

There is also a suspicious place in load_build. I am not sure that it is reproducible.

@sweeneyde
Copy link
Member Author

@sweeneyde sweeneyde commented May 19, 2022

I couldn't figure out how to make a crashing test for batch_list_exact. For that case, save() seems to assume that its parameter obj is borrowed, so I think it is safe, but I'm not 100% sure.

The reason that batch_dict_exact crashes is because save(self, key, 0) clears the dict and leaves value as junk, and then save(self, value, 0) crashes, so I think it is only a problem two calls to save() in a row.

Is it worth adding Py_INCREF to make it more obviously correct?

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented May 20, 2022

What if persistent_id() clears the list and returns None?

@sweeneyde
Copy link
Member Author

@sweeneyde sweeneyde commented Jun 10, 2022

You're right, here's a test case that segfaults:

import pickle
import io

class P(pickle.Pickler):
    def persistent_id(self, obj):
        if obj is a[0]:
            a.clear()
        return None

a = [[[[]]]]
P(io.BytesIO()).dump(a)

Updating the PR soon.

@sweeneyde sweeneyde changed the title gh-92930: _pickle.c: Acquire strong references after PyDict_Next gh-92930: _pickle.c: Acquire strong references before calling save() Jun 10, 2022
@sweeneyde sweeneyde requested a review from serhiy-storchaka Jun 10, 2022
@sweeneyde
Copy link
Member Author

@sweeneyde sweeneyde commented Jun 11, 2022

No refleaks

.\python.bat -m test test_pickle -R3:3
Running Debug|x64 interpreter...
0:00:00 Run tests sequentially
0:00:00 [1/1] test_pickle
beginning 6 repetitions
123456
......
test_pickle passed in 3 min 37 sec

@sweeneyde sweeneyde merged commit 4c496f1 into python:main Jun 11, 2022
13 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jun 11, 2022

Thanks @sweeneyde for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 11, 2022
…ave() (pythonGH-92931)

(cherry picked from commit 4c496f1)

Co-authored-by: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 11, 2022

GH-93707 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 11, 2022
…ave() (pythonGH-92931)

(cherry picked from commit 4c496f1)

Co-authored-by: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 11, 2022

GH-93708 is a backport of this pull request to the 3.10 branch.

@sweeneyde sweeneyde deleted the picklecrasher branch Jun 11, 2022
miss-islington added a commit that referenced this issue Jun 11, 2022
…H-92931)

(cherry picked from commit 4c496f1)

Co-authored-by: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com>
miss-islington added a commit that referenced this issue Jun 11, 2022
…H-92931)

(cherry picked from commit 4c496f1)

Co-authored-by: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants