Skip to content

bpo-34482: Add tests for proper handling of non-UTF-8-encodable strin… #8878

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
taleinat merged 7 commits into from
Oct 23, 2018
Merged

bpo-34482: Add tests for proper handling of non-UTF-8-encodable strin… #8878

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
97037bd
bpo-34482: Add tests for proper handling of non-UTF-8-encodable strin…
Aug 21, 2018
a7e435d
Remove FIXMEs and use assertEqual in strftime tests
Aug 23, 2018
4603f7e
Remove the NEWS entry
Aug 23, 2018
beca2bd
Add a separate test for datetime.strftime()
Aug 23, 2018
51bd826
Remove check for strftime() return value
Aug 24, 2018
f24c13c
Don't mix regular and surrogate-related tests in test_strptime()
Aug 30, 2018
faadbb9
Merge branch 'master' into bpo-34482
taleinat Oct 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 40 additions & 7 deletions Lib/test/datetimetester.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,8 @@ def test_tzname(self):
self.assertEqual('UTC+09:30', timezone(9.5 * HOUR).tzname(None))
self.assertEqual('UTC-00:01', timezone(timedelta(minutes=-1)).tzname(None))
self.assertEqual('XYZ', timezone(-5 * HOUR, 'XYZ').tzname(None))
# bpo-34482: Check that surrogates are handled properly.
self.assertEqual('\ud800', timezone(ZERO, '\ud800').tzname(None))

# Sub-minute offsets:
self.assertEqual('UTC+01:06:40', timezone(timedelta(0, 4000)).tzname(None))
Expand Down Expand Up @@ -1307,6 +1309,14 @@ def test_strftime(self):
except ValueError:
pass

# bpo-34482: Check that surrogates don't cause a crash.
# FIXME: The C datetime implementation raises an exception
# while the pure-Python one succeeds.
try:
t.strftime('\ud800')
except UnicodeEncodeError:
pass

#check that this standard extension works
t.strftime("%f")

Expand Down Expand Up @@ -1746,6 +1756,9 @@ def test_isoformat(self):
self.assertEqual(t.isoformat('T'), "0001-02-03T04:05:01.000123")
self.assertEqual(t.isoformat(' '), "0001-02-03 04:05:01.000123")
self.assertEqual(t.isoformat('\x00'), "0001-02-03\x0004:05:01.000123")
# bpo-34482: Check that surrogates are handled properly.
self.assertEqual(t.isoformat('\ud800'),
"0001-02-03\ud80004:05:01.000123")
self.assertEqual(t.isoformat(timespec='hours'), "0001-02-03T04")
self.assertEqual(t.isoformat(timespec='minutes'), "0001-02-03T04:05")
self.assertEqual(t.isoformat(timespec='seconds'), "0001-02-03T04:05:01")
Expand All @@ -1754,6 +1767,8 @@ def test_isoformat(self):
self.assertEqual(t.isoformat(timespec='auto'), "0001-02-03T04:05:01.000123")
self.assertEqual(t.isoformat(sep=' ', timespec='minutes'), "0001-02-03 04:05")
self.assertRaises(ValueError, t.isoformat, timespec='foo')
# bpo-34482: Check that surrogates are handled properly.
self.assertRaises(ValueError, t.isoformat, timespec='\ud800')
# str is ISO format with the separator forced to a blank.
self.assertEqual(str(t), "0001-02-03 04:05:01.000123")

Expand Down Expand Up @@ -2277,13 +2292,21 @@ def test_utcnow(self):
self.assertLessEqual(abs(from_timestamp - from_now), tolerance)

def test_strptime(self):
string = '2004-12-01 13:02:47.197'
format = '%Y-%m-%d %H:%M:%S.%f'
expected = _strptime._strptime_datetime(self.theclass, string, format)
got = self.theclass.strptime(string, format)
self.assertEqual(expected, got)
self.assertIs(type(expected), self.theclass)
self.assertIs(type(got), self.theclass)
inputs = [

@taleinat taleinat Aug 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave the first "happy path" test as it is.

I suggest separating the surrogate examples, keeping the loop just for them, and removing the type assertions for them, i.e. just checking that the result equals what is expected.

@izbyshev izbyshev Aug 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable, fixed.

('2004-12-01 13:02:47.197', '%Y-%m-%d %H:%M:%S.%f'),
# bpo-34482: Check that surrogates are handled properly.
('2004-12-01\ud80013:02:47.197', '%Y-%m-%d\ud800%H:%M:%S.%f'),
('2004\ud80012-01 13:02:47.197', '%Y\ud800%m-%d %H:%M:%S.%f'),
('2004-12-01 13:02\ud80047.197', '%Y-%m-%d %H:%M\ud800%S.%f'),
]
for string, format in inputs:
with self.subTest(string=string, format=format):
expected = _strptime._strptime_datetime(self.theclass, string,
format)
got = self.theclass.strptime(string, format)
self.assertEqual(expected, got)
self.assertIs(type(expected), self.theclass)
self.assertIs(type(got), self.theclass)

strptime = self.theclass.strptime
self.assertEqual(strptime("+0002", "%z").utcoffset(), 2 * MINUTE)
Expand Down Expand Up @@ -2869,6 +2892,8 @@ def test_isoformat(self):
self.assertEqual(t.isoformat(timespec='microseconds'), "12:34:56.123456")
self.assertEqual(t.isoformat(timespec='auto'), "12:34:56.123456")
self.assertRaises(ValueError, t.isoformat, timespec='monkey')
# bpo-34482: Check that surrogates are handled properly.
self.assertRaises(ValueError, t.isoformat, timespec='\ud800')

t = self.theclass(hour=12, minute=34, second=56, microsecond=999500)
self.assertEqual(t.isoformat(timespec='milliseconds'), "12:34:56.999")
Expand Down Expand Up @@ -2919,6 +2944,14 @@ def test_strftime(self):
# A naive object replaces %z and %Z with empty strings.
self.assertEqual(t.strftime("'%z' '%Z'"), "'' ''")

# bpo-34482: Check that surrogates don't cause a crash.
# FIXME: The C datetime implementation raises an exception
# while the pure-Python one succeeds.
try:
t.strftime('\ud800')

@pganssle pganssle Aug 23, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems worth of a separate test, or at least a subtest.

Also, you may as well assert something about the result of this, self.assertEqual(t.strftime('\ud800'), '\ud800')

@taleinat taleinat Aug 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pganssle, why assert that? It's not clear to me that this should be something we ensure.

IMO the test is fine. I'd just suggest removing the "FIXME" paragraph entirely (here and above).

@pganssle pganssle Aug 23, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we saying that .strftime(unicode_with_surrogates) is undefined behavior? Implementation dependent?

I think it's reasonable to either pick something and stick with it or formally make this an error on the pure-python side as well.

@taleinat taleinat Aug 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. Let's go with your fix (in the PR to this PR's branch) and the test you suggest above. @izbyshev, could you update accordingly?

@izbyshev izbyshev Aug 23, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taleinat OK, but I think it'd cleaner to go with two separate PRs for bpo-34481 and bpo-34482. @pganssle suggests the same.

@pganssle pganssle Aug 23, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. This seems like a separate bug on Mac, honestly, rather than implementation-dependent behavior. Usually the "implementation-dependent" stuff in strftime is in how different platforms interpret the different formatting directives, not stuff like this.

I'd be tempted to move the surrogate assertion portion added into a separate test that you can wrap in @unittest.skipIf or the like.

As for what to do about it, technically it's probably a bug in the platform's strftime implementation, of course, but it might be worth adding a workaround like I did for the C implementation into the Python implementation. It definitely seems wrong that dt.strftime('%Y-%m-%dT%H:%M:%S\ud800') would return ''. It should either raise an error or return the formatted date with the surrogate character in place.

@izbyshev izbyshev Aug 23, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turned out that behavior of strftime with surrogates is even worse. If there is no wcsftime, the string is encoded with surrogateescape error handler, which means that my tests should fail with UnicodeEncodeError even on pure-Python datetime because the surrogate I use (\uD800) is not in the range of surrogates used for escaping (\uDC80-\uDCFF).

This is indeed what happens on my Windows 8.1 with Python 3.7 in a manual test:

Python 3.7.0 (v3.7.0:1bf9cc5093, Jun 27 2018, 04:59:51) [MSC v.1914 64 bit (AMD64)] on win32
>>> import time
>>> time.strftime('\ud800')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeEncodeError: 'locale' codec can't encode character '\ud800' in position 0: encoding error

Moreover:

>>> time.strftime('\udc80')
'\x80'

This I can't explain currently. Decoding in timemodule.c uses the locale encoding (with surrogateescape), and my locale's encoding is not Latin-1. If I do what I think should be the same, I get a different result:

>>> ord('\udc80'.encode('mbcs', 'surrogateescape').decode('mbcs', 'surrogateescape'))
1026 # This is the unicode code point corresponding to b'0x80' in cp1251 encoding

I'll try to understand this tomorrow.

@izbyshev izbyshev Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the summary of what I've learned.

  • strftime implementation uses either strftime or wcsftime from the C standard library, depending on the platform (usually wcsftime is available, e.g. on Linux and macOS, but strftime is used on Windows because wcsftime is broken).
  • Those functions are locale-dependent (in the sense of setlocale()).
  • Those functions don't report errors via errno according to POSIX and man pages. All they can do is return 0, except that on Windows errno is set to EINVAL if buffer is too short.
  • If wcsftime is used, Python converts the format string into wchar_t string, and there are no issues with surrogates at this point. However, wcsftime may perform additional validation of wchars for the current locale. glibc apparently doesn't do that, since I could roundtrip any code point through wcsftime, including surrogates. But wcsftime on macOS does perform the validation: in C locale, it rejects code points > 255 (by returning zero; errno was also set to EILSEQ in my tests, but it's not documented). Even in a locale with UTF-8 charset, it rejects surrogates. This why tests on macOS failed.
  • If strftime is used, Python encodes the input into a locale-dependent charset with surrogateescape error handler, and decodes the result back in the same way. The implications:
    • Surrogates outside of \uDC80-\uDCFF range cause UnicodeEncodeError.
    • Other surrogates can't be reliably round-tripped because they turn into lone bytes on encoding which are then reinterpreted with the locale encoding on the way back. This explains why I got 0x80 from time.strftime('\udc80') on Windows: on encoding, the surrogate was unwrapped into b'0x80', on decoding, since there was no setlocale() calls and the C locale was active, each byte was simply converted to the code point with the same numerical value. I was initially confused because I thought that "locale" means the user-default code page (which would give a different interpretation for b'0x80').

Given the above, I don't think that we should check the return value of strftime in surrogate-related tests, at least in this PR. If such tests are desired, I think that they belong to the time module and require some experiments/doc research on platforms not covered by build bots.

@pganssle pganssle Aug 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@izbyshev Awesome work. I'm thinking the best way forward at this point is to drop the assertion portion and to copy all this information into a separate bug report about the inconsistency in strftime. It might be a wontfix or back-burnered, but I think we can probably get a lot more consistency than we already have.

@izbyshev izbyshev Aug 26, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pganssle After thinking about a separate bug report, I realized that the whole situation is not so different from the usual stuff with file-system-related functions. Some platforms have wide-character APIs, others have only byte-oriented APIs, and Python tries to appease both. open('\ud800') fails on Linux with UnicodeEncodeError, but is OK on Windows. And the twist with wcsftime on macOS rejecting surrogates is somewhat similar Windows refusing to open('*'). Seems unlikely that we can reach consistency here, given that existing users may rely on specific behavior. So I've submitted bpo-34512 to at least try to update the docs.

except UnicodeEncodeError:
pass

def test_format(self):
t = self.theclass(1, 2, 3, 4)
self.assertEqual(t.__format__(''), str(t))
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Tests/2018-08-23-20-42-14.bpo-34482.BzQYUs.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Add tests for proper handling of non-UTF-8-encodable strings in
:mod:`datetime` classes. Patch by Alexey Izbyshev.