Skip to content

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

Merged
Fidget-Spinner merged 7 commits into
python:3.11from
Fidget-Spinner:fix_sqlite_crash
Dec 4, 2022
Merged

[3.11] gh-99886: Fix crash when freeing objects with managed dictionaries#99902
Fidget-Spinner merged 7 commits into
python:3.11from
Fidget-Spinner:fix_sqlite_crash

Conversation

@Fidget-Spinner

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

Copy link
Copy Markdown
Member

@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 reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread Objects/dictobject.c Outdated
erlend-aasland
erlend-aasland reviewed Nov 30, 2022
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Lib/test/test_sqlite3/test_regression.py Outdated
@Fidget-Spinner

Fidget-Spinner commented Dec 1, 2022

Copy link
Copy Markdown
Member Author

... 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 December 1, 2022 16:02
@erlend-aasland

erlend-aasland commented Dec 2, 2022

Copy link
Copy Markdown
Contributor

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 reviewed changes

@kumaraditya303 kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Fidget-Spinner
Fidget-Spinner merged commit 9e38553 into python:3.11 Dec 4, 2022
@Fidget-Spinner

Fidget-Spinner commented Dec 4, 2022

Copy link
Copy Markdown
Member Author

Thanks for the reviews, Erlend and Kumar!

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

Reviewers

@markshannon markshannon markshannon left review comments
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag berkerpeksag is a code owner
@methane methane Awaiting requested review from methane methane is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Fidget-Spinner @erlend-aasland @markshannon @kumaraditya303 @bedevere-bot