gh-117534: Add checking for input parameter in iso_to_ymd #117543

Vlad4896 · 2024-04-04T13:27:19Z

Python terminates with core dumped on the following test:

import datetime

y = datetime.datetime.fromisoformat('0000W25')
print(y)

The test result:

python: ./Modules/_datetimemodule.c:290: days_before_year: Assertion `year >= 1' failed.
Aborted (core dumped)

The issue happened due to lack of checking for input parameters in iso_to_ymd().

Issue: ./Modules/_datetimemodule.c:290: days_before_year: Assertion `year >= 1' failed. #117534

ghost · 2024-04-04T13:27:21Z

All commit authors signed the Contributor License Agreement.

bedevere-app · 2024-04-04T13:27:25Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

serhiy-storchaka · 2024-04-04T15:18:42Z

Modules/_datetimemodule.c

@@ -416,6 +416,9 @@ iso_week1_monday(int year)
 static int
 iso_to_ymd(const int iso_year, const int iso_week, const int iso_day,
           int *year, int *month, int *day) {
+    if (iso_year <= 0) {
+        return -2;


-2 means a specific kind of error (wrong week).

I think that the year value should be checked before calling iso_to_ymd(). It is already checked in one case, but not in other.

Please add tests.

Based on the back trace, the year value is taken from the string at parse_iso format_date() (the caller of the iso_to_umd()). Do you recommend check it in parse_iso format_date() and return -1 (-1: Failed to parse date component)?

#10 0x00007ffff7e3d7d6 in iso_to_ymd (iso_year=0, iso_week=25, iso_day=1, year=year@entry=0x7fffffffd71c, month=month@entry=0x7fffffffd720, day=day@entry=0x7fffffffd724) at ./Modules/_datetimemodule.c:440 #11 0x00007ffff7e3d998 in parse_isoformat_date (dtstr=dtstr@entry=0x7ffff74dd328 "0000W25", len=len@entry=7, year=year@entry=0x7fffffffd71c, month=month@entry=0x7fffffffd720, day=day@entry=0x7fffffffd724) at ./Modules/_datetimemodule.c:803 #12 0x00007ffff7e479c0 in datetime_fromisoformat (cls=0x7ffff7e58000 <PyDateTime_DateTimeType>, dtstr=0x7ffff74dd300) at ./Modules/_datetimemodule.c:5506

Do you mean add testcase to 'def test_fromisoformat_date_examples(self)' ?

Currently, there are two calls of iso_to_umd(). In one case the year value is validated before calling iso_to_umd(), in other case it is not validated. We should either move the validation into iso_to_umd() (and introduce a special error code for this), or always validate the year value before calling iso_to_umd().

Thank you for comments. The fix is updated accordingly.
Also I added the test case to Lib/test/datetimetester.py.

…ted)

bedevere-app · 2024-04-05T12:10:38Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

pganssle · 2024-04-05T17:57:26Z

Thanks for this, and good noticing this edge case!

What you have so far looks good to me, other than the NEWS entry.

I haven't been able to test it out, but does this also work in the pure Python version? Looks like that version may already have logic to deal with this, whereas the equivalent logic in _datetimemodule.c is in date_fromisocalendar.

Lib/test/datetimetester.py

bedevere-app · 2024-04-08T08:43:45Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app · 2024-04-08T09:12:30Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Vlad4896 · 2024-04-08T10:01:14Z

Thanks for this, and good noticing this edge case!

What you have so far looks good to me, other than the NEWS entry.

I haven't been able to test it out, but does this also work in the pure Python version? Looks like that version may already have logic to deal with this, whereas the equivalent logic in _datetimemodule.c is in date_fromisocalendar.

I've added NEWS entry.
Could you please clarify, what do you mean "work in the pure Python version"?

Correct syntax.

Lib/test/datetimetester.py

Misc/NEWS.d/next/C API/2024-04-08-09-44-29.gh-issue-117534.54ZE_n.rst

pganssle

Could you please clarify, what do you mean "work in the pure Python version"?

datetime is a PEP 399 module, meaning there are two implementations, _pydatetime (which is written entirely in Python, and _datetimemodule.c, which is written in C. Most changes need to update both versions, because the logic is supposed to be identical.

In this case, I think it's OK, because the Python version already does the right thing because of the way the code is organized. That said, I tried to run this today and it looks like the core dump only happens if the code is compiled with assertions on — which by default it is not — so I think the severity of this bug is not as bad as I had feared.

Overall, I think this is a good patch. It brings the C code more in line with the Python code by unifying the error handling for invalid years, and prevents us from relying on an accidental feature of the way this was being parsed. I made two suggestions in the comments in-line, and if you want you can add yourself to the ACKS file, but other than that, if the CI passes after the changes, this is looking good to merge, assuming @serhiy-storchaka doesn't have any objections.

bedevere-app · 2024-04-08T17:10:00Z

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 have made the requested changes; please review again. 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.

Update wordings.

pganssle

LGTM, other than moving your name around in the ACKS list.

@serhiy-storchaka Any other comments, or should we merge?

Misc/ACKS

serhiy-storchaka

LGTM.

miss-islington-app · 2024-04-09T17:53:40Z

Thanks @Vlad4896 for the PR, and @pganssle for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

…onGH-117543) Moves the validation for invalid years in the C implementation of the `datetime` module into a common location between `fromisoformat` and `fromisocalendar`, which improves the error message and fixes a failed assertion when parsing invalid ISO 8601 years using one of the "ISO weeks" formats. --------- (cherry picked from commit d5f1139) Co-authored-by: Vlad4896 <[email protected]> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>

bedevere-app · 2024-04-09T17:54:03Z

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

…117543) (#117689) gh-117534: Add checking for input parameter in iso_to_ymd (GH-117543) Moves the validation for invalid years in the C implementation of the `datetime` module into a common location between `fromisoformat` and `fromisocalendar`, which improves the error message and fixes a failed assertion when parsing invalid ISO 8601 years using one of the "ISO weeks" formats. --------- (cherry picked from commit d5f1139) Co-authored-by: Vlad4896 <[email protected]> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>

…on#117543) Moves the validation for invalid years in the C implementation of the `datetime` module into a common location between `fromisoformat` and `fromisocalendar`, which improves the error message and fixes a failed assertion when parsing invalid ISO 8601 years using one of the "ISO weeks" formats. --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>

Vlad4896 requested review from pganssle and abalkin as code owners April 4, 2024 13:27

bedevere-app bot mentioned this pull request Apr 4, 2024

./Modules/_datetimemodule.c:290: days_before_year: Assertion `year >= 1' failed. #117534

Closed

bedevere-app bot added the awaiting review label Apr 4, 2024

serhiy-storchaka reviewed Apr 4, 2024

View reviewed changes

Vlad4896 added 2 commits April 5, 2024 12:59

pythongh-117534: Add checking for input parameter in iso_to_ymd

f77aa5b

pythongh-117534: Add checking for input parameter in iso_to_ymd (upda…

124fd78

…ted)

Vlad4896 force-pushed the fix-issue-117534 branch from 09730d4 to 124fd78 Compare April 5, 2024 12:10

serhiy-storchaka reviewed Apr 6, 2024

View reviewed changes

Lib/test/datetimetester.py Outdated Show resolved Hide resolved

Merge branch 'python:main' into fix-issue-117534

0248641

Add NEWS entry

8053d94

blurb-it bot and others added 2 commits April 8, 2024 09:44

📜🤖 Added by blurb_it.

82883c4

Remove wrong named NEWS

c444cff

Update 2024-04-08-09-44-29.gh-issue-117534.54ZE_n.rst

6c6ac98

Correct syntax.

pganssle reviewed Apr 8, 2024

View reviewed changes

Lib/test/datetimetester.py Outdated Show resolved Hide resolved

pganssle reviewed Apr 8, 2024

View reviewed changes

Misc/NEWS.d/next/C API/2024-04-08-09-44-29.gh-issue-117534.54ZE_n.rst Outdated Show resolved Hide resolved

Rework test cases

d9910fb

pganssle requested changes Apr 8, 2024

View reviewed changes

bedevere-app bot removed the awaiting review label Apr 8, 2024

bedevere-app bot added the awaiting changes label Apr 8, 2024

Vlad4896 added 2 commits April 8, 2024 20:21

Update 2024-04-08-09-44-29.gh-issue-117534.54ZE_n.rst

a3e20c3

Update wordings.

Update ACKS

8a32bc7

pganssle approved these changes Apr 8, 2024

View reviewed changes

Misc/ACKS Outdated Show resolved Hide resolved

bedevere-app bot added awaiting merge and removed awaiting changes labels Apr 8, 2024

Vlad4896 and others added 2 commits April 8, 2024 20:35

Update ACKS

6bc747f

remove trailing space

348743e

serhiy-storchaka approved these changes Apr 9, 2024

View reviewed changes

pganssle merged commit d5f1139 into python:main Apr 9, 2024

bedevere-app bot removed the awaiting merge label Apr 9, 2024

pganssle added the needs backport to 3.12 only security fixes label Apr 9, 2024

bedevere-app bot removed the needs backport to 3.12 only security fixes label Apr 9, 2024

Uh oh!

gh-117534: Add checking for input parameter in iso_to_ymd #117543

gh-117534: Add checking for input parameter in iso_to_ymd #117543

Uh oh!

Conversation

Vlad4896 commented Apr 4, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 4, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app bot commented Apr 4, 2024

Uh oh!

serhiy-storchaka Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

Vlad4896 Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

Vlad4896 Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Apr 5, 2024

Uh oh!

pganssle commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bedevere-app bot commented Apr 8, 2024

Uh oh!

bedevere-app bot commented Apr 8, 2024

Uh oh!

Vlad4896 commented Apr 8, 2024

Uh oh!

Uh oh!

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Apr 8, 2024

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington-app bot commented Apr 9, 2024

Uh oh!

bedevere-app bot commented Apr 9, 2024

Uh oh!

Uh oh!

Vlad4896 commented Apr 4, 2024 •

edited by bedevere-app bot

Loading

ghost commented Apr 4, 2024 •

edited by ghost

Loading

pganssle commented Apr 5, 2024 •

edited

Loading