GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-34543: Fix SystemErrors and segfaults with uninitialized Structs by ZackerySpytz · Pull Request #14777 · python/cpython

Conversation

@ZackerySpytz

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue34543

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ZackerySpytz
bpo-34543: Fix SystemErrors and segfaults with uninitialized Structs
a38f914
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jul 14, 2019
@bedevere-bot bedevere-bot added the awaiting review label Jul 14, 2019
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aeros
aeros approved these changes Jul 15, 2019
View reviewed changes

@aeros aeros left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Jul 15, 2019
aeros
aeros reviewed Jul 15, 2019
View reviewed changes
Comment thread Lib/test/test_struct.py
for meth in s.iter_unpack, s.pack, s.unpack, s.unpack_from:
self.assertRaises(ValueError, meth, b'0')
self.assertRaises(ValueError, s.pack_into, bytearray(1), 0, b'0')
self.assertRaises(ValueError, s.__sizeof__)

@aeros aeros Jul 15, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Also, realized this after submitting the approval. This is quite minor, and I still approve of the PR either way. Instead of using s.__sizeof__, I would recommend using sys.getsizeof(s):

Suggested change
self.assertRaises(ValueError, s.__sizeof__)
self.assertRaises(ValueError, sys.getsizeof(s))

In general, it seems to be preferable to use functions instead of directly accessing the special object attributes when possible. If you had a specific reason for not using sys.getsizeof(), let me know. Here's the a link to the function defintion and the docs. I looked over it, but I'm not very experienced with the python c-api. I mostly rely on the docs when it comes to the modules implemented in c.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@aeros

aeros commented Jul 15, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon brettcannon added the type-bug An unexpected behavior, bug, or error label Jul 15, 2019
@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__?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dekrain dekrain mannequin mentioned this pull request Jul 3, 2022
_struct.Struct: calling functions without calling __init__ results in SystemError #78724
Closed
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@kumaraditya303

kumaraditya303 commented Nov 25, 2022

Copy link
Copy Markdown
Contributor

Superseded by #94532
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303 kumaraditya303 closed this Nov 25, 2022
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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@ZackerySpytz @aeros @jdemeyer @kumaraditya303 @brettcannon @the-knights-who-say-ni @ezio-melotti @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.