bpo-42663: Fix parsing TZ strings in zoneinfo module #23825

serhiy-storchaka · 2020-12-17T21:05:51Z

Support the full range of values determined by RFC 8536.
Detect all invalid formats.
Both implementations now raise exceptions of the same type on invalid data.

Also refactor parsing numbers and times (bpo-42660).

https://bugs.python.org/issue42663

#86826

Also refactor parsing of numbers and times.

vstinner

I have no opinion on the overall approach, just some remarks on the implementation.

vstinner · 2020-12-17T21:42:30Z

Modules/_zoneinfo.c

-        .hour = hour,
-        .minute = minute,
-        .second = second,
+        .month = Py_SAFE_DOWNCAST(month, int, uint8_t),


Py_SAFE_DOWNCAST() should be avoided, it is not safe in release mode: use plain cast (uint8_t)month since you just checked bounds, no?

https://bugs.python.org/issue19692

Modules/_zoneinfo.c

github-actions · 2021-01-17T00:59:32Z

This PR is stale because it has been open for 30 days with no activity.

vstinner · 2021-01-25T11:11:46Z

Modules/_zoneinfo.c

                     julian, day);
        return -1;
    }

+    if (hour < -167 || hour > 167) {
+        PyErr_Format(PyExc_ValueError, "Hour must be in [0, 167]");


is the minimum 0 or -167?

The minimum of the absolute value is 0. But there may be a minus sign before time (HHH:MM:SS).

Modules/_zoneinfo.c

vstinner · 2021-01-25T11:16:11Z

Modules/_zoneinfo.c

@@ -58,16 +58,16 @@ typedef struct {
    uint8_t month;
    uint8_t week;
    uint8_t day;
-    int8_t hour;
+    int16_t hour;


Would you mind to write a short comment explaining that hour is in range [-167; 167) (RFC ...)? Since it's non obvious why minute and second use int8_t, but hour use int16_t.

167 is written in many places, it would help to avoid inconsistencies if it's updated tomorrow.

I suggested to add a comment (ex: mentioning this constant) to explain why hour requires int16_t and not int8_t, where the structure is defined.

vstinner · 2021-01-25T11:18:26Z

Modules/_zoneinfo.c

@@ -1200,14 +1199,14 @@ calendarrule_year_to_timestamp(TransitionRuleType *base_self, int year)
    }

    int64_t ordinal = ymd_to_ord(year, self->month, month_day) - EPOCHORDINAL;
-    return ((ordinal * 86400) + (int64_t)(self->hour * 3600) +
+    return ((ordinal * 86400L) + (int64_t)(self->hour * 3600L) +
            (int64_t)(self->minute * 60) + (int64_t)(self->second));


I prefer syntax (int64_t)var * constant, rather than trying to declare the constant as an int64_t or a long.

For example, (int64_t)self->hour * 3600 rather than (int64_t)(self->hour * 3600L).

vstinner · 2021-01-25T11:19:44Z

Modules/_zoneinfo.c

        PyErr_Format(PyExc_ValueError, "Day must be in [0, 6]");
        return -1;
    }

+    if (hour < -167 || hour > 167) {


Maybe declare a constant for this limit?

I do not think it would make code better. If it is a constant declared far from that place, somebody can change its vale without changing the type of the hour field.

vstinner · 2021-01-25T11:22:19Z

Modules/_zoneinfo.c

    if (self->julian && day >= 59 && is_leap_year(year)) {
        day += 1;
    }

-    return ((days_before_year + day) * 86400) + (self->hour * 3600) +
+    return ((days_before_year + day) * 86400L) + (self->hour * 3600L) +


I suggest to declare constants to enhance the readability (60, 3600, 86400):

#define MIN_TO_SEC 60 #define HOUR_TO_SEC (60 * MIN_TO_SEC) #define DAY_TO_SEC (24 * HOUR_TO_SEC)

Example of Python/pytime.c:

/* To millisecond (10^-3) */ #define SEC_TO_MS 1000 /* To microseconds (10^-6) */ #define MS_TO_US 1000 #define SEC_TO_US (SEC_TO_MS * MS_TO_US) /* To nanoseconds (10^-9) */ #define US_TO_NS 1000 #define MS_TO_NS (MS_TO_US * US_TO_NS) #define SEC_TO_NS (SEC_TO_MS * MS_TO_NS) /* Conversion from nanoseconds */ #define NS_TO_MS (1000 * 1000) #define NS_TO_US (1000)

Constants 60 and 3600 look more readable to me than some named constant (and I have problem with distinguishing similar looking names). 86400 is enough obvious too, but if you prefer, I'll add a named constant for it.

I want to replace a trio hour/minute/second with a single integer (time offset in seconds) in future, so constants 3600 and 60 may go out here.

vstinner

LGTM.

pganssle · 2022-05-06T18:12:05Z

I haven't had a chance to look at the changes in any detail yet, but I think this needs tests, right? Or is the mobile view obscuring them or something?

serhiy-storchaka · 2022-05-07T10:38:32Z

Good point. I started this PR as pure refactoring (it reduces of the C code by 67 lines), but since it also changes the behavior, it needs tests. And it happens that the Python code did not support 24h offset, and did not detect some errors, and could raise wrong type of exception.

serhiy-storchaka · 2023-09-01T17:56:44Z

@pganssle, could you please take another look?

I added tests and found that the old implementation rejected some extreme values and accepted values outside the valid range. The Python implementation contained more bugs than the C implementation and raised incorrect types of exceptions on invalid data (e.g. IndexError).

I consider now this change as a bugfix.

miss-islington · 2023-10-14T20:24:36Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington · 2023-10-14T20:24:48Z

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker ab08ff7882b6181fb785eed7410dbf8030aded70 3.12

miss-islington · 2023-10-14T20:24:59Z

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker ab08ff7882b6181fb785eed7410dbf8030aded70 3.11

…-23825) zipinfo now supports the full range of values in the TZ string determined by RFC 8536 and detects all invalid formats. Both Python and C implementations now raise exceptions of the same type on invalid data.. (cherry picked from commit ab08ff7) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

bedevere-app · 2023-10-14T20:29:02Z

GH-110882 is a backport of this pull request to the 3.12 branch.

GH-110882) zipinfo now supports the full range of values in the TZ string determined by RFC 8536 and detects all invalid formats. Both Python and C implementations now raise exceptions of the same type on invalid data. (cherry picked from commit ab08ff7)

…ythonGH-23825) (pythonGH-110882) zipinfo now supports the full range of values in the TZ string determined by RFC 8536 and detects all invalid formats. Both Python and C implementations now raise exceptions of the same type on invalid data. (cherry picked from commit ab08ff7). (cherry picked from commit 72b0f0e) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…H-23825) (GH-110882) (GH-110889) zipinfo now supports the full range of values in the TZ string determined by RFC 8536 and detects all invalid formats. Both Python and C implementations now raise exceptions of the same type on invalid data. (cherry picked from commit ab08ff7) (cherry picked from commit 72b0f0e)

zipinfo now supports the full range of values in the TZ string determined by RFC 8536 and detects all invalid formats. Both Python and C implementations now raise exceptions of the same type on invalid data.

bpo-42663: Support full range of allowed transition hours in zipinfo.

cf67d21

Also refactor parsing of numbers and times.

serhiy-storchaka assigned pganssle Dec 17, 2020

the-knights-who-say-ni added the CLA signed label Dec 17, 2020

bedevere-bot added the awaiting core review label Dec 17, 2020

vstinner reviewed Dec 17, 2020

View reviewed changes

github-actions bot added the stale Stale PR or inactive for long period of time. label Jan 17, 2021

serhiy-storchaka added 2 commits January 24, 2021 21:03

Merge branch 'master' into zipinfo-parse

7ebd338

Get rid of Py_SAFE_DOWNCAST.

bf4f06c

vstinner reviewed Jan 25, 2021

View reviewed changes

Address review comments.

6f7fff2

vstinner approved these changes Jan 26, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jan 26, 2021

serhiy-storchaka added 3 commits May 16, 2021 17:23

Merge branch 'main' into zipinfo-parse

b15febb

Merge branch 'main' into zipinfo-parse

42647ee

Merge branch 'main' into zipinfo-parse

7de35dd

serhiy-storchaka added the type-feature A feature request or enhancement label May 6, 2022

serhiy-storchaka added 2 commits May 6, 2022 15:33

Merge branch 'main' into zipinfo-parse

e5b8c28

Add a NEWS entry.

11c7118

pganssle changed the title ~~bpo-42663: Support full range of allowed transition hours in zipinfo.~~ bpo-42663: Support full range of allowed transition hours in zoneinfo. May 6, 2022

serhiy-storchaka added 3 commits May 6, 2022 21:29

Merge branch 'main' into zipinfo-parse

1f8184d

Add tests and fix the Python implementation

2756b82

Merge branch 'main' into zipinfo-parse

c89a82a

This was referenced Apr 10, 2022

_zoneinfo.c incorrectly checks bounds of day variable in calenderrule_new #86826

Open

zoneinfo does not support full range of allowed transition hours in fallback string #86829

Closed

ezio-melotti removed the CLA signed label Jul 13, 2022

github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 2, 2022

serhiy-storchaka added 2 commits September 1, 2023 18:17

Merge branch 'main' into zipinfo-parse

6b14ca3

Merge branch 'main' into zipinfo-parse

c67fe4b

serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Sep 1, 2023

serhiy-storchaka changed the title ~~bpo-42663: Support full range of allowed transition hours in zoneinfo.~~ bpo-42663: Fix parsing TZ strings in zoneinfo module Sep 1, 2023

Update the NEWS file.

8ca08cc

serhiy-storchaka requested a review from pganssle September 1, 2023 17:57

serhiy-storchaka merged commit ab08ff7 into python:main Oct 14, 2023

bedevere-app bot removed the awaiting merge label Oct 14, 2023

serhiy-storchaka deleted the zipinfo-parse branch October 14, 2023 20:24

miss-islington assigned serhiy-storchaka and unassigned pganssle Oct 14, 2023

bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Oct 14, 2023

serhiy-storchaka removed their assignment Oct 15, 2023

serhiy-storchaka removed the needs backport to 3.11 only security fixes label Oct 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-42663: Fix parsing TZ strings in zoneinfo module #23825

bpo-42663: Fix parsing TZ strings in zoneinfo module #23825

serhiy-storchaka commented Dec 17, 2020 •

edited

Loading

vstinner left a comment

vstinner Dec 17, 2020

github-actions bot commented Jan 17, 2021

vstinner Jan 25, 2021

serhiy-storchaka Jan 25, 2021

vstinner Jan 25, 2021

vstinner Jan 26, 2021

vstinner Jan 25, 2021

vstinner Jan 25, 2021

serhiy-storchaka Jan 25, 2021

vstinner Jan 25, 2021

serhiy-storchaka Jan 25, 2021

vstinner left a comment

pganssle commented May 6, 2022

serhiy-storchaka commented May 7, 2022

serhiy-storchaka commented Sep 1, 2023

miss-islington commented Oct 14, 2023

miss-islington commented Oct 14, 2023

miss-islington commented Oct 14, 2023

bedevere-app bot commented Oct 14, 2023

bpo-42663: Fix parsing TZ strings in zoneinfo module #23825

bpo-42663: Fix parsing TZ strings in zoneinfo module #23825

Conversation

serhiy-storchaka commented Dec 17, 2020 • edited Loading

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

pganssle commented May 6, 2022

serhiy-storchaka commented May 7, 2022

serhiy-storchaka commented Sep 1, 2023

miss-islington commented Oct 14, 2023

miss-islington commented Oct 14, 2023

miss-islington commented Oct 14, 2023

bedevere-app bot commented Oct 14, 2023

serhiy-storchaka commented Dec 17, 2020 •

edited

Loading