Skip to content

bpo-31752: Fix possible crash in timedelta constructor called with custom integers. #3947

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
serhiy-storchaka merged 2 commits into from
Oct 23, 2017
Merged

bpo-31752: Fix possible crash in timedelta constructor called with custom integers. #3947

serhiy-storchaka merged 2 commits into from
Oct 23, 2017

Conversation

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Oct 10, 2017 •
edited by bedevere-bot
Loading

Bad remainder in divmod() in intermediate calculations caused an assertion failure.

https://bugs.python.org/issue31752

@serhiy-storchaka
bpo-31752: Fix possible crash in timedelta constructor called with cu…
7911378 
…stom integers.

Bad remainder in divmod() in intermediate calculations caused an assertion failure.
@serhiy-storchaka serhiy-storchaka added needs backport to 2.7 type-bug An unexpected behavior, bug, or error labels Oct 10, 2017
vstinner
vstinner approved these changes Oct 11, 2017
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except of a minor comment.

Lib/test/datetimetester.py
def __mul__(self, other):
return Prod()

class Prod:
Copy link
Member

@vstinner vstinner Oct 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe inherit from int as well?

Copy link
Member Author

@serhiy-storchaka serhiy-storchaka Oct 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it shouldn't be inherited from int, otherwise __radd__ would not be called.

Copy link
Member

@vstinner vstinner Oct 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok.

terryjreedy
terryjreedy previously requested changes Oct 13, 2017
View reviewed changes
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Win10, this changes a noisy crash, with crash box and printed message, to a silent crash
f:\dev\3x>python f:/python/mypy/tem.py
Running Debug|Win32 interpreter...

f:\dev\3x> # printed immediately after above

which to me is worse. I verified that datetimetester.py has the new test_issue31752.

@bedevere-bot
Copy link

bedevere-bot commented Oct 13, 2017

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.

@serhiy-storchaka
Copy link
Member Author

serhiy-storchaka commented Oct 22, 2017

What is f:/python/mypy/tem.py? I just have installed mypy, but don't see the file tem.py there. Tests are passed on Windows to me.

@terryjreedy
Copy link
Member

terryjreedy commented Oct 22, 2017

mypy is my personal directory of miscellaneous .py files, which a have had for at least a decade. I guess I should rename it. tem.py should have been the code in the initial post with a print added. I should have been clearer.

Without the patch, with a fresh build,
python -m test test_datetime passes (and ditto for whole suite, without test_datetime added)
python -m unittest test.test_datetime passes
python -m test.test_datetime crashes in test_strftime with 'Debug assertion failed' in line 1184 of min\kernal\crts\ucrt\src\appcrt\time\wcsftime.cpp. I don't know what to make of this.

I will apply the patch again, re-build, and try to reproduce what I think I did before, keep better notes, and report again.

@terryjreedy terryjreedy dismissed their stale review October 22, 2017 18:09

I cannot reproduce the negative result.

@terryjreedy
Copy link
Member

terryjreedy commented Oct 22, 2017

This time, printing the time delta of 1 in your opening code works instead of crashing.
python -m test.test_datetime still crashes, but that must be a different issue.

@serhiy-storchaka serhiy-storchaka merged commit 4ffd465 into python:master Oct 23, 2017
@miss-islington
Copy link
Contributor

miss-islington commented Oct 23, 2017

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

@serhiy-storchaka serhiy-storchaka deleted the timedelta-bad-divmod branch October 23, 2017 14:12
@bedevere-bot
Copy link

bedevere-bot commented Oct 23, 2017

GH-4086 is a backport of this pull request to the 3.6 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 23, 2017
@serhiy-storchaka @miss-islington
bpo-31752: Fix possible crash in timedelta constructor called with cu…
56f0035 
…stom integers. (pythonGH-3947)

Bad remainder in divmod() in intermediate calculations caused an assertion failure.
(cherry picked from commit 4ffd465)
@miss-islington
Copy link
Contributor

miss-islington commented Oct 23, 2017

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

serhiy-storchaka pushed a commit that referenced this pull request Oct 23, 2017
@miss-islington @serhiy-storchaka
bpo-31752: Fix possible crash in timedelta constructor called with cu…
6e45d7b 
…stom integers. (GH-3947) (#4086)

Bad remainder in divmod() in intermediate calculations caused an assertion failure.
(cherry picked from commit 4ffd465)
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Oct 23, 2017
@serhiy-storchaka
[2.7] bpo-31752: Fix possible crash in timedelta constructor called w…
4a49928 
…ith custom integers. (pythonGH-3947)

Bad remainder in divmod() in intermediate calculations caused an assertion failure..
(cherry picked from commit 4ffd465)
@serhiy-storchaka serhiy-storchaka mentioned this pull request Oct 23, 2017
Merged
serhiy-storchaka added a commit that referenced this pull request Oct 23, 2017
@serhiy-storchaka
[2.7] bpo-31752: Fix possible crash in timedelta constructor called w…
5ef883b 
…ith custom integers. (GH-3947) (#4088)

Bad remainder in divmod() in intermediate calculations caused an assertion failure..
(cherry picked from commit 4ffd465)
@serhiy-storchaka serhiy-storchaka mentioned this pull request Oct 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@terryjreedy terryjreedy terryjreedy left review comments

Assignees

No one assigned

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@serhiy-storchaka @bedevere-bot @terryjreedy @miss-islington @vstinner @the-knights-who-say-ni