bpo-41295: Reimplement the Carlo Verre "hackcheck" #21528
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… backwards to find the type that originally defined the final tp_setattro, then make sure we are not jumping over intermediate C-level bases with the Python-level call.
gvanrossum
reviewed
Jul 18, 2020
Lib/test/test_descr.py
Outdated
| @@ -4308,6 +4308,42 @@ def test_carloverre(self): | |||
| else: | |||
| self.fail("Carlo Verre __delattr__ succeeded!") | |||
|
|
|||
| def test_carloverre_multiinherit_valid(self): | |||
There was a problem hiding this comment.
I'd add a _ between multi and inherit.
Objects/typeobject.c
Outdated
|
|
||
| /* Reject calls that jump over intermediate C-level overrides. */ | ||
| PyTypeObject *base = defining_type; | ||
| while (base && base->tp_setattro != func) { |
There was a problem hiding this comment.
I wonder if this would read better if you wrote it as
for (PyTypeObject *base = defining_type; base != NULL; base = base->tp_base) {
if (base->tp_setattro == func) {
/* appropriate comment (same as old code?) */
break;
}
...
(That for-loop is IMO a more idiomatic way to iterate over a linked list.)
Objects/typeobject.c
Outdated
| @@ -5977,9 +5989,8 @@ hackcheck(PyObject *self, setattrofunc func, const char *what) | |||
| type->tp_name); | |||
| return 0; | |||
| } | |||
| base = base->tp_base; | |||
There was a problem hiding this comment.
(If you follow my suggestion this should go of course.)
|
Thanks for the review, works for me. |
|
Do you expect this will require a manual 3.8 backport again? |
|
@scoder: Status check is done, and it's a success ✅ . |
|
Thanks @scoder for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Jul 18, 2020
Walk down the MRO backwards to find the type that originally defined the final `tp_setattro`, then make sure we are not jumping over intermediate C-level bases with the Python-level call. Automerge-Triggered-By: @gvanrossum (cherry picked from commit c53b310) Co-authored-by: scoder <stefan_ml@behnel.de>
|
GH-21540 is a backport of this pull request to the 3.9 branch. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Jul 18, 2020
Walk down the MRO backwards to find the type that originally defined the final `tp_setattro`, then make sure we are not jumping over intermediate C-level bases with the Python-level call. Automerge-Triggered-By: @gvanrossum (cherry picked from commit c53b310) Co-authored-by: scoder <stefan_ml@behnel.de>
|
GH-21541 is a backport of this pull request to the 3.8 branch. |
The only issue was the test module in C last time. The algorithm and the Python tests applied cleanly to Py3.8. Miss Islington already did her job for this PR. |
miss-islington
added a commit
that referenced
this pull request
Jul 18, 2020
Walk down the MRO backwards to find the type that originally defined the final `tp_setattro`, then make sure we are not jumping over intermediate C-level bases with the Python-level call. Automerge-Triggered-By: @gvanrossum (cherry picked from commit c53b310) Co-authored-by: scoder <stefan_ml@behnel.de>
miss-islington
added a commit
that referenced
this pull request
Jul 18, 2020
Walk down the MRO backwards to find the type that originally defined the final `tp_setattro`, then make sure we are not jumping over intermediate C-level bases with the Python-level call. Automerge-Triggered-By: @gvanrossum (cherry picked from commit c53b310) Co-authored-by: scoder <stefan_ml@behnel.de>
|
arun-mani-j
pushed a commit
to arun-mani-j/cpython
that referenced
this pull request
Jul 21, 2020
Walk down the MRO backwards to find the type that originally defined the final `tp_setattro`, then make sure we are not jumping over intermediate C-level bases with the Python-level call. Automerge-Triggered-By: @gvanrossum
ap--
added a commit
to Bayer-Group/paquo
that referenced
this pull request
Jul 23, 2020
Python 3.8.4 had a regression on __setattr__ in multi inheritance with metaclasses, which causes jpype to crash on import. see: python/cpython#21528 (py39) which was backported to 3.8 This excludes python 3.8.4 from paquo's supported pythons.
shihai1991
pushed a commit
to shihai1991/cpython
that referenced
this pull request
Aug 4, 2020
Walk down the MRO backwards to find the type that originally defined the final `tp_setattro`, then make sure we are not jumping over intermediate C-level bases with the Python-level call. Automerge-Triggered-By: @gvanrossum
|
This change has broken some code for us, where we want to skip over a C level definition of It is probably not a super big deal for us, but wanted to have a trace here that this change has broken more stuff that is still "broken" (fixed?) in 3.8.5. |
|
Couldn’t you use |
|
Nope. Same error. And even if this solved it in this case, we do have general purpose code that tries |
|
Yes, that's exactly what the check prevents: reaching behind the back of a superclass implemented in C. Presumably you understand the reason why this is unsafe in general. I don't know why your framework was doing this (getting around an overly restrictive |
|
I don't really understand no. To me it's strange that classes with C code somewhere are now more different than before. If it's unsafe for C classes why is it safe for python classes? The release notes say this:
This sounds like it would be more allowing ("allow C implemented heap types"), while its actually more restrictive. |
shihai1991
pushed a commit
to shihai1991/cpython
that referenced
this pull request
Aug 20, 2020
Walk down the MRO backwards to find the type that originally defined the final `tp_setattro`, then make sure we are not jumping over intermediate C-level bases with the Python-level call. Automerge-Triggered-By: @gvanrossum
|
Read about it here: https://mail.python.org/pipermail/python-dev/2003-April/034535.html |
|
I've read that but it seems to me that the standard library and all C implemented classes are very different things? (Plus what about us being consenting adults?) Anyway, we'll work around this. I just wanted to give a heads up that there might be more projects that will hit this when they upgrade in months or even years. |
|
Consenting adults shouldn't be able to overwrite arbitrary memory (except through specific APIs intended to do so, i.e. ctypes). If a C type (whether a stdlib type or a user-define type) defines And even for Python classes it's bad style -- but by tradition we don't care enough to forbid it, since the worst you can get is a traceback. And yes, I understand that we're breaking a small amount of code. |
|
@boxed it looks to me like what you might have wanted at the time is something like |
xzy3
pushed a commit
to xzy3/cpython
that referenced
this pull request
Oct 18, 2020
Walk down the MRO backwards to find the type that originally defined the final `tp_setattro`, then make sure we are not jumping over intermediate C-level bases with the Python-level call. Automerge-Triggered-By: @gvanrossum
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Walk down the MRO backwards to find the type that originally defined the final
tp_setattro, then make sure we are not jumping over intermediate C-level bases with the Python-level call.https://bugs.python.org/issue41295
Automerge-Triggered-By: @gvanrossum