Fix strange and wrong code around DateTime64 by alexey-milovidov · Pull Request #11875 · ClickHouse/ClickHouse

alexey-milovidov · 2020-06-22T22:09:14Z

Changelog category (leave one):

Bug Fix

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix potential floating point exception when parsing DateTime64. This fixes #11374.

alexey-milovidov · 2020-06-22T22:10:14Z

src/DataTypes/DataTypeDateTime64.cpp

 }

 DataTypeDateTime64::DataTypeDateTime64(UInt32 scale_, const TimezoneMixin & time_zone_info)
-    : DataTypeDecimalBase<DateTime64>(DecimalUtils::maxPrecision<DateTime64>() - scale_, scale_),


This is very strange and ridiculous. Maybe this constructor is not used at all?

alexey-milovidov · 2020-06-22T22:11:13Z

src/DataTypes/DataTypeDecimalBase.h

        if (unlikely(precision < 1 || precision > maxPrecision()))
            throw Exception("Precision " + std::to_string(precision) + " is out of bounds", ErrorCodes::ARGUMENT_OUT_OF_BOUND);
-        if (unlikely(scale < 0 || static_cast<UInt32>(scale) > maxPrecision()))
+        if (unlikely(scale > maxPrecision()))


This was strange code from @4ertus2
Also I'm surprised that tautological comparison is not detected by compiler.

alexey-milovidov · 2020-06-22T22:11:33Z

src/IO/ReadHelpers.h

+                    /// number of decimal digits so far is close to the max for given type.
+                    /// Example: 20 * 10 will overflow Int8.
+
+                    if (buf.count() - initial_pos + 1 >= std::numeric_limits<T>::max_digits10)


Look at this + 1.

alexey-milovidov · 2020-06-22T22:11:58Z

src/IO/ReadHelpers.h

                    {
-                        if (common::mulOverflow(res, static_cast<decltype(res)>(10), res)
-                            || common::addOverflow(res, static_cast<decltype(res)>(*buf.position() - '0'), res))
+                        T signed_res = res;


We must check for overflow inside (possibly) signed data type.

alexey-milovidov · 2020-06-22T22:12:27Z

src/IO/ReadHelpers.h

+
+                    if (buf.count() - initial_pos + 1 >= std::numeric_limits<T>::max_digits10)
                    {
-                        if (common::mulOverflow(res, static_cast<decltype(res)>(10), res)


This decltype is ugly.

alexey-milovidov · 2020-06-22T22:12:40Z

src/IO/ReadHelpers.h

    }

 end:
-    x = negative ? -res : res;


And we must check for overflow here.

alexey-milovidov · 2020-06-22T22:13:07Z

src/IO/ReadHelpers.h

        return ReturnType(false);
    }

-    DB::DecimalUtils::DecimalComponents<DateTime64::NativeType> c{static_cast<DateTime64::NativeType>(whole), 0};


c is bad name for variable.

alexey-milovidov · 2020-06-22T22:13:44Z

src/IO/ReadHelpers.h

    if (!buf.eof() && *buf.position() == '.')
    {
-        buf.ignore(1); // skip separator
-        const auto pos_before_fractional = buf.count();


This is absolutely wrong.
Mind +123 or +1---23.

alexey-milovidov · 2020-06-22T22:15:29Z

src/IO/ReadHelpers.h

-        }
-        else if (adjust_scale < 0)
-        {
-            c.fractional /= common::exp10_i64(-1 * adjust_scale);


adjust_scale can appear too big.
For example, if
scale is 0
and we have parsed
1111111111111111111
(this number successfully passes overflow check)
then we will have to divide by
10000000000000000000
that does not fit in Int64
and buffer overflow happens.

(this is the main reason for the bug)

alexey-milovidov · 2020-06-22T22:16:19Z

src/IO/ReadHelpers.h

-        }
-        else if (adjust_scale < 0)
-        {
-            c.fractional /= common::exp10_i64(-1 * adjust_scale);


Writing -1 * x is strange, we can write just -x.

alexey-milovidov · 2020-06-22T22:17:13Z

src/IO/ReadHelpers.h

    {
-        buf.ignore(1); // skip separator
-        const auto pos_before_fractional = buf.count();
-        if (!tryReadIntText<ReadIntTextCheckOverflow::CHECK_OVERFLOW>(c.fractional, buf))


This is also wrong, because we don't need to parse integer (like -123).
We just need to parse a stream of digits (like 001).

alexey-milovidov · 2020-06-22T22:17:50Z

@Enmk I highlighted all the bugs in comments.

qoega · 2020-06-23T07:43:51Z

Relevant tests:

[ RUN      ] data_type/LeastSuperTypeTest.getLeastSupertype/20
unknown file: Failure
C++ exception with description "Scale 12 is too large for DateTime64. Maximum is up to nanoseconds (9)." thrown in SetUp().
[  FAILED  ] data_type/LeastSuperTypeTest.getLeastSupertype/20, where GetParam() = TypesTestCase{"DateTime DateTime64(12)", "DateTime64(8)"} (0 ms)
[ RUN      ] data_type/LeastSuperTypeTest.getLeastSupertype/21
unknown file: Failure
C++ exception with description "Scale 15 is too large for DateTime64. Maximum is up to nanoseconds (9)." thrown in SetUp().
[  FAILED  ] data_type/LeastSuperTypeTest.getLeastSupertype/21, where GetParam() = TypesTestCase{"Date DateTime64(15)", "DateTime64(13)"} (1 ms)

alexey-milovidov · 2020-06-23T17:34:02Z

No surprise: unit test is wrong, getLeastSuperType implementation is also wrong.

alexey-milovidov · 2020-06-23T19:54:09Z

I will also add a functional test to demonstrate what code was totally and absolutely wrong.

…a2fa7a83f36dd85e7f748d1579637 Cherry pick #11875 to 20.5: Fix strange and wrong code around DateTime64

Backport #11875 to 20.5: Fix strange and wrong code around DateTime64

Fix strange and wrong code around DateTime64 (cherry picked from commit c2fba51)

alexey-milovidov added 7 commits June 22, 2020 23:32

Style

b136999

constexpr intExp10

75357ab

Fix bugs in DateTime64 parsing

4ace4b4

Added a test

0c063d2

Fix wrong code

1191679

Fix strange code

112f615

Update tests

2f79d30

alexey-milovidov requested a review from Enmk June 22, 2020 22:09

alexey-milovidov commented Jun 22, 2020

View reviewed changes

blinkov added the pr-bugfix Pull request with bugfix, not backported by default label Jun 22, 2020

alexey-milovidov commented Jun 22, 2020

View reviewed changes

src/IO/ReadHelpers.h

}

end:

x = negative ? -res : res;

Copy link
Copy Markdown

Member Author

alexey-milovidov Jun 22, 2020

Choose a reason for hiding this comment

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

And we must check for overflow here.

alexey-milovidov commented Jun 22, 2020

View reviewed changes

alexey-milovidov added 2 commits June 23, 2020 20:39

Fix bad code

c51c265

Merge branch 'master' into fix-fpe-datetime64

cda2687

Added a test

25607be

alexey-milovidov merged commit c2fba51 into master Jun 24, 2020

alexey-milovidov deleted the fix-fpe-datetime64 branch June 24, 2020 09:54

abyss7 mentioned this pull request Jun 24, 2020

Cherry pick to 20.4: Fix strange and wrong code around DateTime64 #11924

Merged

abyss7 mentioned this pull request Jun 25, 2020

Cherry pick #11875 to 20.5: Fix strange and wrong code around DateTime64 #11958

Merged

abyss7 added a commit that referenced this pull request Jun 25, 2020

Merge pull request #11958 from ClickHouse/cherrypick/20.5/c2fba5179bc…

0c4f014

…a2fa7a83f36dd85e7f748d1579637 Cherry pick #11875 to 20.5: Fix strange and wrong code around DateTime64

abyss7 mentioned this pull request Jun 25, 2020

Backport #11875 to 20.5: Fix strange and wrong code around DateTime64 #11965

Merged

alexey-milovidov added a commit that referenced this pull request Jun 27, 2020

Merge pull request #11965 from ClickHouse/backport/20.5/11875

a12a7cd

Backport #11875 to 20.5: Fix strange and wrong code around DateTime64

alesapin added the v20.3-conflicts label Jul 10, 2020

alesapin pushed a commit that referenced this pull request Jul 10, 2020

Merge pull request #11875 from ClickHouse/fix-fpe-datetime64

abee8f0

Fix strange and wrong code around DateTime64 (cherry picked from commit c2fba51)

alesapin added v20.3-backported and removed v20.3-conflicts labels Jul 10, 2020

alesapin pushed a commit that referenced this pull request Jul 10, 2020

Merge pull request #11875 from ClickHouse/fix-fpe-datetime64

222445f

Fix strange and wrong code around DateTime64 (cherry picked from commit c2fba51)

alesapin added the v20.4-backported label Jul 10, 2020

qoega added the no-docs-needed label Sep 2, 2020

Conversation

alexey-milovidov commented Jun 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Jun 22, 2020

Uh oh!

qoega commented Jun 23, 2020

Uh oh!

alexey-milovidov commented Jun 23, 2020

Uh oh!

alexey-milovidov commented Jun 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alexey-milovidov Jun 22, 2020 •

edited

Loading