BUG: Add extra check for failing UTF-8 conversion #32548
Conversation
roberthdevries
force-pushed
the
fix-23809-read_excel-crashes-high-surrogate
branch
from
02cd952 to
dc777e5
Compare
pandas/_libs/src/parse_helper.h
Outdated
| @@ -34,6 +34,10 @@ int floatify(PyObject *str, double *result, int *maybe_int) { | |||
| data = PyBytes_AS_STRING(str); | |||
| } else if (PyUnicode_Check(str)) { | |||
| tmp = PyUnicode_AsUTF8String(str); | |||
| if (tmp == NULL) { | |||
| PyErr_SetString(PyExc_TypeError, "UTF8 decode error"); | |||
There was a problem hiding this comment.
Do you need to set the error here? I think the previous call would already set and can just return, no?
There was a problem hiding this comment.
Apparently the 'str' object passed is a Unicode type or a subtype. Ref: https://docs.python.org/3/c-api/unicode.html
And it seems to pass that test fine. However, it fails to convert to UTF8, hence the NULL return value.
BTW, I see know that the message is wrong (decode->encode)
There was a problem hiding this comment.
Let me clarify - so I am asking if you need the PyErr_SetString at all. I think the failing PyUnicode_AsUTF8String call should have already set the global error indicator / message, so I think this is redundant
There was a problem hiding this comment.
Removed
I tried to figure out if PyUnicode_AsUTF8String sets an exception or not, but I failed to find out.
However a manual test showed an exception so I guess we are covered here.
>>> "\udc88".encode()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
UnicodeEncodeError: 'utf-8' codec can't encode character '\udc88' in position 0: surrogates not allowed
There was a problem hiding this comment.
Yea that's generally the pattern for the C API functions. Quoting the guide with respect to the global error indicator:
https://docs.python.org/3/c-api/exceptions.html#exception-handling
Most C API functions don’t clear this on success, but will set it to indicate the cause of the error on failure. Most C API functions also return an error indicator, usually NULL if they are supposed to return a pointer, or -1 if they return an integer (exception: the PyArg_*() functions return 1 for success and 0 for failure).
I'm not sure which ones don't fall into the "Most" category, but good to know this one does
| # GH 23809 | ||
|
|
||
| # should not produce a segmentation violation | ||
| pd.read_excel("high_surrogate.xlsx") |
There was a problem hiding this comment.
Can you create the expected dataframe and use tm.assert_frame_equal?
|
Can you check any impact to the CSV reader as well? Based off of where this fix is made I would imagine this impacts that as well as excel, so maybe should add a test / document fix for both |
|
lgtm. defer to @WillAyd for testing. |
datapythonista
changed the title
|
Great thanks @roberthdevries |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff