Merge topic 'fix-cps-crash-on-bad-input' into release-4.2

fa4bed7844 cmPackageInfoReader: Don't crash if input is malformed
be99a82eee cmPackageInfoReader: Just use std::string

Acked-by: Kitware Robot <[email protected]>
Merge-request: !11390

Source/cmFindPackageCommand.cxx

@@ -2055,8 +2055,11 @@ cmFindPackageCommand::AppendixMap cmFindPackageCommand::FindAppendices(
         continue;
       }
 
+      cmMakefile::CallRAII cs{ this->Makefile, extra, this->Status };
+
       std::unique_ptr<cmPackageInfoReader> reader =
-        cmPackageInfoReader::Read(extra, &baseReader);
+        cmPackageInfoReader::Read(this->Makefile, extra, &baseReader);
+
       if (reader && reader->GetName() == this->Name) {
         std::vector<std::string> components = reader->GetComponentNames();
         Appendix appendix{ std::move(reader), std::move(components) };
@@ -2249,8 +2252,10 @@ bool cmFindPackageCommand::ImportPackageTargets(cmPackageState& packageState,
 
     // Try to read supplemental data from each file found.
     for (std::string const& extra : glob.GetFiles()) {
+      cmMakefile::CallRAII cs{ this->Makefile, extra, this->Status };
+
       std::unique_ptr<cmPackageInfoReader> configReader =
-        cmPackageInfoReader::Read(extra, &reader);
+        cmPackageInfoReader::Read(this->Makefile, extra, &reader);
       if (configReader && configReader->GetName() == this->Name) {
         if (!configReader->ImportTargetConfigurations(this->Makefile,
                                                       this->Status)) {
@@ -2953,8 +2958,11 @@ bool cmFindPackageCommand::CheckVersion(std::string const& config_file)
   std::string ext = cmSystemTools::LowerCase(config_file.substr(pos));
 
   if (ext == ".cps"_s) {
+    cmMakefile::CallRAII cs{ this->Makefile, config_file, this->Status };
+
     std::unique_ptr<cmPackageInfoReader> reader =
-      cmPackageInfoReader::Read(config_file);
+      cmPackageInfoReader::Read(this->Makefile, config_file);
+
     if (reader && reader->GetName() == this->Name) {
       // Read version information.
       cm::optional<std::string> cpsVersion = reader->GetVersion();

Source/cmPackageInfoReader.cxx

@@ -109,9 +109,17 @@ Json::Value ReadJson(std::string const& fileName)
   return data;
 }
 
+std::string ToString(Json::Value const& value)
+{
+  if (value.isString()) {
+    return value.asString();
+  }
+  return {};
+}
+
 bool CheckSchemaVersion(Json::Value const& data)
 {
-  std::string const& version = data["cps_version"].asString();
+  std::string const& version = ToString(data["cps_version"]);
 
   // Check that a valid version is specified.
   if (version.empty()) {
@@ -137,7 +145,7 @@ std::string DeterminePrefix(std::string const& filepath,
                             Json::Value const& data)
 {
   // First check if an absolute prefix was supplied.
-  std::string prefix = data["prefix"].asString();
+  std::string prefix = ToString(data["prefix"]);
   if (!prefix.empty()) {
     // Ensure that the specified prefix is valid.
     if (cmsys::SystemTools::FileIsFullPath(prefix) &&
@@ -151,7 +159,7 @@ std::string DeterminePrefix(std::string const& filepath,
 
   // Get and validate prefix-relative path.
   std::string const& absPath = cmSystemTools::GetFilenamePath(filepath);
-  std::string relPath = data["cps_path"].asString();
+  std::string relPath = ToString(data["cps_path"]);
   cmSystemTools::ConvertToUnixSlashes(relPath);
   if (relPath.empty() || !cmHasLiteralPrefix(relPath, "@prefix@")) {
     // The relative prefix is not valid.
@@ -436,7 +444,8 @@ cm::optional<cmPackageInfoReader::Pep440Version> ParseSimpleVersion(
 } // namespace
 
 std::unique_ptr<cmPackageInfoReader> cmPackageInfoReader::Read(
-  std::string const& path, cmPackageInfoReader const* parent)
+  cmMakefile* makefile, std::string const& path,
+  cmPackageInfoReader const* parent)
 {
   // Read file and perform some basic validation:
   //   - the input is valid JSON
@@ -485,7 +494,13 @@ std::unique_ptr<cmPackageInfoReader> cmPackageInfoReader::Read(
   // Check for a default license.
   Json::Value const& defaultLicense = reader->Data["default_license"];
   if (!defaultLicense.isNull()) {
-    reader->DefaultLicense = defaultLicense.asString();
+    if (defaultLicense.isString()) {
+      reader->DefaultLicense = defaultLicense.asString();
+    } else {
+      makefile->IssueMessage(
+        MessageType::WARNING,
+        "Package attribute \"default_license\" is not a string.");
+    }
   } else if (parent) {
     reader->DefaultLicense = parent->DefaultLicense;
   } else {
@@ -495,7 +510,13 @@ std::unique_ptr<cmPackageInfoReader> cmPackageInfoReader::Read(
     // consistent with not allowing LICENSE and APPENDIX to be used together.
     Json::Value const& packageLicense = reader->Data["license"];
     if (!packageLicense.isNull()) {
-      reader->DefaultLicense = packageLicense.asString();
+      if (packageLicense.isString()) {
+        reader->DefaultLicense = packageLicense.asString();
+      } else {
+        makefile->IssueMessage(
+          MessageType::WARNING,
+          "Package attribute \"license\" is not a string.");
+      }
     }
   }
 
@@ -504,7 +525,7 @@ std::unique_ptr<cmPackageInfoReader> cmPackageInfoReader::Read(
 
 std::string cmPackageInfoReader::GetName() const
 {
-  return this->Data["name"].asString();
+  return ToString(this->Data["name"]);
 }
 
 cm::optional<std::string> cmPackageInfoReader::GetVersion() const
@@ -536,7 +557,7 @@ cmPackageInfoReader::ParseVersion(
 
   // Check if we know how to parse the version.
   Json::Value const& schema = this->Data["version_schema"];
-  if (schema.isNull() || cmStrCaseEq(schema.asString(), "simple"_s)) {
+  if (schema.isNull() || cmStrCaseEq(ToString(schema), "simple"_s)) {
     return ParseSimpleVersion(*version);
   }
 
@@ -551,7 +572,7 @@ std::vector<cmPackageRequirement> cmPackageInfoReader::GetRequirements() const
 
   for (auto ri = requirementObjects.begin(), re = requirementObjects.end();
        ri != re; ++ri) {
-    cmPackageRequirement r{ ri.name(), (*ri)["version"].asString(),
+    cmPackageRequirement r{ ri.name(), ToString((*ri)["version"]),
                             ReadList(*ri, "components"),
                             ReadList(*ri, "hints") };
     requirements.emplace_back(std::move(r));
@@ -627,11 +648,14 @@ void cmPackageInfoReader::AddTargetConfiguration(
   target->SetProperty("IMPORTED_CONFIGURATIONS", cmJoin(newConfigs, ";"_s));
 }
 
-void cmPackageInfoReader::SetImportProperty(cmTarget* target,
+void cmPackageInfoReader::SetImportProperty(cmMakefile* makefile,
+                                            cmTarget* target,
                                             cm::string_view property,
                                             cm::string_view configuration,
-                                            Json::Value const& value) const
+                                            Json::Value const& object,
+                                            std::string const& attribute) const
 {
+  Json::Value const& value = object[attribute];
   if (!value.isNull()) {
     std::string fullprop;
     if (configuration.empty()) {
@@ -641,18 +665,36 @@ void cmPackageInfoReader::SetImportProperty(cmTarget* target,
                           cmSystemTools::UpperCase(configuration));
     }
 
-    target->SetProperty(fullprop, this->ResolvePath(value.asString()));
+    if (value.isString()) {
+      target->SetProperty(fullprop, this->ResolvePath(value.asString()));
+    } else {
+      makefile->IssueMessage(MessageType::WARNING,
+                             cmStrCat("Failed to set property \""_s, property,
+                                      "\" on target \""_s, target->GetName(),
+                                      "\": attribute \"", attribute,
+                                      "\" is not a string."_s));
+    }
   }
 }
 
 void cmPackageInfoReader::SetMetaProperty(
-  cmTarget* target, cm::string_view property, Json::Value const& value,
+  cmMakefile* makefile, cmTarget* target, std::string const& property,
+  Json::Value const& object, std::string const& attribute,
   std::string const& defaultValue) const
 {
+  Json::Value const& value = object[attribute];
   if (!value.isNull()) {
-    target->SetProperty(property.data(), value.asString());
+    if (value.isString()) {
+      target->SetProperty(property, value.asString());
+    } else {
+      makefile->IssueMessage(MessageType::WARNING,
+                             cmStrCat("Failed to set property \""_s, property,
+                                      "\" on target \""_s, target->GetName(),
+                                      "\": attribute \"", attribute,
+                                      "\" is not a string."_s));
+    }
   } else if (!defaultValue.empty()) {
-    target->SetProperty(property.data(), defaultValue);
+    target->SetProperty(property, defaultValue);
   }
 }
 
@@ -702,14 +744,14 @@ void cmPackageInfoReader::SetTargetProperties(
                            });
 
   // Add link name/location(s).
-  this->SetImportProperty(target, "LOCATION"_s, configuration,
-                          data["location"]);
+  this->SetImportProperty(makefile, target, "LOCATION"_s, // br
+                          configuration, data, "location");
 
-  this->SetImportProperty(target, "IMPLIB"_s, configuration,
-                          data["link_location"]);
+  this->SetImportProperty(makefile, target, "IMPLIB"_s, // br
+                          configuration, data, "link_location");
 
-  this->SetImportProperty(target, "SONAME"_s, configuration,
-                          data["link_name"]);
+  this->SetImportProperty(makefile, target, "SONAME"_s, // br
+                          configuration, data, "link_name");
 
   // Add link languages.
   for (std::string const& originalLang : ReadList(data, "link_languages")) {
@@ -734,7 +776,7 @@ void cmPackageInfoReader::SetTargetProperties(
 
   // Add other information.
   if (configuration.empty()) {
-    this->SetMetaProperty(target, "SPDX_LICENSE"_s, data["license"],
+    this->SetMetaProperty(makefile, target, "SPDX_LICENSE", data, "license",
                           this->DefaultLicense);
   }
 }
@@ -768,7 +810,7 @@ bool cmPackageInfoReader::ImportTargets(cmMakefile* makefile,
   for (auto ci = components.begin(), ce = components.end(); ci != ce; ++ci) {
     cm::string_view const name = IterKey(ci);
     std::string const& type =
-      cmSystemTools::LowerCase((*ci)["type"].asString());
+      cmSystemTools::LowerCase(ToString((*ci)["type"]));
 
     // Get and validate full target name.
     std::string const& fullName = cmStrCat(package, "::"_s, name);
@@ -833,7 +875,7 @@ bool cmPackageInfoReader::ImportTargets(cmMakefile* makefile,
 bool cmPackageInfoReader::ImportTargetConfigurations(
   cmMakefile* makefile, cmExecutionStatus& status) const
 {
-  std::string const& configuration = this->Data["configuration"].asString();
+  std::string const& configuration = ToString(this->Data["configuration"]);
 
   if (configuration.empty()) {
     makefile->IssueMessage(MessageType::WARNING,

Source/cmPackageInfoReader.h

@@ -39,7 +39,8 @@ class cmPackageInfoReader
 {
 public:
   static std::unique_ptr<cmPackageInfoReader> Read(
-    std::string const& path, cmPackageInfoReader const* parent = nullptr);
+    cmMakefile* makefile, std::string const& path,
+    cmPackageInfoReader const* parent = nullptr);
 
   std::string GetName() const;
   cm::optional<std::string> GetVersion() const;
@@ -97,11 +98,14 @@ private:
   void SetTargetProperties(cmMakefile* makefile, cmTarget* target,
                            Json::Value const& data, std::string const& package,
                            cm::string_view configuration) const;
-  void SetImportProperty(cmTarget* target, cm::string_view property,
+  void SetImportProperty(cmMakefile* makefile, cmTarget* target,
+                         cm::string_view property,
                          cm::string_view configuration,
-                         Json::Value const& value) const;
-  void SetMetaProperty(cmTarget* target, cm::string_view property,
-                       Json::Value const& value,
+                         Json::Value const& object,
+                         std::string const& attribute) const;
+  void SetMetaProperty(cmMakefile* makefile, cmTarget* target,
+                       std::string const& property, Json::Value const& object,
+                       std::string const& attribute,
                        std::string const& defaultValue = {}) const;
 
   std::string ResolvePath(std::string path) const;

Tests/RunCMake/find_package-CPS/InvalidCps3-result.txt

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

Tests/RunCMake/find_package-CPS/InvalidCps3-stderr.txt

@@ -0,0 +1,13 @@
+CMake Warning in cps/[Cc]orrupt\.cps:
+  Package attribute "license" is not a string\.
+Call Stack \(most recent call first\):
+  InvalidCps3\.cmake:10 \(find_package\)
+  CMakeLists\.txt:3 \(include\)
+
+
+CMake Warning in cps/[Cc]orrupt\.cps:
+  Failed to set property "IMPLIB" on target "Corrupt::corrupt": attribute
+  "link_location" is not a string\.
+Call Stack \(most recent call first\):
+  InvalidCps3\.cmake:10 \(find_package\)
+  CMakeLists\.txt:3 \(include\)

Tests/RunCMake/find_package-CPS/InvalidCps3.cmake

@@ -0,0 +1,10 @@
+cmake_minimum_required(VERSION 4.0)
+
+include(Setup.cmake)
+
+set(CMAKE_FIND_PACKAGE_SORT_ORDER NAME)
+set(CMAKE_FIND_PACKAGE_SORT_DIRECTION DEC)
+
+###############################################################################
+# Test reporting when trying to read a .cps that is not schema conforming.
+find_package(Corrupt REQUIRED)

Tests/RunCMake/find_package-CPS/RunCMakeTest.cmake

@@ -32,6 +32,7 @@ endfunction()
 # General failure tests
 run_cmake(InvalidCps1)
 run_cmake(InvalidCps2)
+run_cmake(InvalidCps3)
 run_cmake(WrongName)
 run_cmake(BadPrefix)
 

Tests/RunCMake/find_package-CPS/cps/corrupt.cps

@@ -0,0 +1,12 @@
+{
+  "cps_version": "0.13.0",
+  "name": "Corrupt",
+  "license": [],
+  "cps_path": "@prefix@/cps",
+  "components": {
+    "corrupt": {
+      "type": "archive",
+      "link_location": []
+    }
+  }
+}