◐ Shell
reader mode source ↗
Skip to content

bpo-34543: Fix SystemErrors and segfaults with uninitialized Structs#14777

Closed
ZackerySpytz wants to merge 1 commit into
python:mainfrom
ZackerySpytz:bpo-34543-struct-crashes
Closed

bpo-34543: Fix SystemErrors and segfaults with uninitialized Structs#14777
ZackerySpytz wants to merge 1 commit into
python:mainfrom
ZackerySpytz:bpo-34543-struct-crashes

Conversation

@ZackerySpytz

@ZackerySpytz ZackerySpytz commented Jul 14, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@ZackerySpytz

ZackerySpytz commented Jul 14, 2019

Copy link
Copy Markdown
Contributor Author

This patch uses a CHECK_INITIALIZED() macro (like what is done in Modules/_io), but there are other ways to fix this issue.

aeros
aeros approved these changes Jul 15, 2019
View reviewed changes

@aeros aeros left a comment •
edited
Loading

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.

Hide comment

In order to test the changes, I attempted to recreate the segfault in the most current commit to cpython master and in your remote branch bpo-34543-struct-crashes.

cpython master:
image

image

PR branch:
image

For the version used for testing the latest cpython, I used the function test_segfault() on the second round in order to perform the test multiple times consecutively. The issue tracker reported similar problems with replication. The first round caused a TypeError, and the second one caused the segfault.

After performing test_segfault() 3 times consecutively in the PR's branch, ValueError was raised each time with the same message. As far as I can tell, this resolves the segfault issue and provides a significant improvement by raising a consistent exception each time.

Nicely done @ZackerySpytz, approved.

@aeros

aeros commented Jul 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Also, this should probably be backported to previous versions. The code sample I used was the same from 3.7 with no noticeable difference in behavior prior to this patch: https://bugs.python.org/msg324498.

@jdemeyer

jdemeyer commented Jul 17, 2019

Copy link
Copy Markdown
Contributor

Wouldn't it make more sense to ensure that the invalid objects can't be created in the first place, by doing the initialization in __new__ instead of __init__?

@dekrain dekrain mannequin mentioned this pull request Jul 3, 2022
Closed
@kumaraditya303

kumaraditya303 commented Nov 25, 2022

Copy link
Copy Markdown
Contributor

Superseded by #94532

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@aeros aeros aeros approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting core review type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ZackerySpytz @aeros @jdemeyer @kumaraditya303 @brettcannon @the-knights-who-say-ni @ezio-melotti @bedevere-bot