Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34.1k
bpo-31293: Fix crashes in truediv and mul of a timedelta by a float with a bad as_integer_ratio() method#3227
Conversation
Lib/test/datetimetester.py Outdated
| class BadFloat(float): | ||
| def as_integer_ratio(self): | ||
| return 1 << 1000 | ||
| self.assertRaises(TypeError, truediv, timedelta(), BadFloat()) |
There was a problem hiding this comment.
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 awith assertRaises(..) block:
withself.assertRaises(TypeError):timedelta()/BadFloat()
Modules/_datetimemodule.c Outdated
| goto error; | ||
| if (!PyTuple_Check(ratio)) { | ||
| PyErr_SetString(PyExc_TypeError, | ||
| "Can't multiply timedelta object by float with " |
There was a problem hiding this comment.
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".
bedevere-bot commentedAug 28, 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 |
orenmn commentedAug 28, 2017
I didn't expect the Spanish Inquisition! |
bedevere-bot commentedAug 28, 2017
Nobody expects the Spanish Inquisition! @abalkin: please review the changes made to this pull request. |
Modules/_datetimemodule.c Outdated
| ratio = _PyObject_CallMethodId(floatobj, &PyId_as_integer_ratio, NULL); | ||
| if (ratio == NULL) | ||
| goto error; | ||
| if (!PyTuple_Check(ratio)) { |
There was a problem hiding this comment.
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 inmultiply_float_timedelta() anddivide_timedelta_int().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
agh, of course.
There was a problem hiding this comment.
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().)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The code ofmultiply_float_timedelta() anddivide_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, andmultiply_float_timedelta() anddivide_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 commentedAug 28, 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 And if you don't make the requested changes,you will be put in the comfy chair! |
orenmn commentedAug 28, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I am working to fix the problems that Serhiy pointed out. please don't merge. |
…the ratio checks inside a helper-func
orenmn commentedAug 28, 2017
I didn't expect the Spanish Inquisition! |
bedevere-bot commentedAug 28, 2017
Nobody expects the Spanish Inquisition! @serhiy-storchaka,@abalkin: please review the changes made to this pull request. |
serhiy-storchaka left a comment
There was a problem hiding this comment.
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.
Lib/test/datetimetester.py Outdated
| 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): |
There was a problem hiding this comment.
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.
miss-islington commentedSep 19, 2017
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 commit865e4b4)
bedevere-bot commentedSep 19, 2017
GH-3654 is a backport of this pull request to the3.6 branch. |
| with self.assertRaises(TypeError): | ||
| timedelta() * get_bad_float(1 << 1000) | ||
| for bad_ratio in [(), (42, ), (1, 2, 3)]: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
miss-islington commentedSep 30, 2017
Thanks@orenmn for the PR, and@serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6. |
miss-islington commentedSep 30, 2017
Sorry,@orenmn and@serhiy-storchaka, I could not cleanly backport this to |
Uh oh!
There was an error while loading.Please reload this page.
_datetimemodule.c- add checks whether as_integer_ratio() returned a tuple.datetimetester.py- add tests to verify that the crashes are no more.https://bugs.python.org/issue31293