From 115000104bf922c598cb82fc9d46c481d7f57a98 Mon Sep 17 00:00:00 2001 From: zhangstar333 <2561612514@qq.com> Date: Wed, 17 Jul 2024 16:19:48 +0800 Subject: [PATCH 1/2] [Bug](function) fix mod function cause core dump --- be/src/vec/functions/modulo.cpp | 17 +++++++ .../sql_functions/math_functions/test_mod.out | 6 +++ .../math_functions/test_mod.groovy | 49 +++++++++++++++++++ 3 files changed, 72 insertions(+) create mode 100644 regression-test/data/query_p0/sql_functions/math_functions/test_mod.out create mode 100644 regression-test/suites/query_p0/sql_functions/math_functions/test_mod.groovy diff --git a/be/src/vec/functions/modulo.cpp b/be/src/vec/functions/modulo.cpp index 66376b66019273..12867f523303ba 100644 --- a/be/src/vec/functions/modulo.cpp +++ b/be/src/vec/functions/modulo.cpp @@ -33,6 +33,19 @@ namespace doris::vectorized { +template +inline void throw_if_division_leads_to_FPE(A a, B b) { + // http://avva.livejournal.com/2548306.html + // (-9223372036854775808 % -1) will cause coredump directly, so check this case to throw exception, or maybe could return 0 as result + if (UNLIKELY(std::is_signed_v && std::is_signed_v && a == std::numeric_limits::min() && + b == -1)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "Division of minimal signed number by minus one is an undefined " + "behavior, {} % {}. ", + a, b); + } +} + template struct ModuloImpl { using ResultType = typename NumberTraits::ResultOfModulo::Type; @@ -51,6 +64,7 @@ struct ModuloImpl { if constexpr (std::is_floating_point_v) { c[i] = std::fmod((double)a[i], (double)b); } else { + throw_if_division_leads_to_FPE(a[i], b); c[i] = a[i] % b; } } @@ -65,6 +79,7 @@ struct ModuloImpl { if constexpr (std::is_floating_point_v) { return std::fmod((double)a, (double)b); } else { + throw_if_division_leads_to_FPE(a, b); return a % b; } } @@ -94,6 +109,7 @@ struct PModuloImpl { if constexpr (std::is_floating_point_v) { c[i] = std::fmod(std::fmod((double)a[i], (double)b) + (double)b, double(b)); } else { + throw_if_division_leads_to_FPE(a[i], b); c[i] = (a[i] % b + b) % b; } } @@ -108,6 +124,7 @@ struct PModuloImpl { if constexpr (std::is_floating_point_v) { return std::fmod(std::fmod((double)a, (double)b) + (double)b, (double)b); } else { + throw_if_division_leads_to_FPE(a, b); return (a % b + b) % b; } } diff --git a/regression-test/data/query_p0/sql_functions/math_functions/test_mod.out b/regression-test/data/query_p0/sql_functions/math_functions/test_mod.out new file mode 100644 index 00000000000000..6e3ce6219debcd --- /dev/null +++ b/regression-test/data/query_p0/sql_functions/math_functions/test_mod.out @@ -0,0 +1,6 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !sql -- +-2147483648 4 -1 +1 2 3 +5 -9223372036854775808 -1 + diff --git a/regression-test/suites/query_p0/sql_functions/math_functions/test_mod.groovy b/regression-test/suites/query_p0/sql_functions/math_functions/test_mod.groovy new file mode 100644 index 00000000000000..027983ff749c8f --- /dev/null +++ b/regression-test/suites/query_p0/sql_functions/math_functions/test_mod.groovy @@ -0,0 +1,49 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +suite("test_mod") { + def tableName = "test_mod" + sql "set enable_fold_constant_by_be = false;" + sql """DROP TABLE IF EXISTS `test_mod`""" + sql """ CREATE TABLE `test_mod` ( + `k1` int NULL COMMENT "用户id", + `k2` bigint COMMENT "数据灌入日期时间", + `k3` int COMMENT "数据灌入日期时间") + DUPLICATE KEY(`k1`) DISTRIBUTED BY HASH(`k1`) + PROPERTIES ( "replication_num" = "1" ); """ + + sql """ insert into `test_mod` values(1,2,3); """ + sql """ insert into `test_mod` values(-2147483648,4,-1); """ + sql """ insert into `test_mod` values(5,-9223372036854775808,-1); """ + + qt_sql """ + SELECT * from test_mod order by 1; + """ + + test { + sql "select mod(-2147483648,-1); " + exception "Division of minimal signed number by minus one is an undefined" + } + test { + sql "select mod(-9223372036854775808,-1); " + exception "Division of minimal signed number by minus one is an undefined" + } + test { + sql "select pmod(-9223372036854775808,-1); " + exception "Division of minimal signed number by minus one is an undefined" + } +} From 08a37844208a2f24a631d9f44743c6f959969e45 Mon Sep 17 00:00:00 2001 From: zhangstar333 <2561612514@qq.com> Date: Fri, 19 Jul 2024 10:22:00 +0800 Subject: [PATCH 2/2] update check --- be/src/vec/functions/modulo.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/be/src/vec/functions/modulo.cpp b/be/src/vec/functions/modulo.cpp index 12867f523303ba..7a2dfc004efc7d 100644 --- a/be/src/vec/functions/modulo.cpp +++ b/be/src/vec/functions/modulo.cpp @@ -37,12 +37,13 @@ template inline void throw_if_division_leads_to_FPE(A a, B b) { // http://avva.livejournal.com/2548306.html // (-9223372036854775808 % -1) will cause coredump directly, so check this case to throw exception, or maybe could return 0 as result - if (UNLIKELY(std::is_signed_v && std::is_signed_v && a == std::numeric_limits::min() && - b == -1)) { - throw Exception(ErrorCode::INVALID_ARGUMENT, - "Division of minimal signed number by minus one is an undefined " - "behavior, {} % {}. ", - a, b); + if constexpr (std::is_signed_v && std::is_signed_v) { + if (b == -1 && a == std::numeric_limits::min()) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "Division of minimal signed number by minus one is an undefined " + "behavior, {} % {}. ", + a, b); + } } }