[memory leak]bpo-16322, bpo-27426: Fix time zone names encoding issues in Windows by denis-osipov · Pull Request #3740 · python/cpython

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

 }
+
+// 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

+    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

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



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

+        ins = tmp + 2;
+    }
+
+    //Replace %Z with time zone name


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

+    get_windows_zone(zone);
+    len_zone = wcslen(zone);
+
+    // Count the number of %Z occurences


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

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

    PyModule_AddIntConstant(m, "daylight", daylight);
+    /*bpo-16322, bpo-27426: For Windows use GetTimeZoneInformation() to avoid
+    wrong encoding of time zone names.*/
+#ifdef MS_WINDOWS


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


-#if defined(MS_WINDOWS) && !defined(HAVE_WCSFTIME)
+#if defined(MS_WINDOWS) && defined(HAVE_WCSFTIME)
    /* check that the format string contains only valid directives */


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

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

+        size_t l = wcslen(fmt) + (len_zone - 2) * count + 1;
+        tmp = result = (time_char *)PyMem_Malloc(l * sizeof(time_char));
+        while (count--) {
+            ins = wcsstr(fmt, L"%Z");


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

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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