X Tutup
The Wayback Machine - https://web.archive.org/web/20221223174107/https://github.com/python/cpython/pull/99902
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

Jump to bottom

[3.11] gh-99886: Fix crash when freeing objects with managed dictionaries #99902

Merged
merged 7 commits into from Dec 4, 2022
Merged

[3.11] gh-99886: Fix crash when freeing objects with managed dictionaries #99902

merged 7 commits into from Dec 4, 2022

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Nov 30, 2022 •
edited by bedevere-bot

@bedevere-bot bedevere-bot mentioned this pull request Nov 30, 2022
Closed
@Fidget-Spinner Fidget-Spinner changed the title gh-99886: Fix crash when freeing objects with managed dictionaries [3.11] gh-99886: Fix crash when freeing objects with managed dictionaries Nov 30, 2022
markshannon
markshannon reviewed Nov 30, 2022
View changes
Copy link
Member

@markshannon markshannon left a comment

We should probably remove the reference to the values array, before we clear it.

Objects/dictobject.c Outdated Show resolved Hide resolved
erlend-aasland
erlend-aasland reviewed Nov 30, 2022
View changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

I don't know the internal parts good enough to comment on them; I trust you here :) The test looks ok, but perhaps we should add a more general test case; buildbots without SQLite installed won't have the sqlite3 module, so this test won't run.

Lib/test/test_sqlite3/test_regression.py Outdated Show resolved Hide resolved
@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Dec 1, 2022

... perhaps we should add a more general test case; buildbots without SQLite installed won't have the sqlite3 module, so this test won't run.

I agree. However I'm not sure what exactly is causing this. My general idea is that something is causing the tp_traverse function to be called multiple times on the same instance, but I don't know why inheriting from sqlite.Cursor causes this. I will try to create a test case without sqlite but it's possible that I won't be able to find it before the next 3.11 release :(. Sorry.

Fidget-Spinner and others added 2 commits Dec 1, 2022
@erlend-aasland
Copy link
Contributor

erlend-aasland commented Dec 2, 2022

I agree. However I'm not sure what exactly is causing this. My general idea is that something is causing the tp_traverse function to be called multiple times on the same instance, but I don't know why inheriting from sqlite.Cursor causes this. I will try to create a test case without sqlite but it's possible that I won't be able to find it before the next 3.11 release :(. Sorry.

No sweat! Thanks so much for the quick fix!

kumaraditya303
kumaraditya303 approved these changes Dec 2, 2022
View changes
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

LGTM

@Fidget-Spinner Fidget-Spinner merged commit 9e38553 into python:3.11 Dec 4, 2022
15 checks passed
@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Dec 4, 2022

Thanks for the reviews, Erlend and Kumar!

@Fidget-Spinner Fidget-Spinner deleted the fix_sqlite_crash branch Dec 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markshannon markshannon

@erlend-aasland erlend-aasland

@kumaraditya303 kumaraditya303

@berkerpeksag berkerpeksag

@methane methane

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Fidget-Spinner @erlend-aasland @markshannon @kumaraditya303 @bedevere-bot
X Tutup