GeezerVison is showing github.com — Incorrect handling of negative `start` values on `PyUnicodeErrorObject` · Issue #123378 · python/cpython
Incorrect handling of negative start values on PyUnicodeErrorObject #123378Labels 3.14bugs and security fixesbugs and security fixesinterpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)topic-C-APItype-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or errortype-crashA hard crash of the interpreter, possibly with a core dumpA hard crash of the interpreter, possibly with a core dump DescriptionBug report
Bug description:
Found when implementing #123343. We have:
int
PyUnicodeEncodeError_GetStart(PyObject *exc, Py_ssize_t *start)
{
Py_ssize_t size;
PyObject *obj = get_unicode(((PyUnicodeErrorObject *)exc)->object,
"object");
if (!obj)
return -1;
*start = ((PyUnicodeErrorObject *)exc)->start;
size = PyUnicode_GET_LENGTH(obj);
if (*start<0)
*start = 0; /*XXX check for values <0*/
if (*start>=size)
*start = size-1;
Py_DECREF(obj);
return 0;
}The line *start = size-1 might set start to -1 when start = 0, in which case this leads to assertion failures when the index is used normally.
CPython versions tested on:
CPython main branch
Operating systems tested on:
Linux
Linked PRs
Activitychanged the title [-]Incorrect handling of `PyUnicode{Decode,Encode}Error_GetStart` when `start = `[/-] [+]Incorrect handling of `PyUnicode{Decode,Encode}Error_GetStart` when `start = 0`[/+] on Aug 27, 2024changed the title [-]Incorrect handling of `PyUnicode{Decode,Encode}Error_GetStart` when `start = 0`[/-] [+]Incorrect handling of `PyUnicode{Decode,Encode}Error_GetStart` when `start = size = 0`[/+] on Aug 27, 2024serhiy-storchaka commented on Aug 27, 2024 This clipping for start and end is weird. It was here from the beginning, from adding support for codec error handling callbacks in 3aeb632 (gh-34615).
@doerwalter, сan you shed some light on this? I guess that the case with size == 0 was not considered, as it does not make much sense. But why not use range 0..size for both start and end?
cc @malemburg picnixz commented on Aug 27, 2024
This clipping for start and end is weird.
Yes, but I didn't want to change the way it was done. With start = size = 0, we should (at least) not set size = -1... I would have expected the clipping to be usable with self.object[self.start:self.end]. (I observed the weirdness in the tests...) malemburg commented on Aug 27, 2024 I'm not too familiar with this code, but from looking at it, it seems that the if-statements should be swapped:
if (*start>=size)
*start = size-1;
if (*start<0)
*start = 0; /*XXX check for values <0*/
It doesn't make sense to set *start to a negative value. Reacted by Serhiy Storchaka picnixz commented on Aug 27, 2024 I'll update the PR accordingly. Should I also reject calls that set Unicode*Error.start to negative values? currently we have
int
PyUnicodeDecodeError_SetStart(PyObject *exc, Py_ssize_t start)
{
((PyUnicodeErrorObject *)exc)->start = start;
return 0;
}so the call unconditionally succeeds. I wanted to return -1 if start is < 0 but I'm not sure if it could break existing code. There is no mention of start or end to be >= 0 (though, setting it to -1 and then retrieve it using GetStart would currently return 0...)
UnicodeError has attributes that describe the encoding or decoding error. For example, err.object[err.start:err.end] gives the particular invalid input that the codec failed on.
How should I proceed? serhiy-storchaka commented on Aug 28, 2024 It would be a double work if you leave it in the getter. We cannot use the check start < size in the setter, because object can be set after start, so the check in the setter can only be one-side. Splitting checks between the getter and the setter is not good. picnixz commented on Aug 28, 2024
We cannot use the check start < size in the setter
I wasn't thinking of doing this check. What I thought about was doing the check start < 0 and raise an exception if this is not the case. Because in the getter, we are not interpreting negative start indices as positive ones from the end (we just clamp them to 0).
So, I have various ideas:
- Idea 1: Leave the setter alone, and use what Marc-Andre suggested.
- Idea 2: Add a
start < 0 check in the setter and use what M-A. suggested.
- Idea 3: Leave the setter alone, interpret negative
start values as in Python.
Idea 3 would be a feature, though I don't think we should do it. However, we should document the C API to indicate that start should not be negative. We can enforce this condition by using idea 2. If you don't want to enforce the condition and silently clamp to 0, then I'd go for idea 1. doerwalter commented on Aug 28, 2024 That code is nearly 22 year old, so I can no longer remember what my intention back then was. But the code does not treat negative values as being relative to the end, but clips them to valid positive offset positions.
So for an empty string, the only reasonable start value is 0. start should never point outside of the string.
The simplest solution would probably be to fix the getter to never return -1.
(Fixing the setter would be possible, but since there's no PyUnicodeDecodeError_SetObject it isn't clear how replacing the object should be handled). picnixz commented on Aug 28, 2024 By the way, I found this:
>>> str(UnicodeEncodeError('utf-8', '', -1, 0, ''))
Fatal Python error: _Py_CheckFunctionResult: a function returned a result with an exception set
Python runtime state: initialized
IndexError: string index out of range
The above exception was the direct cause of the following exception:
SystemError: <class 'str'> returned a result with an exception set
Current thread 0x00007f6e8678e740 (most recent call first):
File "<python-input-3>", line 1 in <module>
File "/lib/python/cpython/Lib/code.py", line 91 in runcode
File "/lib/python/cpython/Lib/_pyrepl/console.py", line 205 in runsource
File "/lib/python/cpython/Lib/code.py", line 312 in push
File "/lib/python/cpython/Lib/_pyrepl/simple_interact.py", line 157 in run_multiline_interactive_console
File "/lib/python/cpython/Lib/_pyrepl/main.py", line 59 in interactive_console
File "/lib/python/cpython/Lib/_pyrepl/__main__.py", line 6 in <module>
File "/lib/python/cpython/Lib/runpy.py", line 88 in _run_code
File "/lib/python/cpython/Lib/runpy.py", line 198 in _run_module_as_main
Aborted (core dumped)The start index should definitely be non-negative since it's being used without checks. We need to fix the setter by disallowing start to be negative in PyUnicode*Error_SetStart.
EDIT: we need to fix the constructor, not the setter actually in this case. doerwalter commented on Aug 28, 2024 Not neccesssarily. We might just have to update UnicodeEncodeError_str to use the getters instead of accessing the start directly.
But fixing the setters might be the safer option. picnixz commented on Oct 27, 2024 Ok, I've managed to crash or raise a SystemError other handlers. We should definitely do something:
./python -c "import codecs; codecs.backslashreplace_errors(UnicodeDecodeError('utf-8', b'00000', 9, 2, 'reason'))"
Traceback (most recent call last):
File "<string>", line 1, in <module>
import codecs; codecs.backslashreplace_errors(UnicodeDecodeError('utf-8', b'00000', 9, 2, 'reason'))
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
SystemError: Negative size passed to PyUnicode_New./python -c "import codecs; codecs.replace_errors(UnicodeTranslateError('000', 1, -7, 'reason'))"
python: Python/codecs.c:743: PyCodec_ReplaceErrors: Assertion `PyUnicode_KIND(res) == PyUnicode_2BYTE_KIND' failed.
Aborted (core dumped)I have some suggestions:
- we fix the handlers so that they only consider positive indices and return garbage otherwise (which could be a breaking change)
- same as above but handlers raise exceptions instead of returning garbage
Note that just fixing the negative values does not seem to patch the above issues. It does patch the following however:
./python -c "import codecs; codecs.xmlcharrefreplace_errors(UnicodeEncodeError('bad', '', 0, 1, 'reason'))"added a commit that references this issue on Dec 4, 2024gh-123378: Ensure results of `PyUnicode*Error_Get{Start,End}` are cla… picnixz commented on Dec 6, 2024 We decided NOT to backport this one even though it's a bug fix. The reason is that it could annoy users (although this would annoy them only in the case of an empty message) but to mitigate breakage, we'll just leave 3.12/3.13 broken and only fix 3.14 and later (see the PR post-merge discussion for details).
In particular, fixes for codec handlers will only be 3.14+ as well (note that we did not have an issue report for the past 20+ years that this code existed so I don't think users will really see a change).
MetadataMetadata3.14bugs and security fixesbugs and security fixesinterpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)topic-C-APItype-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or errortype-crashA hard crash of the interpreter, possibly with a core dumpA hard crash of the interpreter, possibly with a core dump StatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|