Uh oh!
There was an error while loading. Please reload this page.
gh-123378: fix some corner cases of start and end values in PyUnicodeErrorObject - #123380
Conversation
PyUnicode{Encode,Decode}Error_GetStartstart values on PyUnicodeError objectsstart values on PyUnicodeError objectsstart values on PyUnicodeErrorObjectUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
picnixz
commented
Aug 29, 2024
Thank you Victor for your review! |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner
left a comment
There was a problem hiding this comment.
LGTM.
@serhiy-storchaka: Do you want to double check the fix?
encukou
left a comment
There was a problem hiding this comment.
I'm a bit sad that we can't agree to fix the setters, but, if we're fixing the C getters, this is the way to go.
A few nitpicks:
Uh oh!
There was an error while loading. Please reload this page.
| static inline int | ||
| unicode_error_set_end_impl(PyObject *exc, Py_ssize_t end) | ||
| { | ||
| ((PyUnicodeErrorObject *)exc)->end = end; |
There was a problem hiding this comment.
While you're here, could you add a type check before each cast?
assert(PyObject_TypeCheck(exc, (PyTypeObject*)&PyExc_UnicodeError));
Ideally, public API would raise TypeError on wrong types, but that change might be too big.
There was a problem hiding this comment.
Should we make a follow-up PR for adding assertions on the getters as well?
There was a problem hiding this comment.
That would be great! Thank you for volunteering :)
Uh oh!
There was an error while loading. Please reload this page.
| static inline int | ||
| unicode_error_set_end_impl(PyObject *exc, Py_ssize_t end) | ||
| { | ||
| ((PyUnicodeErrorObject *)exc)->end = end; |
There was a problem hiding this comment.
That would be great! Thank you for volunteering :)
encukou
commented
Dec 3, 2024
Feel free to leave all the asserts to a follow-up PR. |
encukou
commented
Dec 4, 2024
Thank you! |
picnixz
commented
Dec 5, 2024
Question: should we backport this to 3.12 and 3.13? it's both a feature and a bugfix in some sense but I feel it's more a bug fix even though it could change the behaviour of some code (hopefully, not too much...). The WDYT? By the way, I spotted some typos here and there. I'll correct them as part of a follow-up PR. |
encukou
commented
Dec 6, 2024
If I understand correctly, it could only return -1 with an empty string, which isn't something one would typically get encoding errors for. Also, at this level you get a wrong value rather than a crash. |
picnixz
commented
Dec 6, 2024
Yup. (to be clear, it's the size that is being stored in In this case, subsequent calls usually lead to crashes because the C API assumes non-relative offsets in general.
Thanks, it's essentially to know whether the patches for codecs handlers will be backported or not (without the backport of this PR, the handlers would still be broken I think). |
…re clamped (pythonGH-123380) Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
startvalues onPyUnicodeErrorObject#123378