From e24d1164d463d1fabc130a66ac0c076ac8db63f2 Mon Sep 17 00:00:00 2001 From: Kunal Tyagi Date: Thu, 6 Feb 2020 18:53:05 +0900 Subject: [PATCH 1/2] Adding tests for test to number parsers --- test/common/CMakeLists.txt | 1 + test/common/test_parse.cpp | 191 +++++++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+) create mode 100644 test/common/test_parse.cpp diff --git a/test/common/CMakeLists.txt b/test/common/CMakeLists.txt index 39dced36c7c..904016c2e88 100644 --- a/test/common/CMakeLists.txt +++ b/test/common/CMakeLists.txt @@ -17,6 +17,7 @@ PCL_ADD_TEST(common_test_wrappers test_wrappers FILES test_wrappers.cpp LINK_WIT PCL_ADD_TEST(common_test_macros test_macros FILES test_macros.cpp LINK_WITH pcl_gtest pcl_common) PCL_ADD_TEST(common_vector_average test_vector_average FILES test_vector_average.cpp LINK_WITH pcl_gtest) PCL_ADD_TEST(common_common test_common FILES test_common.cpp LINK_WITH pcl_gtest pcl_common) +PCL_ADD_TEST(common_parse test_parse FILES test_parse.cpp LINK_WITH pcl_gtest pcl_common) PCL_ADD_TEST(common_geometry test_geometry FILES test_geometry.cpp LINK_WITH pcl_gtest pcl_common) PCL_ADD_TEST(common_copy_point test_copy_point FILES test_copy_point.cpp LINK_WITH pcl_gtest pcl_common) PCL_ADD_TEST(common_transforms test_transforms FILES test_transforms.cpp LINK_WITH pcl_gtest pcl_common) diff --git a/test/common/test_parse.cpp b/test/common/test_parse.cpp new file mode 100644 index 00000000000..d8ad817d72a --- /dev/null +++ b/test/common/test_parse.cpp @@ -0,0 +1,191 @@ +/* + * Software License Agreement (BSD License) + * + * Point Cloud Library (PCL) - www.pointclouds.org + * Copyright (c) 2020-, Open Perception, Inc. + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * * Neither the name of the copyright holder(s) nor the names of its + * contributors may be used to endorse or promote products derived + * from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN + * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + * + */ + +#include +#include +#include + +using namespace std; + +/////////////////////////////////////////////////////////////////////////////////////////// +TEST (PCL, parse_double) +{ + const char arg0[] = {"test_name"}; + const char arg1[] = {"double"}; + const char arg2_1[] = {"-3.14e+0"}; + const char arg2_2[] = {"-3.14f+0"}; + const char arg2_3[] = {"3.14e+309"}; + + const char* argv_1[] = { &arg0[0], &arg1[0], &arg2_1[0], nullptr}; + const char* argv_2[] = { &arg0[0], &arg1[0], &arg2_2[0], nullptr}; + const char* argv_3[] = { &arg0[0], &arg1[0], &arg2_3[0], nullptr}; + const int argc = static_cast (sizeof (argv_1)/sizeof (argv_1[0])) - 1; + + int index = -1; + double value = 0; + + index = pcl::console::parse_argument (argc, argv_1, "double", value); + EXPECT_DOUBLE_EQ(-3.14, value); + EXPECT_EQ(1, index); + + index = pcl::console::parse_argument (argc, argv_2, "double", value); + EXPECT_EQ(-1, index); + + index = pcl::console::parse_argument (argc, argv_3, "double", value); + EXPECT_EQ(-1, index); +} + +/////////////////////////////////////////////////////////////////////////////////////////// +TEST (PCL, parse_float) +{ + const char arg0[] = {"test_name"}; + const char arg1[] = {"float"}; + const char arg2_1[] = {"-3.14e+0"}; + const char arg2_2[] = {"-3.14f+0"}; + const char arg2_3[] = {"3.14e+39"}; + + const char* argv_1[] = { &arg0[0], &arg1[0], &arg2_1[0], nullptr}; + const char* argv_2[] = { &arg0[0], &arg1[0], &arg2_2[0], nullptr}; + const char* argv_3[] = { &arg0[0], &arg1[0], &arg2_3[0], nullptr}; + const int argc = static_cast (sizeof (argv_1)/sizeof (argv_1[0])) - 1; + + int index = -1; + float value = 0; + + index = pcl::console::parse_argument (argc, argv_1, "float", value); + EXPECT_FLOAT_EQ(-3.14, value); + EXPECT_EQ(1, index); + + index = pcl::console::parse_argument (argc, argv_2, "float", value); + EXPECT_EQ(-1, index); + + index = pcl::console::parse_argument (argc, argv_3, "float", value); + EXPECT_EQ(-1, index); +} + +/////////////////////////////////////////////////////////////////////////////////////////// +TEST (PCL, parse_longint) +{ + const char arg0[] = {"test_name"}; + const char arg1[] = {"long_int"}; + const char arg2_1[] = {"-314"}; + const char arg2_2[] = {"3.14"}; + const char arg2_3[] = {"18446744073709551615"}; + + const char* argv_1[] = { &arg0[0], &arg1[0], &arg2_1[0], nullptr}; + const char* argv_2[] = { &arg0[0], &arg1[0], &arg2_2[0], nullptr}; + const char* argv_3[] = { &arg0[0], &arg1[0], &arg2_3[0], nullptr}; + const int argc = static_cast (sizeof (argv_1)/sizeof (argv_1[0])) - 1; + + int index = -1; + long int value = 0; + + index = pcl::console::parse_argument (argc, argv_1, "long_int", value); + EXPECT_EQ(-314, value); + EXPECT_EQ(1, index); + + index = pcl::console::parse_argument (argc, argv_2, "long_int", value); + EXPECT_EQ(-1, index); + + index = pcl::console::parse_argument (argc, argv_3, "long_int", value); + EXPECT_EQ(-1, index); +} + +/////////////////////////////////////////////////////////////////////////////////////////// +TEST (PCL, parse_unsignedint) +{ + const char arg0[] = {"test_name"}; + const char arg1[] = {"unsigned_int"}; + const char arg2_1[] = {"314"}; + const char arg2_2[] = {"-314"}; + const char arg2_3[] = {"18446744073709551615"}; + + const char* argv_1[] = { &arg0[0], &arg1[0], &arg2_1[0], nullptr}; + const char* argv_2[] = { &arg0[0], &arg1[0], &arg2_2[0], nullptr}; + const char* argv_3[] = { &arg0[0], &arg1[0], &arg2_3[0], nullptr}; + const int argc = static_cast (sizeof (argv_1)/sizeof (argv_1[0])) - 1; + + int index = -1; + unsigned int value = 53; + + index = pcl::console::parse_argument (argc, argv_1, "unsigned_int", value); + EXPECT_EQ(314, value); + EXPECT_EQ(1, index); + + index = pcl::console::parse_argument (argc, argv_2, "unsigned_int", value); + EXPECT_EQ(-1, index); + + index = pcl::console::parse_argument (argc, argv_3, "unsigned_int", value); + EXPECT_EQ(-1, index); +} + +/////////////////////////////////////////////////////////////////////////////////////////// +TEST (PCL, parse_int) +{ + const char arg0[] = {"test_name"}; + const char arg1[] = {"int"}; + const char arg2_1[] = {"-314"}; + const char arg2_2[] = {"3.14"}; + const char arg2_3[] = {"18446744073709551615"}; + + const char* argv_1[] = { &arg0[0], &arg1[0], &arg2_1[0], nullptr}; + const char* argv_2[] = { &arg0[0], &arg1[0], &arg2_2[0], nullptr}; + const char* argv_3[] = { &arg0[0], &arg1[0], &arg2_3[0], nullptr}; + const int argc = static_cast (sizeof (argv_1)/sizeof (argv_1[0])) - 1; + + int index = -1; + int value = 0; + + index = pcl::console::parse_argument (argc, argv_1, "int", value); + EXPECT_EQ(-314, value); + EXPECT_EQ(1, index); + + index = pcl::console::parse_argument (argc, argv_2, "int", value); + EXPECT_EQ(-1, index); + + index = pcl::console::parse_argument (argc, argv_3, "int", value); + EXPECT_EQ(-1, index); +} + +/* ---[ */ +int +main (int argc, char** argv) +{ + testing::InitGoogleTest (&argc, argv); + return (RUN_ALL_TESTS ()); +} +/* ]--- */ From 30dcffa184e0d9eea382f9d9270bfb77b02babb2 Mon Sep 17 00:00:00 2001 From: Kunal Tyagi Date: Thu, 6 Feb 2020 18:53:59 +0900 Subject: [PATCH 2/2] Refactoring parsers to remove UB --- common/include/pcl/console/parse.h | 10 ++ common/src/parse.cpp | 155 ++++++++++++++++++++++------- 2 files changed, 130 insertions(+), 35 deletions(-) diff --git a/common/include/pcl/console/parse.h b/common/include/pcl/console/parse.h index 79e14f456c1..04d0e286e55 100644 --- a/common/include/pcl/console/parse.h +++ b/common/include/pcl/console/parse.h @@ -150,6 +150,16 @@ namespace pcl PCL_EXPORTS int parse_argument (int argc, const char * const * argv, const char * str, unsigned int &val); + /** \brief Parse for a specific given command line argument. + * \param[in] argc the number of command line arguments + * \param[in] argv the command line arguments + * \param[in] str the string value to search for + * \param[out] val the resultant value + * \return index of found argument or -1 if arguments do not appear in list + */ + PCL_EXPORTS int + parse_argument (int argc, const char * const * argv, const char * str, long int &val) noexcept; + /** \brief Parse for a specific given command line argument. * \param[in] argc the number of command line arguments * \param[in] argv the command line arguments diff --git a/common/src/parse.cpp b/common/src/parse.cpp index affa06e4a05..26917636324 100644 --- a/common/src/parse.cpp +++ b/common/src/parse.cpp @@ -2,8 +2,7 @@ * Software License Agreement (BSD License) * * Point Cloud Library (PCL) - www.pointclouds.org - * Copyright (c) 2010-2012, Willow Garage, Inc. - * Copyright (c) 2012-, Open Perception, Inc. + * Copyright (c) 2020-, Open Perception, Inc. * * All rights reserved. * @@ -37,17 +36,16 @@ */ #include +#include +#include #include +#include +#include + #include #include -#include -//////////////////////////////////////////////////////////////////////////////// -bool -pcl::console::find_switch (int argc, const char * const * argv, const char * argument_name) -{ - return (find_argument (argc, argv, argument_name) != -1); -} +#include //////////////////////////////////////////////////////////////////////////////// int @@ -64,6 +62,13 @@ pcl::console::find_argument (int argc, const char * const * argv, const char * a return (-1); } +//////////////////////////////////////////////////////////////////////////////// +bool +pcl::console::find_switch (int argc, const char * const * argv, const char * argument_name) +{ + return (find_argument (argc, argv, argument_name) != -1); +} + //////////////////////////////////////////////////////////////////////////////// int pcl::console::parse_argument (int argc, const char * const * argv, const char * str, std::string &val) @@ -76,63 +81,143 @@ pcl::console::parse_argument (int argc, const char * const * argv, const char * } //////////////////////////////////////////////////////////////////////////////// -int -pcl::console::parse_argument (int argc, const char * const * argv, const char * str, bool &val) +namespace pcl +{ +namespace console { +template int +parse_generic (V convert_func, int argc, const char* const* argv, const char* str, T& val) +{ + char *endptr = nullptr; int index = find_argument (argc, argv, str) + 1; + errno = 0; if (index > 0 && index < argc ) - val = atoi (argv[index]) == 1; + { + val = convert_func (argv[index], &endptr); // similar to strtol, strtod, strtof + // handle out-of-range, junk at the end and no conversion + if (errno == ERANGE || *endptr != '\0' || str == endptr) + { + return -1; + } + } return (index - 1); } -//////////////////////////////////////////////////////////////////////////////// int -pcl::console::parse_argument (int argc, const char * const * argv, const char * str, double &val) +parse_argument (int argc, const char * const * argv, const char * str, long int &val) noexcept { - int index = find_argument (argc, argv, str) + 1; - - if (index > 0 && index < argc ) - val = atof (argv[index]); + const auto strtol_10 = [](const char *str, char **str_end){ return strtol(str, str_end, 10); }; + return parse_generic(strtol_10, argc, argv, str, val); +} - return (index - 1); +int +parse_argument (int argc, const char * const * argv, const char * str, long long int &val) noexcept +{ + const auto strtoll_10 = [](const char *str, char **str_end){ return strtoll(str, str_end, 10); }; + return parse_generic(strtoll_10, argc, argv, str, val); } -//////////////////////////////////////////////////////////////////////////////// int -pcl::console::parse_argument (int argc, const char * const * argv, const char * str, float &val) +parse_argument (int argc, const char * const * argv, const char * str, unsigned long long int &val) noexcept { - int index = find_argument (argc, argv, str) + 1; + long long int dummy; + const auto ret = parse_argument (argc, argv, str, dummy); + if ((ret == -1) || dummy < 0) + { + return -1; + } + val = dummy; + return ret; +} - if (index > 0 && index < argc ) - val = static_cast (atof (argv[index])); +namespace detail +{ +template +constexpr auto legally_representable_v = (std::numeric_limits::max () >= std::numeric_limits::max ()) && + (std::numeric_limits::lowest () <= std::numeric_limits::lowest ()); +template +struct legally_representable { + constexpr static bool value = legally_representable_v; +}; + +// assumptions: +// * either long int or long long int is a valid type for storing Integral +// * unsigned long long int is handled specially +template +using primary_legal_input_type = std::conditional_t, + long int, long long int>; + +// special handling if unsigned [long] int is of same size as long long int +template +using legal_input_type = std::conditional_t<(std::is_unsigned::value && + (sizeof (Integral) == sizeof (long long int))), + unsigned long long int, + primary_legal_input_type>; +} - return (index - 1); +template +using IsIntegral = std::enable_if_t::value, bool>; + +template = true> int +parse_argument (int argc, const char * const * argv, const char * str, T &val) noexcept +{ + using InputType = detail::legal_input_type; + InputType dummy; + const auto ret = parse_argument (argc, argv, str, dummy); + if ((ret == -1) || + (dummy < static_cast (std::numeric_limits::min ())) || + (dummy > static_cast (std::numeric_limits::max ()))) + { + return -1; + } + + val = static_cast (dummy); + return ret; +} +} } //////////////////////////////////////////////////////////////////////////////// int -pcl::console::parse_argument (int argc, const char * const * argv, const char * str, int &val) +pcl::console::parse_argument (int argc, const char * const * argv, const char * str, double &val) { - int index = find_argument (argc, argv, str) + 1; - - if (index > 0 && index < argc ) - val = atoi (argv[index]); + return parse_generic(strtod, argc, argv, str, val); +} - return (index - 1); +//////////////////////////////////////////////////////////////////////////////// +int +pcl::console::parse_argument (int argc, const char * const * argv, const char * str, float &val) +{ + return parse_generic(strtof, argc, argv, str, val); } //////////////////////////////////////////////////////////////////////////////// int pcl::console::parse_argument (int argc, const char * const * argv, const char * str, unsigned int &val) { - int index = find_argument (argc, argv, str) + 1; + return parse_argument (argc, argv, str, val); +} - if (index > 0 && index < argc ) - val = atoi (argv[index]); +//////////////////////////////////////////////////////////////////////////////// +int +pcl::console::parse_argument (int argc, const char * const * argv, const char * str, int &val) +{ + return parse_argument (argc, argv, str, val); +} - return (index - 1); +//////////////////////////////////////////////////////////////////////////////// +int +pcl::console::parse_argument (int argc, const char * const * argv, const char * str, bool &val) +{ + long int dummy; + const auto ret = parse_argument (argc, argv, str, dummy); + if (ret != -1) + { + val = static_cast (dummy); + } + return ret; } ////////////////////////////////////////////////////////////////////////////////