[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#3740denis-osipov wants to merge 21 commits into
Conversation
…). Related changes in tmtotuple() and time.gmtime()
| } | ||
|
|
||
| // Function to get time zone name with Windows API | ||
| static void get_windows_zone(wchar_t *out) |
There was a problem hiding this comment.
Move "static void" on a separate line.
| TIME_ZONE_INFORMATION tzi; | ||
| DWORD tzid = GetTimeZoneInformation(&tzi); | ||
|
|
||
| if (tzid < 2) { |
There was a problem hiding this comment.
Wouldn't be better to use named constants? TIME_ZONE_ID_STANDARD, TIME_ZONE_ID_DAYLIGHT, etc.
Handle error (TIME_ZONE_ID_INVALID).
| @@ -622,27 +656,27 @@ time_strftime(PyObject *self, PyObject *args) | ||
| fmt = PyBytes_AS_STRING(format); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
What abut the case defined(MS_WINDOWS) && !defined(HAVE_WCSFTIME)?
There was a problem hiding this comment.
@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.
| ins = tmp + 2; | ||
| } | ||
|
|
||
| //Replace %Z with time zone name |
There was a problem hiding this comment.
Check for integer overflow.
if (len_zone - 2 > (Py_SSIZE_MAX/sizeof(time_char) - 1 - wcslen(fmt)) / count) {
// raise MemoryError
}
| get_windows_zone(zone); | ||
| len_zone = wcslen(zone); | ||
|
|
||
| // Count the number of %Z occurences |
There was a problem hiding this comment.
You could use the same variable for ins and tmp here.
| PyModule_AddIntConstant(m, "daylight", daylight); | ||
| /*bpo-16322, bpo-27426: For Windows use GetTimeZoneInformation() to avoid | ||
| wrong encoding of time zone names.*/ | ||
| #ifdef MS_WINDOWS |
There was a problem hiding this comment.
Handle error (TIME_ZONE_ID_INVALID).
Is DaylightName initialized properly if GetTimeZoneInformation() != TIME_ZONE_ID_DAYLIGHT?
There was a problem hiding this comment.
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.
|
|
||
| #if defined(MS_WINDOWS) && !defined(HAVE_WCSFTIME) | ||
| #if defined(MS_WINDOWS) && defined(HAVE_WCSFTIME) | ||
| /* check that the format string contains only valid directives */ |
There was a problem hiding this comment.
Loops for Windows, AIX and Solaris are too similar. Can they be merged?
| 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"); |
There was a problem hiding this comment.
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.
|
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 And if you don't make the requested changes, you will be put in the comfy chair! |
|
I didn't expect the Spanish Inquisition! |
|
Nobody expects the Spanish Inquisition! @serhiy-storchaka: please review the changes made to this pull request. |
|
Oops... Fix of memory leak needed. |
Use Windows APIs to avoid wrong encoding of time zone names: bpo-16322 and bpo-27426.
Switch back to using wcsftime because:
https://bugs.python.org/issue16322