Repository navigation
bpo-34543: Fix SystemErrors and segfaults with uninitialized Structs - #14777
ZackerySpytz wants to merge 1 commit into
Conversation
Sorry, something went wrong.
|
This patch uses a CHECK_INITIALIZED() macro (like what is done in Modules/_io), but there are other ways to fix this issue. |
Sorry, something went wrong.
There was a problem hiding this 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:
PR branch:
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.
| 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__) |
There was a problem hiding this comment.
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):
| 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.
|
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. |
Sorry, something went wrong.
|
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__? |
Sorry, something went wrong.
https://bugs.python.org/issue34543