Skip to content

avoid integer overflow when decoding large time numbers #1965

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

Closed
wants to merge 4 commits into from
Closed

avoid integer overflow when decoding large time numbers #1965

wants to merge 4 commits into from

Conversation

j08lue
Copy link
Contributor

@j08lue j08lue commented Mar 5, 2018

The issue: int32 time data in seconds or so leads to an overflow in time decoding.

This is in way the back side of #1859: By ensuring that _NS_PER_TIME_DELTA is integer, we got rid of round-off errors that were due to casting to float but now we are getting int overflow in this line:

flat_num_dates_ns_int = (flat_num_dates *
_NS_PER_TIME_DELTA[delta]).astype(np.int64)

e.g. '2001-01-01' in seconds since 1970-01-01 means np.array([978307200]) * int(1e9) which gives 288686080 that gets decoded to '1970-01-01T00:00:00.288686080' -- note also the trailing digits. Something is very wrong here.

  • Tests added (for all bug fixes or enhancements)
  • Tests passed (for all non-documentation changes)
  • Fully documented, including whats-new.rst for all changes

@j08lue j08lue changed the title test decoding large ints test decoding time in large ints Mar 5, 2018
@j08lue
Copy link
Contributor Author

j08lue commented Mar 5, 2018

Seems like this issue only exists on Windows. See https://ci.appveyor.com/project/shoyer/xray/build/1.0.3782/job/yjodrbenev1tw5y7#L4686

(The Travis failure is due to a failing build.)

@j08lue
Copy link
Contributor Author

j08lue commented Mar 5, 2018

I can see netcdftime is casting the time numbers to float64:

https://github.com/Unidata/netcdftime/blob/e745434547de728d53cde6316cae75b62db102e2/netcdftime/_netcdftime.pyx#L241

like I also suggested for #1863 (comment).

How about we do that, too?

@shoyer
Copy link
Member

shoyer commented Mar 9, 2018

OK, this seems reasonable to me. Can you add a brief note on the bug fix in whats-new?

@j08lue j08lue changed the title test decoding time in large ints avoid integer overflow when decoding large time numbers Mar 9, 2018
@j08lue
Copy link
Contributor Author

j08lue commented Mar 9, 2018

Can you add a brief note on the bug fix in whats-new?

Done.

@jhamman
Copy link
Member

jhamman commented May 1, 2018

@j08lue - mind freshening up this PR? It seems there are some conflicts that need to be resolved before merging.

@j08lue
Copy link
Contributor Author

j08lue commented May 1, 2018

mind freshening up this PR

I deleted my fork in the meantime. I opened a new PR at #2096 and fixed the merge conflict there.

@j08lue j08lue closed this May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants