Skip to content

[hotfix] Fixed an issue where overflow from Long to Int was unsafe#1618

Open
XuQianJin-Stars wants to merge 1 commit into
apache:mainfrom
XuQianJin-Stars:hotfixForLongToInt
Open

[hotfix] Fixed an issue where overflow from Long to Int was unsafe#1618
XuQianJin-Stars wants to merge 1 commit into
apache:mainfrom
XuQianJin-Stars:hotfixForLongToInt

Conversation

@XuQianJin-Stars

Copy link
Copy Markdown
Contributor

Fixed an issue where overflow from Long to Int was unsafe.

@danny0405
danny0405 force-pushed the master branch 2 times, most recently from 80f411d to ca27fe9 Compare November 30, 2019 07:52
if (v instanceof Long) {
// TIMESTAMP
v2 = (int) (((Long) v) / TimeUnit.DAY.multiplier.longValue());
v2 = Math.toIntExact(((Long) v) / TimeUnit.DAY.multiplier.longValue());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One think I don't understand here is v2 is still used as long in the following code DateTimeUtils.unixDateExtract(timeUnitRange, v2);. Maybe the right fix is do not do long -> int conversion?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

One think I don't understand here is v2 is still used as long in the following code DateTimeUtils.unixDateExtract(timeUnitRange, v2);. Maybe the right fix is do not do long -> int conversion?

v2 = (Integer) v; should we cast to long too? If it was you would have to handle the int and long cases separately for DateTimeUtils.unixDateExtract (timeUnitRange, v2);

@danny0405 danny0405 Dec 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do you think an integer days would overflow ? Can you give a test case ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Based on math, if you use max_long / milliseoncd_per_day, the number will still exceed int_max. So technically overflow exists.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is the upper/max value of timestamp million-seconds ?

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