Windows: Win32 migration tools code review

Signed-off-by: Michael Schuster <michael@schuster.ms>

admin/win/tools/CMakeLists.txt

@@ -22,38 +22,36 @@ add_definitions(-D_WINDOWS)
 add_definitions(-D_WIN32_WINNT=0x0601)
 add_definitions(-DWINVER=0x0601)
 
-if(MSVC)
-    # Use automatic overload for suitable CRT safe-functions
-    # See https://docs.microsoft.com/de-de/cpp/c-runtime-library/security-features-in-the-crt?view=vs-2019
-    add_definitions(-D_CRT_SECURE_CPP_OVERLOAD_STANDARD_NAMES=1)
-    # Also: Disable compiler warnings because we don't use Windows CRT safe-functions explicitly and don't intend to
-    # as this is a pure cross-platform source the only alternative would be a ton of ifdefs with calls to the _s version
-    add_definitions(-D_CRT_SECURE_NO_WARNINGS)
+# Use automatic overload for suitable CRT safe-functions
+# See https://docs.microsoft.com/de-de/cpp/c-runtime-library/security-features-in-the-crt?view=vs-2019
+add_definitions(-D_CRT_SECURE_CPP_OVERLOAD_STANDARD_NAMES=1)
+# Also: Disable compiler warnings because we don't use Windows CRT safe-functions explicitly and don't intend to
+# as this is a pure cross-platform source the only alternative would be a ton of ifdefs with calls to the _s version
+add_definitions(-D_CRT_SECURE_NO_WARNINGS)
 
-    # Optimize for size
-    set(COMPILER_FLAGS "/GL /O1 /sdl /Zc:inline /Oi /EHsc /nologo")
-    set(LINKER_FLAGS "/LTCG /OPT:REF /SUBSYSTEM:WINDOWS /NOLOGO")
+# Optimize for size
+set(COMPILER_FLAGS "/GL /O1 /sdl /Zc:inline /Oi /EHsc /nologo")
+set(LINKER_FLAGS "/LTCG /OPT:REF /SUBSYSTEM:WINDOWS /NOLOGO")
 
-    # Enable DEP, ASLR and CFG
-    set(LINKER_FLAGS "${LINKER_FLAGS} /nxcompat /dynamicbase /guard:cf")
+# Enable DEP, ASLR and CFG
+set(LINKER_FLAGS "${LINKER_FLAGS} /nxcompat /dynamicbase /guard:cf")
 
-    # x86 only: Enable SafeSEH
-    if(CMAKE_SIZEOF_VOID_P MATCHES 4)
-        set(LINKER_FLAGS "${LINKER_FLAGS} /safeseh")
-    endif()
-
-    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${COMPILER_FLAGS}")
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${COMPILER_FLAGS}")
-
-    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${LINKER_FLAGS}")
-    set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${LINKER_FLAGS}")
-
-    # Use static runtime for all subdirectories
-    foreach(buildType "" "_DEBUG" "_MINSIZEREL" "_RELEASE" "_RELWITHDEBINFO")
-        string(REPLACE "/MD" "/MT" "CMAKE_CXX_FLAGS${buildType}" "${CMAKE_CXX_FLAGS${buildType}}")
-    endforeach()
+# x86 only: Enable SafeSEH
+if(CMAKE_SIZEOF_VOID_P MATCHES 4)
+    set(LINKER_FLAGS "${LINKER_FLAGS} /safeseh")
 endif()
 
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${COMPILER_FLAGS}")
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${COMPILER_FLAGS}")
+
+set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${LINKER_FLAGS}")
+set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${LINKER_FLAGS}")
+
+# Use static runtime for all subdirectories
+foreach(buildType "" "_DEBUG" "_MINSIZEREL" "_RELEASE" "_RELWITHDEBINFO")
+    string(REPLACE "/MD" "/MT" "CMAKE_CXX_FLAGS${buildType}" "${CMAKE_CXX_FLAGS${buildType}}")
+endforeach()
+
 add_subdirectory(NCToolsShared)
 
 if(BUILD_WIN_MSI)

admin/win/tools/NCMsiHelper/CMakeLists.txt

@@ -7,18 +7,12 @@ else()
     message(STATUS "WiX Toolset SDK path: ${WIX_SDK_PATH}")
 endif()
 
-include_directories(
-    ${WIX_SDK_PATH}/inc
-)
+include_directories(${WIX_SDK_PATH}/inc)
 
 if(CMAKE_SIZEOF_VOID_P MATCHES 4)
-    link_directories(
-        ${WIX_SDK_PATH}/lib/x86
-    )
+    link_directories(${WIX_SDK_PATH}/lib/x86)
 else()
-    link_directories(
-        ${WIX_SDK_PATH}/lib/x64
-    )
+    link_directories(${WIX_SDK_PATH}/lib/x64)
 endif()
 
 add_definitions(-D_NCMSIHELPER_EXPORTS)

admin/win/tools/NCMsiHelper/CustomAction.cpp

@@ -19,7 +19,6 @@
  * 
  */
 
-#include "NCTools.h"
 #include "NCMsiHelper.h"
 
 /**
@@ -48,22 +47,17 @@
  */
 UINT CustomActionArgcArgv(MSIHANDLE hInstall, CUSTOM_ACTION_ARGC_ARGV func, LPCSTR actionName)
 {
-	HRESULT hr = S_OK;
-	UINT er = ERROR_SUCCESS;
-    LPWSTR pszCustomActionData = nullptr;
-    int argc = 0;
-    LPWSTR *argv = nullptr;
+    LPWSTR pszCustomActionData = nullptr, *argv = nullptr;
 
-	hr = WcaInitialize(hInstall, actionName);
-	ExitOnFailure(hr, "Failed to initialize");
+    HRESULT hr = WcaInitialize(hInstall, actionName);
+    ExitOnFailure(hr, "Failed to initialize");
 
-	WcaLog(LOGMSG_STANDARD, "Initialized.");
+    WcaLog(LOGMSG_STANDARD, "Initialized.");
     
     // Retrieve our custom action property. This is one of
     // only three properties we can request on a Deferred
     // Custom Action.  So, we assume the caller puts all
     // parameters in this one property.
-    pszCustomActionData = nullptr;
     hr = WcaGetProperty(L"CustomActionData", &pszCustomActionData);
     ExitOnFailure(hr, "Failed to get Custom Action Data.");
     WcaLog(LOGMSG_STANDARD, "Custom Action Data = '%ls'.", pszCustomActionData);
@@ -71,9 +65,9 @@ UINT CustomActionArgcArgv(MSIHANDLE hInstall, CUSTOM_ACTION_ARGC_ARGV func, LPCS
     // Convert the string retrieved into a standard argc/arg layout
     // (ignoring the fact that the first parameter is whatever was
     // passed, not necessarily the application name/path).
+    int argc = 0;
     argv = CommandLineToArgvW(pszCustomActionData, &argc);
-    if (argv)
-    {
+    if (argv) {
         hr = HRESULT_FROM_WIN32(GetLastError());
         ExitOnFailure(hr, "Failed to convert Custom Action Data to argc/argv.");
     }
@@ -87,8 +81,7 @@ LExit:
     if (argv)
         LocalFree(argv);
 
-	er = SUCCEEDED(hr) ? ERROR_SUCCESS : ERROR_INSTALL_FAILURE;
-	return WcaFinalize(er);
+    return WcaFinalize(SUCCEEDED(hr) ? ERROR_SUCCESS : ERROR_INSTALL_FAILURE);
 }
 
 UINT __stdcall ExecNsisUninstaller(MSIHANDLE hInstall)
@@ -105,21 +98,21 @@ UINT __stdcall RemoveNavigationPaneEntries(MSIHANDLE hInstall)
  * DllMain - Initialize and cleanup WiX custom action utils.
  */
 extern "C" BOOL WINAPI DllMain(
-	__in HINSTANCE hInst,
-	__in ULONG ulReason,
-	__in LPVOID
-	)
+    __in HINSTANCE hInst,
+    __in ULONG ulReason,
+    __in LPVOID
+    )
 {
-	switch(ulReason)
-	{
-	case DLL_PROCESS_ATTACH:
-		WcaGlobalInitialize(hInst);
-		break;
+    switch(ulReason)
+    {
+    case DLL_PROCESS_ATTACH:
+        WcaGlobalInitialize(hInst);
+        break;
 
-	case DLL_PROCESS_DETACH:
-		WcaGlobalFinalize();
-		break;
-	}
+    case DLL_PROCESS_DETACH:
+        WcaGlobalFinalize();
+        break;
+    }
 
-	return TRUE;
+    return TRUE;
 }

admin/win/tools/NCMsiHelper/LogResult.cpp

@@ -19,7 +19,6 @@
  * 
  */
 
-#include "NCTools.h"
 #include "NCMsiHelper.h"
 
 //
@@ -64,14 +63,14 @@ void LogError(DWORD dwErrorMsgId)
             hInst, // Handle to the DLL.
             dwMessageId, // Message identifier.
             MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), // Default language.
-            (LPSTR)&pBuffer, // Buffer that will hold the text string.
+            reinterpret_cast<LPSTR>(&pBuffer), // Buffer that will hold the text string.
             ERRMSGBUFFERSIZE, // Allocate at least this many chars for pBuffer.
             nullptr // No insert values.
         );
     }
 
     if (0 < ret && nullptr != pBuffer) {
-        pMessage = (LPSTR)pBuffer;
+        pMessage = static_cast<LPSTR>(pBuffer);
     }
 
     // Display the string.
@@ -102,21 +101,20 @@ void LogResult(
     va_start(args, fmt);
 #pragma warning(push)
 #pragma warning(disable : 4996)
-    auto len = _vsnprintf(nullptr, 0, fmt, args) + 1;
+    const auto len = _vsnprintf(nullptr, 0, fmt, args) + 1;
 #pragma warning(pop)
-    auto buffer = (char*)malloc(len * sizeof(char));
+    auto buffer = static_cast<char*>(malloc(len * sizeof(char)));
 
 #ifdef _DEBUG
     ::ZeroMemory(buffer, len);
 #endif // _DEBUG
-    _vsnprintf_s(buffer, len, len-1, fmt, args);
+    _vsnprintf_s(buffer, len, len - 1, fmt, args);
 
     // (MSDN code complete)
 
     // Now that the buffer holds the formatted string, send it to
     // the appropriate output.
-    if (WcaIsInitialized())
-    {
+    if (WcaIsInitialized()) {
         if (FAILED(hr)) {
             WcaLogError(hr, buffer);
             LogError(hr);
@@ -124,8 +122,7 @@ void LogResult(
             WcaLog(LOGMSG_STANDARD, buffer);
         }
     } else { // Log to stdout/stderr
-        if (FAILED(hr))
-        {
+        if (FAILED(hr)) {
             fprintf_s(stderr, "%s\n", buffer);
             LogError(hr);
         } else {

admin/win/tools/NCMsiHelper/NCMsiHelper.cpp

@@ -12,10 +12,9 @@
  * for more details.
  */
 
-#include "NCTools.h"
+#include "NCMsiHelper.h"
 #include "utility.h"
 #include "LogResult.h"
-#include "NCMsiHelper.h"
 
 using namespace NCTools;
 
@@ -25,8 +24,8 @@ HRESULT NCMSIHELPER_API DoExecNsisUninstaller(int argc, LPWSTR *argv)
         return HRESULT_FROM_WIN32(ERROR_INVALID_PARAMETER);
     }
 
-    auto appShortName = std::wstring(argv[0]);
-    auto uninstallExePath = std::wstring(argv[1]);
+    const auto appShortName = std::wstring(argv[0]);
+    const auto uninstallExePath = std::wstring(argv[1]);
 
     if (appShortName.empty()
          || uninstallExePath.empty()) {
@@ -34,7 +33,7 @@ HRESULT NCMSIHELPER_API DoExecNsisUninstaller(int argc, LPWSTR *argv)
     }
 
     auto appInstallDir = uninstallExePath;
-    auto posLastSlash = appInstallDir.find_last_of(PathSeparator);
+    const auto posLastSlash = appInstallDir.find_last_of(PathSeparator);
     if (posLastSlash != std::wstring::npos) {
         appInstallDir.erase(posLastSlash);
     } else {
@@ -42,7 +41,7 @@ HRESULT NCMSIHELPER_API DoExecNsisUninstaller(int argc, LPWSTR *argv)
     }
 
     // Run uninstaller
-    std::wstring cmd = L'\"' + uninstallExePath + L"\" /S _?=" + appInstallDir;
+    const std::wstring cmd = L'\"' + uninstallExePath + L"\" /S _?=" + appInstallDir;
     LogResult(S_OK, "Running '%ls'.", cmd.data());
     Utility::execCmd(cmd);
 
@@ -69,7 +68,7 @@ HRESULT NCMSIHELPER_API DoRemoveNavigationPaneEntries(int argc, LPWSTR *argv)
         return HRESULT_FROM_WIN32(ERROR_INVALID_PARAMETER);
     }
 
-    auto appName = std::wstring(argv[0]);
+    const auto appName = std::wstring(argv[0]);
 
     if (appName.empty()) {
         return HRESULT_FROM_WIN32(ERROR_INVALID_PARAMETER);

admin/win/tools/NCMsiHelper/NCMsiHelper.h

@@ -33,6 +33,8 @@
  */
 #pragma once
 
+#include <windows.h>
+
 #ifdef _NCMSIHELPER_EXPORTS
 #  pragma comment (lib, "newdev")
 #  pragma comment (lib, "setupapi")
@@ -41,8 +43,10 @@
 #  pragma comment (lib, "wcautil")
 #  pragma comment (lib, "Version")
 
+#  include <cstdlib>
+#  include <string>
+#  include <tchar.h>
 #  include <msiquery.h>
-#  include <stdlib.h>
 #  include <lmerr.h>
 
 // WiX Header Files:
@@ -92,5 +96,4 @@ HRESULT NCMSIHELPER_API DoRemoveNavigationPaneEntries(int argc, LPWSTR *argv);
  *  As a result, all functions defined in this header should
  *  conform to this function prototype.
  */
-typedef HRESULT NCMSIHELPER_API (*CUSTOM_ACTION_ARGC_ARGV)(
-    int argc, LPWSTR *argv);
+using CUSTOM_ACTION_ARGC_ARGV = NCMSIHELPER_API HRESULT(*)(int argc, LPWSTR *argv);

admin/win/tools/NCNavRemove/ConfigIni.cpp

@@ -12,7 +12,7 @@
  * for more details.
  */
 
-#include "NCTools.h"
+#include <windows.h>
 #include "3rdparty/SimpleIni.h"
 #include "NavRemoveConstants.h"
 #include "ConfigIni.h"
@@ -23,8 +23,7 @@ ConfigIni::ConfigIni()
 
 bool ConfigIni::load()
 {
-    std::wstring filename;
-    DWORD bufferLen = GetCurrentDirectory(0, nullptr);
+    const DWORD bufferLen = GetCurrentDirectory(0, nullptr);
     TCHAR *pszBuffer = nullptr;
 
     if (bufferLen == 0) {
@@ -36,10 +35,11 @@ bool ConfigIni::load()
         return false;
     }
 
+    std::wstring filename;
     if (GetCurrentDirectory(bufferLen, pszBuffer) != 0) {
         filename = pszBuffer;
     }
-    delete [] pszBuffer;
+    delete[] pszBuffer;
 
     if (filename.empty()) {
         return false;
@@ -52,18 +52,17 @@ bool ConfigIni::load()
     const wchar_t iniSection[] = CFG_KEY;
     const wchar_t iniKey[] = CFG_VAR_APPNAME;
 
-    auto rc = ini.LoadFile(filename.data());
+    const auto rc = ini.LoadFile(filename.data());
 
     if (rc != SI_OK) {
         return false;
     }
 
-    auto pv = ini.GetValue(iniSection, iniKey);
+    const auto pv = ini.GetValue(iniSection, iniKey);
     bool success = false;
 
     if (pv) {
         _appName = pv;
-        
         success = !_appName.empty();
     }
  
@@ -72,7 +71,7 @@ bool ConfigIni::load()
     return success;
 }
 
-const std::wstring ConfigIni::getAppName() const
+std::wstring ConfigIni::getAppName() const
 {
     return _appName;
 }

admin/win/tools/NCNavRemove/ConfigIni.h

@@ -14,7 +14,7 @@
 
 #pragma once
 
-#include "NCTools.h"
+#include <string>
 
 class ConfigIni
 {
@@ -23,7 +23,7 @@ public:
 
     bool load();
 
-    const std::wstring getAppName() const;
+    std::wstring getAppName() const;
 
 private:
     std::wstring _appName;

admin/win/tools/NCNavRemove/dll/NavRemove.cpp

@@ -12,7 +12,7 @@
  * for more details.
  */
 
-#include "NCTools.h"
+#include <windows.h>
 #include "utility.h"
 #include "NavRemove.h"
 #include "../ConfigIni.h"

admin/win/tools/NCNavRemove/dll/NavRemove.h

@@ -14,7 +14,7 @@
 
 #pragma once
 
-#include "NCTools.h"
+#include <windows.h>
 
 // The following ifdef block is the standard way of creating macros which make exporting
 // from a DLL simpler. All files within this DLL are compiled with the _NAVREMOVE_EXPORTS

admin/win/tools/NCNavRemove/dll/dllmain.cpp

@@ -12,31 +12,31 @@
  * for more details.
  */
 
-#include "NCTools.h"
-#include "SimpleMutex.h"
+#include <windows.h>
+#include "SimpleNamedMutex.h"
 #include "NavRemoveConstants.h"
 
-SimpleMutex g_mutex;
+SimpleNamedMutex g_mutex(std::wstring(MUTEX_NAME));
 bool g_alreadyRunning = false;
 
 extern "C" BOOL WINAPI DllMain(
-	__in HINSTANCE hInst,
-	__in ULONG ulReason,
-	__in LPVOID
-	)
+    __in HINSTANCE hInst,
+    __in ULONG ulReason,
+    __in LPVOID
+    )
 {
-	switch(ulReason)
-	{
-	case DLL_PROCESS_ATTACH:
+    switch(ulReason)
+    {
+    case DLL_PROCESS_ATTACH:
         // Mutex
-        g_alreadyRunning = !g_mutex.create(std::wstring(MUTEX_NAME));
-		break;
+        g_alreadyRunning = !g_mutex.lock();
+        break;
 
-	case DLL_PROCESS_DETACH:
+    case DLL_PROCESS_DETACH:
         // Release mutex
-        g_mutex.release();
-		break;
-	}
+        g_mutex.unlock();
+        break;
+    }
 
-	return TRUE;
+    return TRUE;
 }

admin/win/tools/NCNavRemove/exe/main.cpp

@@ -12,9 +12,9 @@
  * for more details.
  */
 
-#include "NCTools.h"
+#include <windows.h>
 #include "utility.h"
-#include "SimpleMutex.h"
+#include "SimpleNamedMutex.h"
 #include "NavRemoveConstants.h"
 #include "../ConfigIni.h"
 
@@ -31,9 +31,9 @@ int APIENTRY wWinMain(_In_ HINSTANCE hInstance,
     UNREFERENCED_PARAMETER(nCmdShow);
 
     // Mutex
-    SimpleMutex mutex;
+    SimpleNamedMutex mutex(std::wstring(MUTEX_NAME));
 
-    if (!mutex.create(std::wstring(MUTEX_NAME))) {
+    if (!mutex.lock()) {
         return 0;
     }
 
@@ -47,7 +47,7 @@ int APIENTRY wWinMain(_In_ HINSTANCE hInstance,
     Utility::removeNavigationPaneEntries(ini.getAppName());
 
     // Release mutex
-    mutex.release();
+    mutex.unlock();
 
     return 0;
 }

admin/win/tools/NCToolsShared/CMakeLists.txt

@@ -1,4 +1,4 @@
 add_library(NCToolsShared STATIC
     utility_win.cpp
-    SimpleMutex.cpp
+    SimpleNamedMutex.cpp
 )

admin/win/tools/NCToolsShared/NCTools.h

@@ -1,31 +0,0 @@
-// NCTools.h : include file for standard system include files
-//
-
-#pragma once
-
-#include <WinSDKVer.h>
-
-// // Including SDKDDKVer.h defines the highest available Windows platform.
-// If you wish to build your application for a previous Windows platform, include WinSDKVer.h and
-// set the _WIN32_WINNT macro to the platform you wish to support before including SDKDDKVer.h.
-#include <SDKDDKVer.h>
-
-#define WIN32_LEAN_AND_MEAN             // Exclude rarely-used stuff from Windows headers
-
-// Windows Header Files
-#include <windows.h>
-#include <shellapi.h>
-#include <Shlobj.h>
-#include <psapi.h>
-#include <wincred.h>
-
-// C RunTime Header Files
-#include <cstdlib>
-#include <malloc.h>
-#include <memory.h>
-#include <tchar.h>
-#include <algorithm>
-#include <functional>
-#include <string>
-#include <vector>
-#include <variant>

admin/win/tools/NCToolsShared/SimpleNamedMutex.cpp

@@ -12,19 +12,21 @@
  * for more details.
  */
 
-#include "NCTools.h"
-#include "SimpleMutex.h"
+#include "SimpleNamedMutex.h"
 
-SimpleMutex::SimpleMutex()
+SimpleNamedMutex::SimpleNamedMutex(const std::wstring &name)
 {
+    _name = name;
 }
 
-bool SimpleMutex::create(const std::wstring &name)
+bool SimpleNamedMutex::lock()
 {
-    release();
+    if (_name.empty() || _hMutex) {
+        return false;
+    }
 
     // Mutex
-    _hMutex = CreateMutex(nullptr, TRUE, name.data());
+    _hMutex = CreateMutex(nullptr, TRUE, _name.data());
 
     if (GetLastError() == ERROR_ALREADY_EXISTS) {
         CloseHandle(_hMutex);
@@ -35,7 +37,7 @@ bool SimpleMutex::create(const std::wstring &name)
     return true;
 }
 
-void SimpleMutex::release()
+void SimpleNamedMutex::unlock()
 {
     // Release mutex
     if (_hMutex) {

admin/win/tools/NCToolsShared/SimpleNamedMutex.h

@@ -14,16 +14,18 @@
 
 #pragma once
 
-#include "NCTools.h"
+#include <windows.h>
+#include <string>
 
-class SimpleMutex
+class SimpleNamedMutex
 {
 public:
-    SimpleMutex();
+    SimpleNamedMutex(const std::wstring &name);
 
-    bool create(const std::wstring &name);
-    void release();
+    bool lock();
+    void unlock();
 
 private:
+    std::wstring _name;
     HANDLE _hMutex = nullptr;
 };

admin/win/tools/NCToolsShared/utility.h

@@ -20,7 +20,11 @@
 
 #pragma once
 
-#include "NCTools.h"
+#include <windows.h>
+#include <string>
+#include <vector>
+#include <variant>
+#include <functional>
 
 namespace NCTools {
 

admin/win/tools/NCToolsShared/utility_win.cpp

@@ -17,9 +17,11 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
-#include <cassert>
-#include "NCTools.h"
 #include "utility.h"
+#include <cassert>
+#include <algorithm>
+#include <Shlobj.h>
+#include <psapi.h>
 
 #define ASSERT assert
 #define Q_ASSERT assert