The CPython interpreter write out-of-bounds of allocated memory in certain edge cases in the utf-16 and utf-32 encoders. 

The attached script registers two error handlers that either write one ascii character, or two bytes, and tells the encoder to start again from the start of the encoding error. The script then tries to encode an invalid codepoint in either utf-16 or utf-32. Each of the calls to encode independently cause segfaults

Since the encoder starts over again and keeps trying to append the result of the error handler, the lack of proper re-allocations leads to a buffer overflow, and corrupts the stack.

Easily reproduced on master, thanks

(lldb) run encode_crash.py
Process 14743 launched: '/Users/anthonyshaw/repo/cpython/python.exe' (x86_64)
Objects/unicodeobject.c:448: _PyUnicode_CheckConsistency: Assertion "((((((PyObject*)(op))->ob_type))->tp_flags & ((1UL << 28))) != 0)" failed
Enable tracemalloc to get the memory block allocation traceback

object  : Process 14743 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
    frame #0: 0x00000001000b5c15 python.exe`PyObject_Repr(v=0x0000000101376f90) at object.c:535:11
   532 	       infinitely. */
   533 	    if (Py_EnterRecursiveCall(" while getting the repr of an object"))
   534 	        return NULL;
-> 535 	    res = (*v->ob_type->tp_repr)(v);
   536 	    Py_LeaveRecursiveCall();
   537 	    if (res == NULL)
   538 	        return NULL;

Reproduced on 3.11.

I am working on it, since it is more complex issue, and PR 13134 does not solve it.

1. This bug affects also other codecs implemented in C: ASCII, Latin1, UTF-8, etc.
2. It still crashes in UTF-16/32 encoders if the error handler returns a position less than the current position.
3. Incorrect exception can be raised if the error handler returns invalid string/bytes: a non-ASCII string or a bytes object consisting of not a whole number of units.
4. The code for standard error handlers and for decoders needs a revision too. I have some suspects.

We could just forbid error handlers returning position not in the range (start , end], but it can break some code, so it is better to do this only in a new release.

> We could just forbid error handlers returning position not in the range (start , end]

Yeah, that sounds like a reasonable solution. I don't see the point of returning a position outside this range. What would be the use case?

For me, the only corner case is the "ignore" error handler which returns an empty string, but it returns a position in this range, no?

> it can break some code, so it is better to do this only in a new release.

Implementing custom error handlers is a rare use case, so it should only affect a minority of users. Moreover, IMO returning a position outside the valid range is a bug. It's common that security fixes change the behavior, like rejecting values which were previously acceptd, to prevent a Python crash.

Looking at the specs in PEP 293 (https://www.python.org/dev/peps/pep-0293/), it is certainly possible for the error handler to return a newpos outside the range start - end, meaning in most cases: a value >= end.

There's a good reason for this: the codec may not be able to correctly determine the end of the sequence and so the end value presented by the codec is not necessarily a valid start to continue encoding/decoding. The error handler can e.g. choose to skip more input characters by trying to find the next valid sequence.

In the example script, the handler returns start, so the value is within the range. A limit would not solve the problem.

It seems that the reallocation logic of the codecs is the main problem here.

Restricting the returned position to be strictly larger than start would solve the problem with infinite loop and OOM. But this is a different issue.

On 29.09.2021 10:41, Serhiy Storchaka wrote:
> 
> Restricting the returned position to be strictly larger than start would solve the problem with infinite loop and OOM. But this is a different issue.

Yes, this would make sense, since having the codec process
the same error location over and over again will not resolve the
error, so it's clearly a bug in the error handler.

The original specification (PEP 293) required that an error handler called for encoding *must* return a replacement string (not bytes). This returned string must then be encoded again. Only if this fails an exception must be raised.

Returning bytes from the encoding error handler is an extension specified by PEP 383:

> The error handler interface is extended to allow the encode error handler to return byte strings immediately, in addition to returning Unicode strings which then get encoded again (also see the discussion below).

So for 3. in Serhiy's problem list

> 3. Incorrect exception can be raised if the error handler returns invalid string/bytes: a non-ASCII string or a bytes object consisting of not a whole number of units.

I get:

🐚 ~/ ❯ python
Python 3.9.7 (default, Sep  3 2021, 12:37:55)
[Clang 12.0.5 (clang-1205.0.22.9)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> def bad(exc):
...  return ('\udbc0', exc.start)
...
>>> import codecs
>>> codecs.register_error('bad', bad)
>>> '\udbc0'.encode('utf-16', 'bad')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeEncodeError

I would have expected an exception message that basically looks like the one I'd get, if I had used the strict error handler.

But otherwise returning a replacement that is unencodable is allowed and should raise an exception (which happens here, but with a missing exception message). (Returning something unencodable might make sense when the error handler is able to create replacement characters for some unencodable input, but not for other, but of course the error handler can always raise an exception directly).

Returning invalid bytes is not an issue, they simply get written to the output. That's exactly the use case of PEP 383: The bytes couldn't be decoded in the specified encoding, so they are "invalid", but the surrogateescape error handler encodes them back to the same "invalid" bytes. So the error handler is allowed to output bytes that can't be decoded again with the same encoding.

Returning a restart position outside the valid range of the length of the original string should raise an IndexError according to PEP 293:

> If the callback does not raise an exception (either the one passed in, or a different one), it must return a tuple: `(replacement, newpos)`
> `replacement` is a unicode object that the encoder will encode and emit instead of the unencodable `object[start:end]` part, `newpos` specifies
> a new position within object, where (after encoding the replacement) the encoder will continue encoding.
> Negative values for `newpos` are treated as being relative to end of object. If `newpos` is out of bounds the encoder will raise an `IndexError`.

Of course we could retroactively reinterpret "out of bounds" as outside of `range(exc.start + 1, len(object))`, instead of outside `range(0, len(object))`. An error handler that never advances is broken anyway. But we can't detect "never".

However it would probably be OK to reject pathological error handlers (i.e. those that don't advance (i.e. return at least `exc.start + 1` as the restart position)). But I'm not sure how that's different from an error handler that skips ahead much farther (i.e. returns something like `(exc.start+len(object))//2` or `max(exc.start+1, len(object)-10)`): The returned restart position leads to a certain expectation of how many bytes the encoder might have to output until everything is encoded and must adjust accordingly.