CPackWiX: use safe IDs for generated start menu shortcuts

Source/CPack/WiX/cmCPackWIXGenerator.cxx

@@ -170,6 +170,16 @@ bool cmCPackWIXGenerator::InitializeWiXConfiguration()
       }
     }
 
+  if(GetOption("CPACK_PACKAGE_VENDOR") == 0)
+    {
+      std::string defaultVendor = "Humanity";
+      SetOption("CPACK_PACKAGE_VENDOR", defaultVendor.c_str());
+
+      cmCPackLogger(cmCPackLog::LOG_VERBOSE,
+        "CPACK_PACKAGE_VENDOR implicitly set to " << defaultVendor << " . "
+        << std::endl);
+    }
+
   return true;
 }
 
@@ -368,117 +378,49 @@ bool cmCPackWIXGenerator::CreateWiXSourceFiles()
   featureDefinitions.AddAttribute("Id", "ProductFeature");
   featureDefinitions.AddAttribute("Title", Name);
   featureDefinitions.AddAttribute("Level", "1");
-  featureDefinitions.EndElement();
+  featureDefinitions.EndElement("Feature");
 
   featureDefinitions.BeginElement("FeatureRef");
   featureDefinitions.AddAttribute("Id", "ProductFeature");
 
+  std::vector<std::string> cpackPackageExecutablesList;
   const char *cpackPackageExecutables = GetOption("CPACK_PACKAGE_EXECUTABLES");
-  std::vector<std::string> cpackPkgExecutables;
+  if(cpackPackageExecutables)
-  std::string regKey;
-  if ( cpackPackageExecutables )
     {
       cmSystemTools::ExpandListArgument(cpackPackageExecutables,
-      cpackPkgExecutables);
+        cpackPackageExecutablesList);
-    if ( cpackPkgExecutables.size() % 2 != 0 )
+      if(cpackPackageExecutablesList.size() % 2 != 0 )
         {
         cmCPackLogger(cmCPackLog::LOG_ERROR,
           "CPACK_PACKAGE_EXECUTABLES should contain pairs of <executable> and "
-        "<icon name>." << std::endl);
+          "<text label>." << std::endl);
-      cpackPkgExecutables.clear();
+        return false;
-      }
-
-    const char *cpackVendor = GetOption("CPACK_PACKAGE_VENDOR");
-    const char *cpackPkgName = GetOption("CPACK_PACKAGE_NAME");
-    if (!cpackVendor || !cpackPkgName)
-      {
-      cmCPackLogger(cmCPackLog::LOG_WARNING, "CPACK_PACKAGE_VENDOR and "
-        "CPACK_PACKAGE_NAME must be defined for shortcut creation"
-        << std::endl);
-      cpackPkgExecutables.clear();
-      }
-    else
-      {
-        regKey = std::string("Software/") + cpackVendor + "/" + cpackPkgName;
         }
     }
 
-  std::vector<std::string> dirIdExecutables;
   AddDirectoryAndFileDefinitons(
     toplevel, "INSTALL_ROOT",
     directoryDefinitions, fileDefinitions, featureDefinitions,
-    cpackPkgExecutables, dirIdExecutables);
+    cpackPackageExecutablesList);
 
-  directoryDefinitions.EndElement();
+  if(!CreateStartMenuShortcuts(
-  directoryDefinitions.EndElement();
+    directoryDefinitions, fileDefinitions, featureDefinitions))
-
-  if (dirIdExecutables.size() > 0 && dirIdExecutables.size() % 3 == 0)
     {
-    fileDefinitions.BeginElement("DirectoryRef");
+      return false;
-    fileDefinitions.AddAttribute("Id", "PROGRAM_MENU_FOLDER");
-    fileDefinitions.BeginElement("Component");
-    fileDefinitions.AddAttribute("Id", "SHORTCUT");
-    fileDefinitions.AddAttribute("Guid", "*");
-
-    std::vector<std::string>::iterator it;
-    for ( it = dirIdExecutables.begin() ;
-          it != dirIdExecutables.end();
-          ++it)
-      {
-      std::string fileName = *it++;
-      std::string iconName = *it++;
-      std::string directoryId = *it;
-
-      fileDefinitions.BeginElement("Shortcut");
-
-      // the iconName is more likely to contain blanks early on
-      std::string shortcutName = fileName;
-
-      std::string::size_type const dotPos = shortcutName.find('.');
-      if(std::string::npos == dotPos)
-        { shortcutName = shortcutName.substr(0, dotPos); }
-      fileDefinitions.AddAttribute("Id", "SHORTCUT_" + shortcutName);
-      fileDefinitions.AddAttribute("Name", iconName);
-      std::string target = "[" + directoryId + "]" + fileName;
-      fileDefinitions.AddAttribute("Target", target);
-      fileDefinitions.AddAttribute("WorkingDirectory", directoryId);
-      fileDefinitions.EndElement();
-      }
-    fileDefinitions.BeginElement("Shortcut");
-    fileDefinitions.AddAttribute("Id", "UNINSTALL");
-    std::string pkgName = GetOption("CPACK_PACKAGE_NAME");
-    fileDefinitions.AddAttribute("Name", "Uninstall " + pkgName);
-    fileDefinitions.AddAttribute("Description", "Uninstalls " + pkgName);
-    fileDefinitions.AddAttribute("Target", "[SystemFolder]msiexec.exe");
-    fileDefinitions.AddAttribute("Arguments", "/x [ProductCode]");
-    fileDefinitions.EndElement();
-    fileDefinitions.BeginElement("RemoveFolder");
-    fileDefinitions.AddAttribute("Id", "PROGRAM_MENU_FOLDER");
-    fileDefinitions.AddAttribute("On", "uninstall");
-    fileDefinitions.EndElement();
-    fileDefinitions.BeginElement("RegistryValue");
-    fileDefinitions.AddAttribute("Root", "HKCU");
-    fileDefinitions.AddAttribute("Key", regKey);
-    fileDefinitions.AddAttribute("Name", "installed");
-    fileDefinitions.AddAttribute("Type", "integer");
-    fileDefinitions.AddAttribute("Value", "1");
-    fileDefinitions.AddAttribute("KeyPath", "yes");
-
-    featureDefinitions.BeginElement("ComponentRef");
-    featureDefinitions.AddAttribute("Id", "SHORTCUT");
-    featureDefinitions.EndElement();
-    directoryDefinitions.BeginElement("Directory");
-    directoryDefinitions.AddAttribute("Id", "ProgramMenuFolder");
-    directoryDefinitions.BeginElement("Directory");
-    directoryDefinitions.AddAttribute("Id", "PROGRAM_MENU_FOLDER");
-    const char *startMenuFolder = GetOption("CPACK_WIX_PROGRAM_MENU_FOLDER");
-    directoryDefinitions.AddAttribute("Name", startMenuFolder);
     }
 
-  featureDefinitions.EndElement();
+  featureDefinitions.EndElement("FeatureRef");
-  featureDefinitions.EndElement();
+  featureDefinitions.EndElement("Fragment");
-  fileDefinitions.EndElement();
+  fileDefinitions.EndElement("Fragment");
-  directoryDefinitions.EndElement();
+
+  for(size_t i = 1; i < install_root.size(); ++i)
+    {
+    directoryDefinitions.EndElement("Directory");
+    }
+
+  directoryDefinitions.EndElement("Directory");
+  directoryDefinitions.EndElement("Directory");
+  directoryDefinitions.EndElement("Fragment");
 
   std::string wixTemplate = FindTemplate("WIX.template.in");
   if(GetOption("CPACK_WIX_TEMPLATE") != 0)
@@ -508,6 +450,106 @@ bool cmCPackWIXGenerator::CreateWiXSourceFiles()
   return true;
 }
 
+bool cmCPackWIXGenerator::CreateStartMenuShortcuts(
+  cmWIXSourceWriter& directoryDefinitions,
+  cmWIXSourceWriter& fileDefinitions,
+  cmWIXSourceWriter& featureDefinitions)
+{
+  if(shortcutMap.empty())
+    {
+      return true;
+    }
+
+  std::string cpackVendor;
+  if(!RequireOption("CPACK_PACKAGE_VENDOR", cpackVendor))
+    {
+    return false;
+    }
+
+  std::string cpackPackageName;
+  if(!RequireOption("CPACK_PACKAGE_NAME", cpackPackageName))
+    {
+    return false;
+    }
+
+  fileDefinitions.BeginElement("DirectoryRef");
+  fileDefinitions.AddAttribute("Id", "PROGRAM_MENU_FOLDER");
+  fileDefinitions.BeginElement("Component");
+  fileDefinitions.AddAttribute("Id", "SHORTCUT");
+  fileDefinitions.AddAttribute("Guid", "*");
+
+  for(shortcut_map_t::const_iterator
+    i = shortcutMap.begin(); i != shortcutMap.end(); ++i)
+    {
+    std::string const& id = i->first;
+    cmWIXShortcut const& shortcut = i->second;
+
+    std::string shortcutId = std::string("CM_S") + id;
+    std::string fileId = std::string("CM_F") + id;
+
+    fileDefinitions.BeginElement("Shortcut");
+    fileDefinitions.AddAttribute("Id", shortcutId);
+    fileDefinitions.AddAttribute("Name", shortcut.textLabel);
+    std::string target = "[#" + fileId + "]";
+    fileDefinitions.AddAttribute("Target", target);
+    fileDefinitions.AddAttribute("WorkingDirectory",
+      shortcut.workingDirectoryId);
+    fileDefinitions.EndElement("Shortcut");
+    }
+
+  CreateUninstallShortcut(cpackPackageName, fileDefinitions);
+
+  fileDefinitions.BeginElement("RemoveFolder");
+  fileDefinitions.AddAttribute("Id", "PROGRAM_MENU_FOLDER");
+  fileDefinitions.AddAttribute("On", "uninstall");
+  fileDefinitions.EndElement("RemoveFolder");
+
+  std::string registryKey =
+    std::string("Software/") + cpackVendor + "/" + cpackPackageName;
+
+  fileDefinitions.BeginElement("RegistryValue");
+  fileDefinitions.AddAttribute("Root", "HKCU");
+  fileDefinitions.AddAttribute("Key", registryKey);
+  fileDefinitions.AddAttribute("Name", "installed");
+  fileDefinitions.AddAttribute("Type", "integer");
+  fileDefinitions.AddAttribute("Value", "1");
+  fileDefinitions.AddAttribute("KeyPath", "yes");
+  fileDefinitions.EndElement("RegistryValue");
+
+  fileDefinitions.EndElement("Component");
+  fileDefinitions.EndElement("DirectoryRef");
+
+  featureDefinitions.BeginElement("ComponentRef");
+  featureDefinitions.AddAttribute("Id", "SHORTCUT");
+  featureDefinitions.EndElement("ComponentRef");
+
+  directoryDefinitions.BeginElement("Directory");
+  directoryDefinitions.AddAttribute("Id", "ProgramMenuFolder");
+
+  directoryDefinitions.BeginElement("Directory");
+  directoryDefinitions.AddAttribute("Id", "PROGRAM_MENU_FOLDER");
+  const char *startMenuFolder = GetOption("CPACK_WIX_PROGRAM_MENU_FOLDER");
+  directoryDefinitions.AddAttribute("Name", startMenuFolder);
+  directoryDefinitions.EndElement("Directory");
+
+  directoryDefinitions.EndElement("Directory");
+
+  return true;
+}
+
+void cmCPackWIXGenerator::CreateUninstallShortcut(
+  std::string const& packageName,
+  cmWIXSourceWriter& fileDefinitions)
+{
+  fileDefinitions.BeginElement("Shortcut");
+  fileDefinitions.AddAttribute("Id", "UNINSTALL");
+  fileDefinitions.AddAttribute("Name", "Uninstall " + packageName);
+  fileDefinitions.AddAttribute("Description", "Uninstalls " + packageName);
+  fileDefinitions.AddAttribute("Target", "[SystemFolder]msiexec.exe");
+  fileDefinitions.AddAttribute("Arguments", "/x [ProductCode]");
+  fileDefinitions.EndElement("Shortcut");
+}
+
 bool cmCPackWIXGenerator::CreateLicenseFile()
 {
   std::string licenseSourceFilename;
@@ -561,8 +603,7 @@ void cmCPackWIXGenerator::AddDirectoryAndFileDefinitons(
   cmWIXSourceWriter& directoryDefinitions,
   cmWIXSourceWriter& fileDefinitions,
   cmWIXSourceWriter& featureDefinitions,
-  const std::vector<std::string>& pkgExecutables,
+  const std::vector<std::string>& packageExecutables)
-  std::vector<std::string>& dirIdExecutables)
 {
   cmsys::Directory dir;
   dir.Load(topdir.c_str());
@@ -596,9 +637,9 @@ void cmCPackWIXGenerator::AddDirectoryAndFileDefinitons(
         directoryDefinitions,
         fileDefinitions,
         featureDefinitions,
-        pkgExecutables,
+        packageExecutables);
-        dirIdExecutables);
+
-      directoryDefinitions.EndElement();
+      directoryDefinitions.EndElement("Directory");
       }
     else
       {
@@ -617,28 +658,25 @@ void cmCPackWIXGenerator::AddDirectoryAndFileDefinitons(
       fileDefinitions.AddAttribute("Source", fullPath);
       fileDefinitions.AddAttribute("KeyPath", "yes");
 
-      fileDefinitions.EndElement();
+      fileDefinitions.EndElement("File");
-      fileDefinitions.EndElement();
+      fileDefinitions.EndElement("Component");
-      fileDefinitions.EndElement();
+      fileDefinitions.EndElement("DirectoryRef");
 
       featureDefinitions.BeginElement("ComponentRef");
       featureDefinitions.AddAttribute("Id", componentId);
-      featureDefinitions.EndElement();
+      featureDefinitions.EndElement("ComponentRef");
 
-      std::vector<std::string>::const_iterator it;
+      for(size_t j = 0; j < packageExecutables.size(); ++j)
-      for (it = pkgExecutables.begin() ;
-           it != pkgExecutables.end() ;
-           ++it)
         {
-        std::string execName = *it++;
+        std::string const& executableName = packageExecutables[j++];
-        std::string iconName = *it;
+        std::string const& textLabel = packageExecutables[j];
 
-        if (cmSystemTools::LowerCase(fileName) ==
+        if(cmSystemTools::LowerCase(fileName) ==
-            cmSystemTools::LowerCase(execName) + ".exe")
+            cmSystemTools::LowerCase(executableName) + ".exe")
           {
-            dirIdExecutables.push_back(fileName);
+          cmWIXShortcut &shortcut = shortcutMap[id];
-            dirIdExecutables.push_back(iconName);
+          shortcut.textLabel= textLabel;
-            dirIdExecutables.push_back(directoryId);
+          shortcut.workingDirectoryId = directoryId;
           }
         }
       }

Source/CPack/WiX/cmCPackWIXGenerator.h

@@ -18,6 +18,12 @@
 #include <string>
 #include <map>
 
+struct cmWIXShortcut
+{
+  std::string textLabel;
+  std::string workingDirectoryId;
+};
+
 class cmWIXSourceWriter;
 
 /** \class cmCPackWIXGenerator
@@ -56,6 +62,7 @@ protected:
 private:
   typedef std::map<std::string, std::string> id_map_t;
   typedef std::map<std::string, size_t> ambiguity_map_t;
+  typedef std::map<std::string, cmWIXShortcut> shortcut_map_t;
 
   bool InitializeWiXConfiguration();
 
@@ -71,6 +78,15 @@ private:
 
   bool CreateWiXSourceFiles();
 
+  bool CreateStartMenuShortcuts(
+    cmWIXSourceWriter& directoryDefinitions,
+    cmWIXSourceWriter& fileDefinitions,
+    cmWIXSourceWriter& featureDefinitions);
+
+  void CreateUninstallShortcut(
+    std::string const& packageName,
+    cmWIXSourceWriter& fileDefinitions);
+
   void AppendUserSuppliedExtraSources();
 
   void AppendUserSuppliedExtraObjects(std::ostream& stream);
@@ -89,9 +105,7 @@ private:
     cmWIXSourceWriter& directoryDefinitions,
     cmWIXSourceWriter& fileDefinitions,
     cmWIXSourceWriter& featureDefinitions,
-    const std::vector<std::string>& pkgExecutables,
+    const std::vector<std::string>& pkgExecutables);
-    std::vector<std::string>& dirIdExecutables
-    );
 
   bool RequireOption(const std::string& name, std::string& value) const;
 
@@ -118,6 +132,7 @@ private:
   std::vector<std::string> wixSources;
   id_map_t pathToIdMap;
   ambiguity_map_t idAmbiguityCounter;
+  shortcut_map_t shortcutMap;
 };
 
 #endif

Source/CPack/WiX/cmWIXSourceWriter.cxx

@@ -21,7 +21,8 @@ cmWIXSourceWriter::cmWIXSourceWriter(cmCPackLog* logger,
   bool isIncludeFile):
     Logger(logger),
     file(filename.c_str()),
-    state(DEFAULT)
+    state(DEFAULT),
+    sourceFilename(filename)
 {
   WriteXMLDeclaration();
 
@@ -39,10 +40,15 @@ cmWIXSourceWriter::cmWIXSourceWriter(cmCPackLog* logger,
 
 cmWIXSourceWriter::~cmWIXSourceWriter()
 {
-  while(elements.size())
+  if(elements.size() > 1)
     {
-    EndElement();
+    cmCPackLogger(cmCPackLog::LOG_ERROR,
+      elements.size() - 1 << " WiX elements were still open when closing '" <<
+      sourceFilename << "'" << std::endl);
+    return;
     }
+
+  EndElement(elements.back());
 }
 
 void cmWIXSourceWriter::BeginElement(const std::string& name)
@@ -60,12 +66,22 @@ void cmWIXSourceWriter::BeginElement(const std::string& name)
   state = BEGIN;
 }
 
-void cmWIXSourceWriter::EndElement()
+void cmWIXSourceWriter::EndElement(std::string const& name)
 {
   if(elements.empty())
     {
     cmCPackLogger(cmCPackLog::LOG_ERROR,
-      "can not end WiX element with no open elements" << std::endl);
+      "can not end WiX element with no open elements in '" <<
+      sourceFilename << "'" << std::endl);
+    return;
+    }
+
+  if(elements.back() != name)
+    {
+    cmCPackLogger(cmCPackLog::LOG_ERROR,
+      "WiX element <" << elements.back() <<
+      "> can not be closed by </" << name << "> in '" <<
+      sourceFilename << "'" << std::endl);
     return;
     }
 
@@ -173,6 +189,9 @@ std::string cmWIXSourceWriter::EscapeAttributeValue(
     case '<':
       result += "&lt;";
       break;
+    case '>':
+      result += "&gt;";
+      break;
     case '&':
       result +="&amp;";
       break;

Source/CPack/WiX/cmWIXSourceWriter.h

@@ -32,7 +32,7 @@ public:
 
   void BeginElement(const std::string& name);
 
-  void EndElement();
+  void EndElement(const std::string& name);
 
   void AddProcessingInstruction(
     const std::string& target, const std::string& content);
@@ -62,6 +62,8 @@ private:
   State state;
 
   std::vector<std::string> elements;
+
+  std::string sourceFilename;
 };
 
 #endif

Tests/CPackWiXGenerator/CMakeLists.txt

@@ -4,15 +4,17 @@ project(CPackWiXGenerator)
 
 add_library(mylib mylib.cpp)
 
-add_executable(mylibapp mylibapp.cpp)
+add_executable(my-libapp mylibapp.cpp)
-target_link_libraries(mylibapp mylib)
+target_link_libraries(my-libapp mylib)
+
+add_executable(my-other-app myotherapp.cpp)
 
 install(TARGETS mylib
   ARCHIVE
   DESTINATION lib
   COMPONENT libraries)
 
-install(TARGETS mylibapp
+install(TARGETS my-libapp my-other-app
   RUNTIME
   DESTINATION bin
   COMPONENT applications)
@@ -36,7 +38,11 @@ set(CPACK_PACKAGE_INSTALL_DIRECTORY "CPack Component Example")
 
 set(CPACK_WIX_UPGRADE_GUID "BF20CE5E-7F7C-401D-8F7C-AB45E8D170E6")
 set(CPACK_WIX_UNINSTALL "1")
-set(CPACK_PACKAGE_EXECUTABLES "mylibapp; CPack Wix Test")
+
+set(CPACK_PACKAGE_EXECUTABLES
+  "my-libapp" "CPack WiX Test"
+  "my-other-app" "Second CPack WiX Test"
+)
 
 include(CPack)
 

Tests/CPackWiXGenerator/myotherapp.cpp

@@ -0,0 +1 @@
+int main() {}