Merge pull request #3613 from kunaltyagi/dup1

Removing undefined behavior and stricter checks in console arg parsing

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

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 <cctype>
+#include <cerrno>
+#include <complex>
 #include <cstdio>
+#include <limits>
+#include <type_traits>
+
 #include <pcl/console/parse.h>
 #include <pcl/console/print.h>
-#include <boost/algorithm/string.hpp>
 
-////////////////////////////////////////////////////////////////////////////////
-bool
-pcl::console::find_switch (int argc, const char * const * argv, const char * argument_name)
-{
-  return (find_argument (argc, argv, argument_name) != -1);
-}
+#include <boost/algorithm/string.hpp>
 
 ////////////////////////////////////////////////////////////////////////////////
 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 <class T, class V = T(*)(const char*, const char**)> 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<float> (atof (argv[index]));
+namespace detail
+{
+template <typename T, typename U>
+constexpr auto legally_representable_v = (std::numeric_limits<T>::max () >= std::numeric_limits<U>::max ()) &&
+                                       (std::numeric_limits<T>::lowest () <= std::numeric_limits<U>::lowest ());
+template <typename T, typename U>
+struct legally_representable {
+    constexpr static bool value = legally_representable_v<T, U>;
+};
+
+// assumptions:
+// * either long int or long long int is a valid type for storing Integral
+// * unsigned long long int is handled specially
+template <typename Integral>
+using primary_legal_input_type = std::conditional_t<legally_representable_v<long int, Integral>,
+                                                    long int, long long int>;
+
+// special handling if unsigned [long] int is of same size as long long int
+template <typename Integral>
+using legal_input_type = std::conditional_t<(std::is_unsigned<Integral>::value &&
+                                             (sizeof (Integral) == sizeof (long long int))),
+                                            unsigned long long int,
+                                            primary_legal_input_type<Integral>>;
+}
 
-  return (index - 1);
+template <typename T>
+using IsIntegral = std::enable_if_t<std::is_integral<T>::value, bool>;
+
+template <typename T, IsIntegral<T> = true> int
+parse_argument (int argc, const char * const * argv, const char * str, T &val) noexcept
+{
+  using InputType = detail::legal_input_type<T>;
+  InputType dummy;
+  const auto ret = parse_argument (argc, argv, str, dummy);
+  if ((ret == -1) ||
+      (dummy < static_cast<InputType> (std::numeric_limits<T>::min ())) ||
+      (dummy > static_cast<InputType> (std::numeric_limits<T>::max ())))
+  {
+    return -1;
+  }
+
+  val = static_cast<T> (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<unsigned int> (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<int> (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<bool> (dummy);
+  }
+  return ret;
 }
 
 ////////////////////////////////////////////////////////////////////////////////

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)

test/common/test_parse.cpp

+/*
+ * 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 <gtest/gtest.h>
+#include <pcl/pcl_tests.h>
+#include <pcl/console/parse.h>
+
+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<int> (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<int> (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<int> (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<int> (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<int> (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 ());
+}
+/* ]--- */