Merge topic 'list-index-arg-parsing'

79eaa908dc cmListCommand: add a policy for failing on invalid indicies
1f1fdff7fa cmListCommand: prefer strtol to atoi
9934a97642 cmListCommand: refactor out index argument parsing

Acked-by: Kitware Robot <kwrobot@kitware.com>
Acked-by: Michael Hirsch, Ph.D. <michael@scivision.dev>
Merge-request: !5817

Help/manual/cmake-policies.7.rst

@@ -51,6 +51,14 @@ The :variable:`CMAKE_MINIMUM_REQUIRED_VERSION` variable may also be used
 to determine whether to report an error on use of deprecated macros or
 functions.
 
+Policies Introduced by CMake 3.21
+=================================
+
+.. toctree::
+   :maxdepth: 1
+
+   CMP0121: The list command detects invalid indicies </policy/CMP0121>
+
 Policies Introduced by CMake 3.20
 =================================
 

Help/policy/CMP0121.rst

@@ -0,0 +1,21 @@
+CMP0121
+-------
+
+.. versionadded:: 3.21
+
+The :command:`list` command now detects invalid indicies.
+
+Prior to CMake version 3.21, the :command:`list` command's ``GET``,
+``INSERT``, ``SUBLIST``, and ``REMOVE_AT`` subcommands did not detect invalid
+index arguments.
+
+The ``OLD`` behavior of this policy is for invalid indicies to be treated as
+their integer value (if any) at the start of the string. For example,
+``2good4you`` is a ``2`` and ``not_an_integer`` is a ``0``. The ``NEW``
+behavior is for invalid indicies to trigger an error.
+
+This policy was introduced in CMake version 3.21.  CMake version |release|
+warns when the policy is not set and uses ``OLD`` behavior. Use the
+:command:`cmake_policy` command to set it to ``OLD`` or ``NEW`` explicitly.
+
+.. include:: DEPRECATED.txt

Help/release/dev/list-index-arg-parsing.rst

@@ -0,0 +1,7 @@
+list-index-arg-parsing
+----------------------
+
+* The :command:`list` command's ``GET``, ``INSERT``, ``SUBLIST``, and
+  ``REMOVE_AT`` subcommands now error with invalid (i.e., non-integer) values
+  are given as any of their index arguments based on the setting of policy
+  :policy:`CMP0121`.

Source/cmListCommand.cxx

@@ -4,9 +4,7 @@
 
 #include <algorithm>
 #include <cassert>
-#include <cstddef>
 #include <cstdio>
-#include <cstdlib> // required for atoi
 #include <functional>
 #include <iterator>
 #include <set>
@@ -36,6 +34,42 @@
 
 namespace {
 
+bool GetIndexArg(char const* arg, int* idx, cmMakefile& mf)
+{
+  long value;
+  if (!cmStrToLong(arg, &value)) {
+    switch (mf.GetPolicyStatus(cmPolicies::CMP0121)) {
+      case cmPolicies::WARN: {
+        // Default is to warn and use old behavior OLD behavior is to allow
+        // compatibility, so issue a warning and use the previous behavior.
+        std::string warn =
+          cmStrCat(cmPolicies::GetPolicyWarning(cmPolicies::CMP0121),
+                   " Invalid list index \"", arg, "\".");
+        mf.IssueMessage(MessageType::AUTHOR_WARNING, warn);
+        break;
+      }
+      case cmPolicies::OLD:
+        // OLD behavior is to allow compatibility, so just ignore the
+        // situation.
+        break;
+      case cmPolicies::NEW:
+        return false;
+      case cmPolicies::REQUIRED_IF_USED:
+      case cmPolicies::REQUIRED_ALWAYS:
+        std::string msg =
+          cmStrCat(cmPolicies::GetRequiredPolicyError(cmPolicies::CMP0121),
+                   " Invalid list index \"", arg, "\".");
+        mf.IssueMessage(MessageType::FATAL_ERROR, msg);
+        break;
+    }
+  }
+
+  // Truncation is happening here, but it had always been happening here.
+  *idx = static_cast<int>(value);
+
+  return true;
+}
+
 bool FilterRegex(std::vector<std::string> const& args, bool includeMatches,
                  std::string const& listName,
                  std::vector<std::string>& varArgsExpanded,
@@ -154,7 +188,11 @@ bool HandleGetCommand(std::vector<std::string> const& args,
   const char* sep = "";
   size_t nitem = varArgsExpanded.size();
   for (cc = 2; cc < args.size() - 1; cc++) {
-    int item = atoi(args[cc].c_str());
+    int item;
+    if (!GetIndexArg(args[cc].c_str(), &item, status.GetMakefile())) {
+      status.SetError(cmStrCat("index: ", args[cc], " is not a valid index"));
+      return false;
+    }
     value += sep;
     sep = ";";
     if (item < 0) {
@@ -362,7 +400,11 @@ bool HandleInsertCommand(std::vector<std::string> const& args,
   const std::string& listName = args[1];
 
   // expand the variable
-  int item = atoi(args[2].c_str());
+  int item;
+  if (!GetIndexArg(args[2].c_str(), &item, status.GetMakefile())) {
+    status.SetError(cmStrCat("index: ", args[2], " is not a valid index"));
+    return false;
+  }
   std::vector<std::string> varArgsExpanded;
   if ((!GetList(varArgsExpanded, listName, status.GetMakefile()) ||
        varArgsExpanded.empty()) &&
@@ -1282,8 +1324,16 @@ bool HandleSublistCommand(std::vector<std::string> const& args,
     return true;
   }
 
-  const int start = atoi(args[2].c_str());
-  const int length = atoi(args[3].c_str());
+  int start;
+  int length;
+  if (!GetIndexArg(args[2].c_str(), &start, status.GetMakefile())) {
+    status.SetError(cmStrCat("index: ", args[2], " is not a valid index"));
+    return false;
+  }
+  if (!GetIndexArg(args[3].c_str(), &length, status.GetMakefile())) {
+    status.SetError(cmStrCat("index: ", args[3], " is not a valid index"));
+    return false;
+  }
 
   using size_type = decltype(varArgsExpanded)::size_type;
 
@@ -1338,7 +1388,11 @@ bool HandleRemoveAtCommand(std::vector<std::string> const& args,
   std::vector<size_t> removed;
   size_t nitem = varArgsExpanded.size();
   for (cc = 2; cc < args.size(); ++cc) {
-    int item = atoi(args[cc].c_str());
+    int item;
+    if (!GetIndexArg(args[cc].c_str(), &item, status.GetMakefile())) {
+      status.SetError(cmStrCat("index: ", args[cc], " is not a valid index"));
+      return false;
+    }
     if (item < 0) {
       item = static_cast<int>(nitem) + item;
     }

Source/cmPolicies.h

@@ -359,7 +359,10 @@ class cmMakefile;
          3, 20, 0, cmPolicies::WARN)                                          \
   SELECT(POLICY, CMP0120,                                                     \
          "The WriteCompilerDetectionHeader module is removed.", 3, 20, 0,     \
-         cmPolicies::WARN)
+         cmPolicies::WARN)                                                    \
+  SELECT(POLICY, CMP0121,                                                     \
+         "The list() command now validates parsing of index arguments.", 3,   \
+         21, 0, cmPolicies::WARN)
 
 #define CM_SELECT_ID(F, A1, A2, A3, A4, A5, A6) F(A1)
 #define CM_FOR_EACH_POLICY_ID(POLICY)                                         \

Tests/RunCMake/CMP0121/CMP0121-ERANGE-Common.cmake

@@ -0,0 +1,8 @@
+set(listvar a b c d e)
+
+list(GET listvar
+  18446744073709551616 # 2^64
+  2147483648 # 2^31
+  4294967296 # 2^32; errors out-of-range as -2147483643 due to underflow
+  out)
+message("ERANGE: -->${out}<--")

Tests/RunCMake/CMP0121/CMP0121-ERANGE-NEW-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/CMP0121/CMP0121-ERANGE-NEW-stderr.txt

@@ -0,0 +1,8 @@
+CMake Error at CMP0121-ERANGE-Common.cmake:3 \(list\):
+  list index: 18446744073709551616 is not a valid index
+Call Stack \(most recent call first\):
+  CMP0121-ERANGE-NEW.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+
+
+ERANGE: --><--

Tests/RunCMake/CMP0121/CMP0121-ERANGE-NEW.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0121 NEW)
+include(CMP0121-ERANGE-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-ERANGE-OLD-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/CMP0121/CMP0121-ERANGE-OLD-stderr.txt

@@ -0,0 +1,8 @@
+CMake Error at CMP0121-ERANGE-Common.cmake:3 \(list\):
+  list index: (-2147483643|2147483647) out of range \(-5, 4\)
+Call Stack \(most recent call first\):
+  CMP0121-ERANGE-OLD.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+
+
+ERANGE: --><--

Tests/RunCMake/CMP0121/CMP0121-ERANGE-OLD.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0121 OLD)
+include(CMP0121-ERANGE-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-ERANGE-WARN-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/CMP0121/CMP0121-ERANGE-WARN-stderr.txt

@@ -0,0 +1,18 @@
+CMake Warning \(dev\) at CMP0121-ERANGE-Common.cmake:3 \(list\):
+  Policy CMP0121 is not set: The list\(\) command now validates parsing of
+  index arguments.  Run "cmake --help-policy CMP0121" for policy details.
+  Use the cmake_policy command to set the policy and suppress this warning.
+  Invalid list index "18446744073709551616".
+Call Stack \(most recent call first\):
+  CMP0121-ERANGE-WARN.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+This warning is for project developers.  Use -Wno-dev to suppress it.
+
+CMake Error at CMP0121-ERANGE-Common.cmake:3 \(list\):
+  list index: (-2147483643|2147483647) out of range \(-5, 4\)
+Call Stack \(most recent call first\):
+  CMP0121-ERANGE-WARN.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+
+
+ERANGE: --><--

Tests/RunCMake/CMP0121/CMP0121-ERANGE-WARN.cmake

@@ -0,0 +1,2 @@
+
+include(CMP0121-ERANGE-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-GET-Common.cmake

@@ -0,0 +1,4 @@
+set(listvar a b c d e)
+
+list(GET listvar 0 2junk out)
+message("GET: -->${out}<--")

Tests/RunCMake/CMP0121/CMP0121-GET-NEW-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/CMP0121/CMP0121-GET-NEW-stderr.txt

@@ -0,0 +1,8 @@
+CMake Error at CMP0121-GET-Common.cmake:3 \(list\):
+  list index: 2junk is not a valid index
+Call Stack \(most recent call first\):
+  CMP0121-GET-NEW.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+
+
+GET: --><--

Tests/RunCMake/CMP0121/CMP0121-GET-NEW.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0121 NEW)
+include(CMP0121-GET-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-GET-OLD-stderr.txt

@@ -0,0 +1 @@
+GET: -->a;c<--

Tests/RunCMake/CMP0121/CMP0121-GET-OLD.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0121 OLD)
+include(CMP0121-GET-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-GET-WARN-stderr.txt

@@ -0,0 +1,11 @@
+CMake Warning \(dev\) at CMP0121-GET-Common.cmake:3 \(list\):
+  Policy CMP0121 is not set: The list\(\) command now validates parsing of
+  index arguments.  Run "cmake --help-policy CMP0121" for policy details.
+  Use the cmake_policy command to set the policy and suppress this warning.
+  Invalid list index "2junk".
+Call Stack \(most recent call first\):
+  CMP0121-GET-WARN.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+This warning is for project developers.  Use -Wno-dev to suppress it.
+
+GET: -->a;c<--

Tests/RunCMake/CMP0121/CMP0121-GET-WARN.cmake

@@ -0,0 +1,2 @@
+
+include(CMP0121-GET-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-INSERT-Common.cmake

@@ -0,0 +1,4 @@
+set(listvar a b c d e)
+
+list(INSERT listvar junk2 new)
+message("INSERT: -->${listvar}<--")

Tests/RunCMake/CMP0121/CMP0121-INSERT-NEW-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/CMP0121/CMP0121-INSERT-NEW-stderr.txt

@@ -0,0 +1,8 @@
+CMake Error at CMP0121-INSERT-Common.cmake:3 \(list\):
+  list index: junk2 is not a valid index
+Call Stack \(most recent call first\):
+  CMP0121-INSERT-NEW.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+
+
+INSERT: -->a;b;c;d;e<--

Tests/RunCMake/CMP0121/CMP0121-INSERT-NEW.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0121 NEW)
+include(CMP0121-INSERT-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-INSERT-OLD-stderr.txt

@@ -0,0 +1 @@
+INSERT: -->new;a;b;c;d;e<--

Tests/RunCMake/CMP0121/CMP0121-INSERT-OLD.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0121 OLD)
+include(CMP0121-INSERT-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-INSERT-WARN-stderr.txt

@@ -0,0 +1,11 @@
+CMake Warning \(dev\) at CMP0121-INSERT-Common.cmake:3 \(list\):
+  Policy CMP0121 is not set: The list\(\) command now validates parsing of
+  index arguments.  Run "cmake --help-policy CMP0121" for policy details.
+  Use the cmake_policy command to set the policy and suppress this warning.
+  Invalid list index "junk2".
+Call Stack \(most recent call first\):
+  CMP0121-INSERT-WARN.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+This warning is for project developers.  Use -Wno-dev to suppress it.
+
+INSERT: -->new;a;b;c;d;e<--

Tests/RunCMake/CMP0121/CMP0121-INSERT-WARN.cmake

@@ -0,0 +1,2 @@
+
+include(CMP0121-INSERT-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-REMOVE_AT-Common.cmake

@@ -0,0 +1,4 @@
+set(listvar a b c d e)
+
+list(REMOVE_AT listvar 0 invalid)
+message("REMOVE_AT: -->${listvar}<--")

Tests/RunCMake/CMP0121/CMP0121-REMOVE_AT-NEW-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/CMP0121/CMP0121-REMOVE_AT-NEW-stderr.txt

@@ -0,0 +1,8 @@
+CMake Error at CMP0121-REMOVE_AT-Common.cmake:3 \(list\):
+  list index: invalid is not a valid index
+Call Stack \(most recent call first\):
+  CMP0121-REMOVE_AT-NEW.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+
+
+REMOVE_AT: -->a;b;c;d;e<--

Tests/RunCMake/CMP0121/CMP0121-REMOVE_AT-NEW.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0121 NEW)
+include(CMP0121-REMOVE_AT-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-REMOVE_AT-OLD-stderr.txt

@@ -0,0 +1 @@
+REMOVE_AT: -->b;c;d;e<--

Tests/RunCMake/CMP0121/CMP0121-REMOVE_AT-OLD.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0121 OLD)
+include(CMP0121-REMOVE_AT-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-REMOVE_AT-WARN-stderr.txt

@@ -0,0 +1,11 @@
+CMake Warning \(dev\) at CMP0121-REMOVE_AT-Common.cmake:3 \(list\):
+  Policy CMP0121 is not set: The list\(\) command now validates parsing of
+  index arguments.  Run "cmake --help-policy CMP0121" for policy details.
+  Use the cmake_policy command to set the policy and suppress this warning.
+  Invalid list index "invalid".
+Call Stack \(most recent call first\):
+  CMP0121-REMOVE_AT-WARN.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+This warning is for project developers.  Use -Wno-dev to suppress it.
+
+REMOVE_AT: -->b;c;d;e<--

Tests/RunCMake/CMP0121/CMP0121-REMOVE_AT-WARN.cmake

@@ -0,0 +1,2 @@
+
+include(CMP0121-REMOVE_AT-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-length-Common.cmake

@@ -0,0 +1,4 @@
+set(listvar a b c d e)
+
+list(SUBLIST listvar 0 invalid out)
+message("SUBLIST-length: -->${out}<--")

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-length-NEW-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-length-NEW-stderr.txt

@@ -0,0 +1,8 @@
+CMake Error at CMP0121-SUBLIST-length-Common.cmake:3 \(list\):
+  list index: invalid is not a valid index
+Call Stack \(most recent call first\):
+  CMP0121-SUBLIST-length-NEW.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+
+
+SUBLIST-length: --><--

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-length-NEW.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0121 NEW)
+include(CMP0121-SUBLIST-length-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-length-OLD-stderr.txt

@@ -0,0 +1 @@
+SUBLIST-length: --><--

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-length-OLD.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0121 OLD)
+include(CMP0121-SUBLIST-length-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-length-WARN-stderr.txt

@@ -0,0 +1,11 @@
+CMake Warning \(dev\) at CMP0121-SUBLIST-length-Common.cmake:3 \(list\):
+  Policy CMP0121 is not set: The list\(\) command now validates parsing of
+  index arguments.  Run "cmake --help-policy CMP0121" for policy details.
+  Use the cmake_policy command to set the policy and suppress this warning.
+  Invalid list index "invalid".
+Call Stack \(most recent call first\):
+  CMP0121-SUBLIST-length-WARN.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+This warning is for project developers.  Use -Wno-dev to suppress it.
+
+SUBLIST-length: --><--

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-length-WARN.cmake

@@ -0,0 +1,2 @@
+
+include(CMP0121-SUBLIST-length-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-start-Common.cmake

@@ -0,0 +1,4 @@
+set(listvar a b c d e)
+
+list(SUBLIST listvar invalid 2 out)
+message("SUBLIST-start: -->${out}<--")

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-start-NEW-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-start-NEW-stderr.txt

@@ -0,0 +1,8 @@
+CMake Error at CMP0121-SUBLIST-start-Common.cmake:3 \(list\):
+  list index: invalid is not a valid index
+Call Stack \(most recent call first\):
+  CMP0121-SUBLIST-start-NEW.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+
+
+SUBLIST-start: --><--

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-start-NEW.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0121 NEW)
+include(CMP0121-SUBLIST-start-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-start-OLD-stderr.txt

@@ -0,0 +1 @@
+SUBLIST-start: -->a;b<--

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-start-OLD.cmake

@@ -0,0 +1,2 @@
+cmake_policy(SET CMP0121 OLD)
+include(CMP0121-SUBLIST-start-Common.cmake)

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-start-WARN-stderr.txt

@@ -0,0 +1,11 @@
+CMake Warning \(dev\) at CMP0121-SUBLIST-start-Common.cmake:3 \(list\):
+  Policy CMP0121 is not set: The list\(\) command now validates parsing of
+  index arguments.  Run "cmake --help-policy CMP0121" for policy details.
+  Use the cmake_policy command to set the policy and suppress this warning.
+  Invalid list index "invalid".
+Call Stack \(most recent call first\):
+  CMP0121-SUBLIST-start-WARN.cmake:2 \(include\)
+  CMakeLists.txt:3 \(include\)
+This warning is for project developers.  Use -Wno-dev to suppress it.
+
+SUBLIST-start: -->a;b<--

Tests/RunCMake/CMP0121/CMP0121-SUBLIST-start-WARN.cmake

@@ -0,0 +1,2 @@
+
+include(CMP0121-SUBLIST-start-Common.cmake)

Tests/RunCMake/CMP0121/CMakeLists.txt

@@ -0,0 +1,3 @@
+cmake_minimum_required(VERSION 3.20)
+project(${RunCMake_TEST} NONE)
+include(${RunCMake_TEST}.cmake)

Tests/RunCMake/CMP0121/RunCMakeTest.cmake

@@ -0,0 +1,7 @@
+include(RunCMake)
+
+foreach (subcommand IN ITEMS ERANGE GET INSERT REMOVE_AT SUBLIST-length SUBLIST-start)
+  run_cmake(CMP0121-${subcommand}-WARN)
+  run_cmake(CMP0121-${subcommand}-OLD)
+  run_cmake(CMP0121-${subcommand}-NEW)
+endforeach ()

Tests/RunCMake/CMakeLists.txt

@@ -131,6 +131,7 @@ if(CMAKE_GENERATOR MATCHES "Ninja")
 endif()
 add_RunCMake_test(CMP0118)
 add_RunCMake_test(CMP0119 -DCMAKE_C_COMPILER_ID=${CMAKE_C_COMPILER_ID})
+add_RunCMake_test(CMP0121)
 
 # The test for Policy 65 requires the use of the
 # CMAKE_SHARED_LIBRARY_LINK_CXX_FLAGS variable, which both the VS and Xcode