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

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

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

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

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

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".

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.

I didn't expect the Spanish Inquisition!

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

Choose a reason for hiding this comment

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

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().

Choose a reason for hiding this comment

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

agh, of course.

Choose a reason for hiding this comment

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

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().)

Choose a reason for hiding this comment

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

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?

And if you don't make the requested changes, you will be put in the comfy chair!

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

…the ratio checks inside a helper-func

I didn't expect the Spanish Inquisition!

Choose a reason for hiding this comment

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

LGTM. I have added just a nitpick.

		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):

Choose a reason for hiding this comment

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

No starting underscore is needed.

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Sep 19, 2017

		with self.assertRaises(TypeError):
		timedelta() * get_bad_float(1 << 1000)

		for bad_ratio in [(), (42, ), (1, 2, 3)]:

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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.

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

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