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.

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

bpo-26669: Fix nan arg value error in pytime.c #3085

Merged
merged 4 commits into from Sep 8, 2017

Conversation

Projects
None yet
9 participants
@hahnlee
Contributor

hahnlee commented Aug 13, 2017

  • Make value error message for nan value
    Previously, it did not show error message in arm system
  • Add test case for nan value

https://bugs.python.org/issue26669

@the-knights-who-say-ni

This comment has been minimized.

the-knights-who-say-ni commented Aug 13, 2017

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@hahnlee hahnlee changed the title from bpo-26669: Fix nan input error in pytime.c to bpo-26669: Fix nan arg value error in pytime.c Aug 13, 2017

@hahnlee hahnlee closed this Aug 13, 2017

@hahnlee hahnlee reopened this Aug 13, 2017

@@ -155,6 +155,10 @@ _PyTime_ObjectToTime_t(PyObject *obj, time_t *sec, _PyTime_round_t round)
volatile double d;

d = PyFloat_AsDouble(obj);
if (isnan(d)) {
PyErr_SetString(PyExc_ValueError, "Invalid Value");

This comment has been minimized.

@wkschwartz

wkschwartz Aug 15, 2017

Might be helpful for the error message specifically to mention that the value is NaN.

@pitrou

This comment has been minimized.

Member

pitrou commented Aug 29, 2017

@Haypo, do you have any comments about this?

@@ -489,6 +489,10 @@ def test_localtime_failure(self):
self.assertRaises(OSError, time.localtime, invalid_time_t)
self.assertRaises(OSError, time.ctime, invalid_time_t)

# Issue #26669: check for localtime() failure
self.assertRaises(ValueError, time.localtime, float("nan"))
self.assertRaises(ValueError, time.ctime, float("nan"))

This comment has been minimized.

@vstinner

vstinner Aug 31, 2017

Member

Ok for "functional" tests", but I also would like to see unit tests on the following _testcapi functions:

  • pytime_object_to_time_t
  • pytime_object_to_timeval
  • pytime_object_to_timespec
  • PyTime_FromSeconds
  • PyTime_FromSecondsObject
  • PyTime_AsSecondsDouble

See CPyTimeTestCase in test_time.py.

@bedevere-bot

This comment has been minimized.

bedevere-bot commented Aug 31, 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 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!

@pitrou

This comment has been minimized.

Member

pitrou commented Aug 31, 2017

Hmm... Did @warsaw hack the bot?

@vstinner

This comment has been minimized.

Member

vstinner commented Sep 4, 2017

@sn0wle0pard: Would you mind to add requested unit tests? Otherwise, I might try to modify your PR to add them myself?

@hahnlee

This comment has been minimized.

Contributor

hahnlee commented Sep 6, 2017

@Haypo I'll add test case as soon as possible.

@hahnlee

This comment has been minimized.

Contributor

hahnlee commented Sep 7, 2017

@Haypo I'm really sorry for the delay in writing the code.

Summary

System: Ubuntu 16.04 x86
pytime_object_to_timeval got error.
If the result is the same as expected, I will replace the passed test code with the changed _PyTime_FromSecondsObject and _PyTime_ObjectToDenominator.

  • pytime_object_to_time_t: pass
  • pytime_object_to_timeval : error assert(0 <= *usec && *usec < SEC_TO_US);
  • pytime_object_to_timespec: error assert(0 <= *nsec && *nsec < SEC_TO_NS);
  • PyTime_FromSecondsObject: pass (When add some C code)
  • PyTime_FromSeconds: TypeError: integer argument expected, got float
  • PyTime_FromSecondsObject: TypeError: integer argument expected, got float

Result

I tested the following code in _check_rounding.

# test nan
for time_rnd, _ in ROUNDING_MODES:
    with self.assertRaises(ValueError):
        pytime_converter(float('nan'), time_rnd)

Here are the results:

Python/pytime.c:116: _PyTime_DoubleToDenominator: Assertion `0.0 <= floatpart && floatpart < denominator' failed.

...so I tested one by one.
pytime_object_to_time_t: pass
pytime_object_to_timeval: error, pytime.c:116
pytime_object_to_timespec: error, pytime.c:116

PyTime_FromSeconds: error TypeError: integer argument expected, got float (did not use time_rnd)
PyTime_AsSecondsDouble: has same error PyTime_FromSecondsObject (did not use time_rnd)

And PyTime_FromSecondsObject: did not raise
_PyTime_FromSecondsObject call _PyTime_FromObject and _PyTime_FromObject has same logic at _PyTime_ObjectToTime_t (check float but do not check nan)
So I changed _PyTime_FromObject and passed test for _PyTime_FromSecondsObject.

In case pytime_object_to_timeval and pytime_object_to_timespec they call _PyTime_ObjectToDenominator and _PyTime_ObjectToDenominator has PyFloat_AsDouble too

I add Py_IS_NAN(d) at _PyTime_ObjectToDenominator

but pytime_object_to_timeval and pytime_object_to_timespec got assert error
it mean in case _PyTime_ObjectToDenominator, *obj float('nan') has problem in usec or nsec

i think PyTime_FromSeconds and PyTime_AsSecondsDouble seem reasonable.
but i'm not sure pytime_object_to_timeval and pytime_object_to_timespec has reasonable error.

If the result is the same as expected, I will replace the passed test code with the changed _PyTime_FromSecondsObject and _PyTime_ObjectToDenominator.

@hahnlee

This comment has been minimized.

Contributor

hahnlee commented Sep 8, 2017

pytime_object_to_timeval also has same error on macOS sierra

@hahnlee

This comment has been minimized.

Contributor

hahnlee commented Sep 8, 2017

oh... in case NaN numerator is zero. i add some code for numerator zero. then it pass all test cases.

@vstinner vstinner added the skip news label Sep 8, 2017

@vstinner

This comment has been minimized.

Member

vstinner commented Sep 8, 2017

Ok, it now LGTM!

@vstinner vstinner merged commit 829dacc into python:master Sep 8, 2017

4 checks passed

bedevere/issue-number Issue number 26669 found
Details
bedevere/news "skip news" label found
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

miss-islington commented Sep 8, 2017

🐍🍒🤖 Thanks @sn0wle0pard for the PR, and @Haypo for merging it 🌮🎉.I'm working now to backport this PR to: 3.6.

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

[3.6] bpo-26669: Fix nan arg value error in pytime.c (pythonGH-3085)
* Fix GH-26669

* Modify NaN check function and error message

* Fix pytime.c when arg is nan

* fix whitespace
(cherry picked from commit 829dacc)
@bedevere-bot

This comment has been minimized.

bedevere-bot commented Sep 8, 2017

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

Mariatta added a commit that referenced this pull request Sep 9, 2017

[3.6] bpo-26669: Fix nan arg value error in pytime.c (GH-3085) (GH-3467)
* Modify NaN check function and error message
* Fix pytime.c when arg is nan
* fix whitespace
(cherry picked from commit 829dacc)

@hahnlee hahnlee deleted the hahnlee:fix-issue-26669 branch Sep 11, 2017

daxlab added a commit to daxlab/cpython that referenced this pull request Oct 1, 2017

bpo-26669: Fix nan arg value error in pytime.c (python#3085)
* Fix #26669

* Modify NaN check function and error message

* Fix pytime.c when arg is nan

* fix whitespace

@Mariatta Mariatta removed the awaiting merge label Oct 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment