bpo-31293: Fix crashes in truediv and mul of a timedelta by a float with a bad as_integer_ratio() method by orenmn · Pull Request #3227 · python/cpython

orenmn · 2017-08-28T15:35:56Z

in _datetimemodule.c - add checks whether as_integer_ratio() returned a tuple.
in datetimetester.py - add tests to verify that the crashes are no more.

https://bugs.python.org/issue31293

abalkin · 2017-08-28T15:56:32Z

+        class BadFloat(float):
+            def as_integer_ratio(self):
+                return 1 << 1000
+        self.assertRaises(TypeError, truediv, timedelta(), BadFloat())


The modern style to test for exceptions is to use a with assertRaises(..) block:

with self.assertRaises(TypeError): timedelta() / BadFloat()

abalkin · 2017-08-28T16:06:28Z

        goto error;
+    if (!PyTuple_Check(ratio)) {
+        PyErr_SetString(PyExc_TypeError,
+                        "Can't multiply timedelta object by float with "


It looks like other error messages in this file start with a lowercase letter. Let's keep the style consistent.

I also wonder if a better message would be "unexpected return type from as_integer_ratio(): expected tuple, got %s".

bedevere-bot · 2017-08-28T16:06:56Z

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.

orenmn · 2017-08-28T17:02:27Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-08-28T17:02:29Z

Nobody expects the Spanish Inquisition!

@abalkin: please review the changes made to this pull request.

serhiy-storchaka · 2017-08-28T16:59:10Z

    ratio = _PyObject_CallMethodId(floatobj, &PyId_as_integer_ratio, NULL);
    if (ratio == NULL)
        goto error;
+    if (!PyTuple_Check(ratio)) {


This is not enough. The size of the tuple should be 2.

Perhaps the code can be shared in multiply_float_timedelta() and divide_timedelta_int().

agh, of course.

how can code be shared between these two functions? (I wrote a helper-function to share my patch's code between multiply_float_timedelta() and truedivide_timedelta_float().)

The code of multiply_float_timedelta() and divide_timedelta_int() is almost the same. It is enough a one boolean parameter to distinguish multiplication from division. All code can be moved in a separate function, and multiply_float_timedelta() and divide_timedelta_int() will call it with additional argument 0 or 1.

Usually a refactoring is made only in develop version, but I think this one can be done in a bugfix change. It is enough simple and can help to fix other bugs if they will be found. What are your thoughts @abalkin?

bedevere-bot · 2017-08-28T17:03:48Z

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!

orenmn · 2017-08-28T17:24:34Z

I am working to fix the problems that Serhiy pointed out. please don't merge.

…the ratio checks inside a helper-func

orenmn · 2017-08-28T20:43:54Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-08-28T20:43:57Z

Nobody expects the Spanish Inquisition!

@serhiy-storchaka, @abalkin: please review the changes made to this pull request.

serhiy-storchaka

LGTM. I have added just a nitpick.

serhiy-storchaka · 2017-09-19T11:45:10Z

+    def test_issue31293(self):
+        # The interpreter shouldn't crash in case a timedelta is divided or
+        # multiplied by a float with a bad as_integer_ratio() method.
+        def _get_bad_float(bad_ratio):


No starting underscore is needed.

miss-islington · 2017-09-19T12:58:13Z

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

…loat with a bad as_integer_ratio() method. (pythonGH-3227) (cherry picked from commit 865e4b4)

bedevere-bot · 2017-09-19T12:58:26Z

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

…loat with a bad as_integer_ratio() method. (GH-3227) (#3654) (cherry picked from commit 865e4b4)

abalkin · 2017-09-29T23:29:16Z

+        with self.assertRaises(TypeError):
+            timedelta() * get_bad_float(1 << 1000)
+
+        for bad_ratio in [(), (42, ), (1, 2, 3)]:


What would happen if as_integer_ratio() returns a pair of non-integers? A pair of strings? A pair of floats? Could you add tests for these scenarios?

TypeError will be raised on the attempt to divide on a string.

It doesn't matter what errors are raised with a bad as_integer_ratio(), but the code shouldn't crash.

miss-islington · 2017-09-30T17:22:40Z

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

miss-islington · 2017-09-30T17:22:48Z

Sorry, @orenmn and @serhiy-storchaka, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 865e4b4f630e2ae91e61239258abb58b488f1d65 3.6

orenmn added 2 commits August 28, 2017 17:53

init commit

6338b6a

update the test

6c0f6e4

the-knights-who-say-ni added the CLA signed label Aug 28, 2017

bedevere-bot added the awaiting review label Aug 28, 2017

abalkin requested changes Aug 28, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Aug 28, 2017

fix tests style, and improve error msgs

08969fc

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 28, 2017

serhiy-storchaka requested changes Aug 28, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Aug 28, 2017

abalkin approved these changes Aug 28, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Aug 28, 2017

check that as_integer_ratio() returned a 2-tuple, add tests, and put …

a7b4e5a

…the ratio checks inside a helper-func

bedevere-bot added awaiting change review and removed awaiting merge labels Aug 28, 2017

serhiy-storchaka approved these changes Sep 19, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 19, 2017

remove redandant starting underscore

8299f61

serhiy-storchaka added needs backport to 3.6 type-bug An unexpected behavior, bug, or error labels Sep 19, 2017

serhiy-storchaka merged commit 865e4b4 into python:master Sep 19, 2017

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 19, 2017

[3.6] bpo-31293: Fix crashes in truediv and mul of a timedelta by a f…

349355e

…loat with a bad as_integer_ratio() method. (pythonGH-3227) (cherry picked from commit 865e4b4)

bedevere-bot removed the needs backport to 3.6 label Sep 19, 2017

serhiy-storchaka pushed a commit that referenced this pull request Sep 19, 2017

[3.6] bpo-31293: Fix crashes in truediv and mul of a timedelta by a f…

f37dd11

…loat with a bad as_integer_ratio() method. (GH-3227) (#3654) (cherry picked from commit 865e4b4)

abalkin reviewed Sep 29, 2017

View reviewed changes

serhiy-storchaka added the needs backport to 3.6 label Sep 30, 2017

serhiy-storchaka removed the needs backport to 3.6 label Sep 30, 2017

Mariatta removed the awaiting merge label Oct 8, 2017

Uh oh!

Conversation

orenmn commented Aug 28, 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!

bedevere-bot commented Aug 28, 2017

Uh oh!

orenmn commented Aug 28, 2017

Uh oh!

bedevere-bot commented Aug 28, 2017

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!

bedevere-bot commented Aug 28, 2017

Uh oh!

orenmn commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orenmn commented Aug 28, 2017

Uh oh!

bedevere-bot commented Aug 28, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Sep 19, 2017

Uh oh!

bedevere-bot commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Sep 30, 2017

Uh oh!

miss-islington commented Sep 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

orenmn commented Aug 28, 2017 •

edited by bedevere-bot

Loading

orenmn commented Aug 28, 2017 •

edited

Loading