homepage

A failure of PyUnicode_AsUTF8AndSize() in various fromisoformat() functions in Modules/_datetimemodule.c leads to NULL dereference due to the missing check, e.g.:
>>> from datetime import date
>>> date.fromisoformat('\ud800')
Segmentation fault (core dumped)
This is similar to msg123474. The missing NULL check was reported by Svace static analyzer.
While preparing tests for this issue, I've discovered a deeper problem. The C datetime implementation uses PyUnicode_AsUTF8AndSize() in several places, making some functions reject strings containing surrogate code points (0xD800 - 0xDFFF) since they can't be encoded in UTF-8. On the other hand, the pure-Python datetime implementation doesn't have this restriction. For example:
>>> import sys
>>> sys.modules['_datetime'] = None # block C implementation
>>> from datetime import time
>>> time().strftime('\ud800')
'\ud800'
>>> del sys.modules['datetime']
>>> del sys.modules['_datetime']
>>> from datetime import time
>>> time().strftime('\ud800')
Traceback (most recent call last):
 File "<stdin>", line 1, in <module>
UnicodeEncodeError: 'utf-8' codec can't encode character '\ud800' in position 0: surrogates not allowed
My PR (coming soon) doesn't address this difference but focuses on fixing the immediate problem instead. Suggestions are appreciated.

So this is related to something I was actually meaning to fix. When I wrote this code I didn't understand the way PyUnicode works, there's actually no need to call `PyUnicode_AsUTF8AndSize()` on the entire unicode string.
My understanding is that each glyph in a given PyUnicode object is the same size, which means that this section of the code can go: https://github.com/python/cpython/blob/master/Modules/_datetimemodule.c#L4862
Instead we can just break the string up as glyphs 0-10 and 11+ and pass them on. Since by the contract of the function glyphs 0-10 and 11+ *must* be ASCII, we no longer need to worry about *valid* use cases where a character un-representable by UTF-8 will lead to anything except an error.
Obviously the null pointer error needs to be fixed since it should raise an error and not segfault.
I'd be happy to do the part where the string is broken up *before* being passed to PyUnicode_AsUTF8AndSize() if it would make it easier to implement your PR (which seems to touch a lot of other parts of the code as well).

I will be glad to rebase my PR and remove the try/except from the test if you remove the dependency of separator searching code on PyUnicode_AsUTF8AndSize() as you suggest. Or we can go the other way and merge mine first -- whatever you prefer.
Note that technically a difference between C and Python implementation of fromisoformat() will still remain: if a part of the input string before or after the separator contains surrogates, the C code will throw a UnicodeEncodeError while the Python code -- ValueError. But since the former error is a subclass of the latter, I guess it's OK, what do you think?
Also, note that the other discovered C/Python impl difference (for strftime, handled by another try/catch in tests) won't go away, of course, unless someone is ready to fix that as well.

> Note that technically a difference between C and Python implementation of fromisoformat() will still remain: if a part of the input string before or after the separator contains surrogates, the C code will throw a UnicodeEncodeError while the Python code -- ValueError. But since the former error is a subclass of the latter, I guess it's OK, what do you think?
I think the fact that the unicode string is decoded is an implementation detail. I would suggest swallowing the decode error and raising a standard ValueError.

I agree, that raising UnicodeEncodeError instead of ValueError is acceptable, since the former is a subclass of the latter. But since the default implementation raises more specialized subclass, it is probably that the user code will catch more narrow exception type. Seems, it is not hard to make the same exception be raised in all cases. For example, in date_fromisoformat():
 if (!dt_ptr || len != 10
 || parse_isoformat_date(dt_ptr, &year, &month, &day) < 0)
 {
 PyErr_Format(PyExc_ValueError, "Invalid isoformat string: %R", dstr);
 return NULL;
 }

New changeset 096329f0b2bf5e3f0a16363aa631d993ce078737 by Tal Einat (Paul Ganssle) in branch 'master':
bpo-34454: fix .fromisoformat() methods crashing on inputs with surrogate code points (GH-8862)
https://github.com/python/cpython/commit/096329f0b2bf5e3f0a16363aa631d993ce078737

New changeset 89b1654e0bc7bc69709dca86dd4c92eb7122ac7e by Miss Islington (bot) in branch '3.7':
bpo-34454: fix .fromisoformat() methods crashing on inputs with surrogate code points (GH-8862)
https://github.com/python/cpython/commit/89b1654e0bc7bc69709dca86dd4c92eb7122ac7e

Thanks for the report and initial patch, Alexey!
Thanks for the followup and final PR, Paul!

I reopen the issue since there are still 2 open PRs: PR 8878 and PR 8959.

New changeset 3df85404d4bf420db3362eeae1345f2cad948a71 by Victor Stinner (Paul Ganssle) in branch 'master':
bpo-34454: Clean up datetime.fromisoformat surrogate handling (GH-8959)
https://github.com/python/cpython/commit/3df85404d4bf420db3362eeae1345f2cad948a71

New changeset 18450be94d74b5c7e05a08f4aaa4792749ecda18 by Miss Islington (bot) in branch '3.7':
bpo-34454: Clean up datetime.fromisoformat surrogate handling (GH-8959)
https://github.com/python/cpython/commit/18450be94d74b5c7e05a08f4aaa4792749ecda18

Thanks Paul Ganssle for your fixes!