gh-128974: Fix UnicodeError.__str__ when custom attributes have side-effects
#128975
Conversation
|
@vstinner I plan to merge this one with no commit message and the PR's title as the commit title. Do you want me to expand on the issue itself in the commit message or do you think it's fine to leave it in the issue's description? (we can both do a UAF and an invalid type assertion) |
| @@ -3379,7 +3399,10 @@ UnicodeEncodeError_str(PyObject *self) | |||
| if (encoding_str == NULL) { | |||
| goto done; | |||
| } | |||
|
|
|||
| // calls to PyObject_Str(...) above might mutate 'exc->object' | |||
| if (check_unicode_error_attribute(exc->object, "object", false) < 0) { | |||
There was a problem hiding this comment.
IMO this check is done too late. I would prefer to disallow creating inconsistent exception object, and so implement such check in UnicodeEncodeError_init() instead.
There was a problem hiding this comment.
The problem is that it's possible to override exc.object in Python. A setter function is needed here to also check when the attribute is set.
There was a problem hiding this comment.
IMO this check is done too late
Agreed, but I was more worried about the crash itself rather than whether the API was correct in the first place or not. I can add a setter function if you want but I haven't looked at whether this would introduce other corner cases or not and how it would interact with the rest.
|
I'm planning to merge this one, considering I had Petr's approval. While I understand the concern of the API itself, I think this should be addressed separately. |
|
I don't think I'll backport the change because the structure of |
|
For an old bug that was found by code inspection, rather than by real-world use, I think it's fine to not backport. |
UnicodeError.__str__with attributes have custom__str__#128974