Bug: exception in noexcept what() when Python exception contains a surrogate character

[Skylion007]

Discussed in #4287

^{Originally posted by TheShiftedBit October 26, 2022}
By default, Python produces errors when converting encoding strs with utf-8 if the str contains surrogate characters. This can be disabled by passing surrogatepass as a second argument to .encode(). Pybind11 has this same behavior with its str -> std::string conversion. However, the bug is this: if an exception message contains a surrogate character, calling .what() on an error_already_set with such an exception causes another exception to be thrown, but since .what() is noexcept, that exception cannot be caught and the program std::terminates.

I'm not sure what the correct behavior regarding surrogate characters is. Perhaps pybind11 should always use surrogatepass, perhaps not. However, even if that's not the right choice, it should probably use it during exception handling, or Python exceptions like this are extremely difficult to diagnose.

[rwgk]

Without having looked very closely here, it seems similar to what we were running into with the absl::Status type, which also reports error messages. We settled on this:

https://github.com/pybind/pybind11_abseil/blob/ef7a738c6472f661aa3a77c228ff0b456fee3f2f/pybind11_abseil/utils_pybind11_absl.cc

That approach could be adopted in error_already_set.

Although that throws error_already_set, which we don't want to do when we're already inside error_already_set I think. I'm not sure how to best error out if there is really no way to recover the error message (some kind of system error or undefined behavior, presumably).

[Skylion007]

@TheShiftedBit Could be a bit more specific on what type of exception is being thrown here and possible provide an minimal compilable example? Better yet, open a PR that adds a unit test which fails under the current framework.

There are a couple of potential solutions. Apparently, one is just returning unicode characters directly in what() per this StackOverflow exchange: https://stackoverflow.com/q/3760731/2444240 . Another option is escaping / replacing the error. I am trying to figure if the exception is coming. I think I narrowed it down to this line:

pybind11/include/pybind11/pytypes.h

Line 1481 in fcb5554

temp = reinterpret_steal<object>(PyUnicode_AsUTF8String(m_ptr));

Now, it's a bit odd we have explicit static conversion to std::string here. I guess because it is faster / simpler. We do have support for casting to the relevant std::string. Wouldn't just changing the exception type string to that type work well? Now, not sure exactly where it is triggered in our stack, but I think it might be at this conversion here:

pybind11/include/pybind11/pytypes.h

Line 508 in fcb5554

result = value_str.cast<std::string>();

. The static_cast fails and throws an exception because of the surrogate char though because of behavior introduced in Python 3.4: https://python.readthedocs.io/en/stable/whatsnew/3.4.html .

Okay, there is a big perf issue here at least. We reinterpret_cast<object> the value object to an a py::object which means when we call caststd::string() it calls the str_caster in cast.h instead of our static_cast converter pytypes.h. (I see why you did it so early, because we need to turn the function into a forward decl otherwise, still might be useful though as that caster is a lot faster since we don't have to do any lookups onto the heap / check overloads etc...

[Skylion007]

Something like this could work as a hotfix, but is still inefficient.

--- a/include/pybind11/pytypes.h
+++ b/include/pybind11/pytypes.h
@@ -505,6 +505,8 @@ struct error_fetch_and_normalize {
                 message_error_string = detail::error_string();
                 result = "<MESSAGE UNAVAILABLE DUE TO ANOTHER EXCEPTION>";
             } else {
+                value_str = reinterpret_steal<object>(
+                    PyUnicode_AsEncodedString(value_str.ptr(), "utf-8", "replace")); // or "surrogatepass"
                 result = value_str.cast<std::string>();
             }
         } else {

[Skylion007]

On MacOS at least: I'm not getting an exception, I am just junk characters as the output. I suspect this may be an issue on Windows though.

    def test_surrogate_exception():
        ucs_surrogates_str = "\udcc3"
>       m.test_surrogate_str(ucs_surrogates_str)
\x1b[1m\x1b[31mE       AttributeError: 'str' object has no attribute '\udcc3'\x1b[0m

ucs_surrogates_str = '\udcc3'

[TheShiftedBit]

You asked for a reproducer, I posted it on the other issue: #4287 (comment)

I temporarily fixed it by adapting your what() implementation. I roughly do this:

auto value_str = py::reinterpret_steal<py::object>(PyObject_Str(ex.value().ptr()));
py::bytes bytes = value_str.attr("encode").call("utf-8", "backslashreplace");
std::string result = bytes;

(This one doesn't have the backtrace, just the message). In my case, I don't care about recovering from an exception, I just want to end the program with as much detail as possible; so using backslashreplace is a good choice for me. For a more general implementation, you probably want surrogatepass or something.

[Skylion007]

@TheShiftedBit Your repro doesn't seem to actually reproduce the issue, :( #4295

[TheShiftedBit]

I've attached a zip containing a Dockerfile with the full reproducer. I pinned every relevant version number I could find, including pinning pybind11 to the latest release.

Run with:

cd <place_with_Dockerfile>
sudo docker build -t repro .
sudo docker run repro

pybind11_repro.zip

[rwgk]

This reproduces the error:

index 70b6ffea..0114bc31 100644
--- a/tests/test_exceptions.py
+++ b/tests/test_exceptions.py
@@ -275,6 +275,11 @@ def test_local_translator(msg):
     assert msg(excinfo.value) == "this mod"


+def test_error_already_set_message_with_unicode_surrogate():
+    # Issue #4288
+    m.error_already_set_what(RuntimeError, "\ud927")
+
+
 class FlakyException(Exception):
     def __init__(self, failure_point):
         if failure_point == "failure_point_init":

test_exceptions.py::test_error_already_set_message_with_unicode_surrogate terminate called after throwing an instance of 'pybind11::cast_error'
  what():  Unable to cast Python instance of type <class 'str'> to C++ type 'std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >'
Fatal Python error: Aborted

Current thread 0x00007f15b3d4a740 (most recent call first):
  File "/usr/local/google/home/rwgk/forked/pybind11/tests/test_exceptions.py", line 280 in test_error_already_set_message_with_unicode_surrogate
  File "/usr/local/lib/python3.10/dist-packages/_pytest/python.py", line 192 in pytest_pyfunc_call

I think it's an easy fix, but I need to play a bit.

[rwgk]

@henryiii This would be a good one to get out ASAP. It's a bit nasty that it dies with terminate.