[memory leak]bpo-16322, bpo-27426: Fix time zone names encoding issues in Windows #3740

denis-osipov · 2017-09-25T07:07:32Z

Use Windows APIs to avoid wrong encoding of time zone names: bpo-16322 and bpo-27426.
Switch back to using wcsftime because:

replace wcsftime() with strftime() to fix encoding issues doesn't work anymore ( http://bugs.python.org/issue8304#msg255133 );
not to convert new time zone names from Windows API to char*.

https://bugs.python.org/issue16322

…). Related changes in tmtotuple() and time.gmtime()

serhiy-storchaka · 2017-09-26T09:10:36Z

Modules/timemodule.c

@@ -109,6 +109,20 @@ win_perf_counter(_Py_clock_info_t *info)
    }
    return PyFloat_FromDouble(diff / (double)cpu_frequency);
 }
+
+// Function to get time zone name with Windows API
+static void get_windows_zone(wchar_t *out)


Move "static void" on a separate line.

serhiy-storchaka · 2017-09-26T09:13:47Z

Modules/timemodule.c

+    TIME_ZONE_INFORMATION tzi;
+    DWORD tzid = GetTimeZoneInformation(&tzi);
+
+    if (tzid < 2) {


Wouldn't be better to use named constants? TIME_ZONE_ID_STANDARD, TIME_ZONE_ID_DAYLIGHT, etc.

Handle error (TIME_ZONE_ID_INVALID).

serhiy-storchaka · 2017-09-26T09:22:05Z

Modules/timemodule.c

@@ -622,27 +656,27 @@ time_strftime(PyObject *self, PyObject *args)
    fmt = PyBytes_AS_STRING(format);
 #endif

-#if defined(MS_WINDOWS) && !defined(HAVE_WCSFTIME)
+#if defined(MS_WINDOWS) && defined(HAVE_WCSFTIME)


What abut the case defined(MS_WINDOWS) && !defined(HAVE_WCSFTIME)?

@serhiy-storchaka If I'm not mistaken, this case is possible only when we undefine HAVE_WCSFTIME somewhere. Do you mean we should have special code for this case?
Now I just returned this block to state before switching from wcsftime to strftime.

serhiy-storchaka · 2017-09-26T09:26:36Z

Modules/timemodule.c

+
+    //Replace %Z with time zone name
+    if (count) {
+        size_t l = wcslen(fmt) + (len_zone - 2) * count + 1;


Check for integer overflow.

if (len_zone - 2 > (Py_SSIZE_MAX/sizeof(time_char) - 1 - wcslen(fmt)) / count) { // raise MemoryError }

serhiy-storchaka · 2017-09-26T09:32:00Z

Modules/timemodule.c

+
+    // Count the number of %Z occurences
+    ins = fmt;
+    for (count = 0; tmp = wcsstr(ins, L"%Z"); ++count) {


You could use the same variable for ins and tmp here.

serhiy-storchaka · 2017-09-26T09:35:43Z

Modules/timemodule.c

+    wrong encoding of time zone names.*/
+#ifdef MS_WINDOWS
+    TIME_ZONE_INFORMATION tzi;
+    GetTimeZoneInformation(&tzi);


Handle error (TIME_ZONE_ID_INVALID).

Is DaylightName initialized properly if GetTimeZoneInformation() != TIME_ZONE_ID_DAYLIGHT?

Yes, I think so. TIME_ZONE_INFORMATION structure get all members if GetTimeZoneInformation() succeeds. On my system GetTimeZoneInformation() == TIME_ZONE_ID_UNKNOWN, i.e. daylight saving time is not used, but both names initialized properly. However, DaylightName and StandardName can be empty.

serhiy-storchaka · 2017-09-26T09:41:26Z

Modules/timemodule.c

    /* check that the format string contains only valid directives */
-    for (outbuf = strchr(fmt, '%');
+    for (outbuf = wcschr(fmt, L'%');


Loops for Windows, AIX and Solaris are too similar. Can they be merged?

serhiy-storchaka · 2017-09-26T09:47:09Z

Modules/timemodule.c

+        while (count--) {
+            ins = wcsstr(fmt, L"%Z");
+            len_copy = ins - fmt;
+            if (wcsncmp(ins - 1, L"%", 1) == 0) {


If the format starts with a "%Z", ins == fmt and ins - 1 is outside of a buffer.

If "%Z" follows a double "%" ("%%%Z"), it is copied as is.

bedevere-bot · 2017-09-26T09:48:31Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

denis-osipov · 2017-09-30T12:41:53Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-09-30T12:41:55Z

Nobody expects the Spanish Inquisition!

@serhiy-storchaka: please review the changes made to this pull request.

denis-osipov · 2017-10-19T15:26:36Z

Oops... Fix of memory leak needed.

./python.bat -bb -E -Wd -m test test_strftime -r -uall -j0 -R :
...
test_strftime leaked [1, 2, 1, 1] memory blocks, sum=5
...

denis-osipov added 9 commits September 22, 2017 09:37

Call native Windows APIs to get the timezone name.

1badc2c

Call native Windows APIs to get the timezone name for time.localtime(…

ed05218

…). Related changes in tmtotuple() and time.gmtime()

Add comment.

f1a7b92

Back to wcsftime. Convert tabs to space.

1639cad

Add time zone directive's replacement

fb417ff

Add function to get time zone name.

90b6cdb

Add handling with '%%Z'

4bacd1e

Try to follow PEP 7

92b2f45

Minor changes (one row less).

8ecb774

the-knights-who-say-ni added the CLA signed label Sep 25, 2017

bedevere-bot added the awaiting review label Sep 25, 2017

serhiy-storchaka self-requested a review September 25, 2017 08:03

Add news entry in Misc/NEWS.d

52dc479

serhiy-storchaka requested changes Sep 26, 2017

View reviewed changes

bedevere-bot removed the awaiting review label Sep 26, 2017

bedevere-bot added the awaiting changes label Sep 26, 2017

denis-osipov added 8 commits September 26, 2017 18:12

Move 'static void' on a separate line.

1492776

Use named constants. Add error handling for TIME_ZONE_ID_INVALID.

b515a62

Add PyMem_Free call if error occured.

a898b3f

Merge format checking for Windows, AIX and Solaris

b129f62

Move %Z count to format checking block. Check for integer overflow.

fb68cbf

PyMem_Free(format) added. Rewrite part replacing %Z.

2231d72

Changed code for %Z replacement.

2c1e468

Handle error in PyInit_timezone().

3d64b76

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 30, 2017

Remove one empty line.

4ae8592

denis-osipov added 2 commits October 10, 2017 20:38

Merge branch 'master' into fix-time-3

8dcebff

Merge branch 'master' into fix-time-3

e393839

serhiy-storchaka self-assigned this Oct 13, 2017

denis-osipov changed the title ~~bpo-16322, bpo-27426: Fix time zone names encoding issues in Windows~~ [memory leak]bpo-16322, bpo-27426: Fix time zone names encoding issues in Windows Oct 19, 2017

denis-osipov closed this Oct 19, 2017

serhiy-storchaka removed their assignment Dec 6, 2018

Uh oh!

[memory leak]bpo-16322, bpo-27426: Fix time zone names encoding issues in Windows #3740

[memory leak]bpo-16322, bpo-27426: Fix time zone names encoding issues in Windows #3740

Uh oh!

Conversation

denis-osipov commented Sep 25, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denis-osipov Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denis-osipov Sep 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 26, 2017

Uh oh!

denis-osipov commented Sep 30, 2017

Uh oh!

bedevere-bot commented Sep 30, 2017

Uh oh!

denis-osipov commented Oct 19, 2017

Uh oh!

Uh oh!

denis-osipov commented Sep 25, 2017 •

edited by bedevere-bot

Loading

denis-osipov Sep 26, 2017 •

edited

Loading

denis-osipov Sep 30, 2017 •

edited

Loading