MDEV-33480 Windows MSI installer, convert from WIX v3 to supported versions#5338
MDEV-33480 Windows MSI installer, convert from WIX v3 to supported versions#5338vaintroub wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Windows packaging configuration to support both modern WiX (v5/v6) and legacy WiX v3.x toolsets, refactoring the custom action C++ code to use native MSI APIs instead of legacy WiX utilities. The review feedback highlights several critical issues in the C++ changes, including incorrect error handling where GetLastError() is used instead of checking MsiGetProperty return codes, a bug where the size variable is not re-initialized before retrieving INSTALLDIR, and multiple potential buffer overflows or crashes when using wcscat_s or swprintf_s with MAX_PATH buffers. Additionally, it recommends checking the return values of MsiGetPropertyW and making the WiX version regex matching more robust.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (MsiGetPropertyA(hInstall,"INSTALLDIR", installDir, &size) | ||
| != ERROR_SUCCESS) | ||
| { | ||
| hr = HRESULT_FROM_WIN32(GetLastError()); | ||
| ExitOnFailure(hr, "MsiGetPropertyW failed"); | ||
| er= GetLastError(); | ||
| Log("MsiGetPropertyW failed"); | ||
| goto LExit; | ||
| } |
There was a problem hiding this comment.
The size variable is modified by the previous call to MsiGetPropertyW (line 1010) to hold the length of the ProductVersion string (typically 5-10 characters). Since it is not re-initialized to MAX_PATH before calling MsiGetPropertyA for INSTALLDIR, the function will assume the buffer is only 5-10 characters long and fail with ERROR_MORE_DATA for any installation path longer than that. This completely breaks the service upgrade check. Additionally, MsiGetProperty returns the error code directly rather than setting the thread's last error, so calling GetLastError() is incorrect.
size = MAX_PATH;
er = MsiGetPropertyA(hInstall, "INSTALLDIR", installDir, &size);
if (er != ERROR_SUCCESS)
{
Log("MsiGetPropertyA failed with %u", er);
goto LExit;
}There was a problem hiding this comment.
size is re-initialized right above the call — size= MAX_PATH; precedes MsiGetPropertyA(). The GetLastError() part of the comment was valid and is fixed: the return value of MsiGetProperty* is now captured and logged directly.
| if (MsiGetPropertyW(hInstall, L"ProductVersion", installerVersion, &size) | ||
| != ERROR_SUCCESS) | ||
| { | ||
| hr = HRESULT_FROM_WIN32(GetLastError()); | ||
| ExitOnFailure(hr, "MsiGetPropertyW failed"); | ||
| er= GetLastError(); | ||
| Log("MsiGetPropertyW failed"); | ||
| goto LExit; | ||
| } |
There was a problem hiding this comment.
MsiGetPropertyW returns the error code directly (e.g., ERROR_SUCCESS, ERROR_MORE_DATA) and does not set the thread's last error code. Calling GetLastError() to retrieve the error code is incorrect and will return a stale or unrelated error code. Capture and use the return value of MsiGetPropertyW directly.
er = MsiGetPropertyW(hInstall, L"ProductVersion", installerVersion, &size);
if (er != ERROR_SUCCESS)
{
Log("MsiGetPropertyW failed with %u", er);
goto LExit;
}| wchar_t localpath[MAX_PATH]; | ||
| wcscpy_s(localpath, installdir); | ||
| wcscat_s(localpath, dir); | ||
| localpath[MAX_PATH - 1]= 0; |
There was a problem hiding this comment.
If installdir is close to MAX_PATH in length, appending dir (which can be up to 8 characters) via wcscat_s will exceed MAX_PATH and trigger the invalid parameter handler, crashing the installer. Declaring localpath with a larger size (e.g., MAX_PATH + 20) prevents this crash. Additionally, the explicit null-termination at localpath[MAX_PATH - 1] = 0; is redundant and unsafe since wcscat_s already guarantees null-termination on success.
wchar_t localpath[MAX_PATH + 20];
wcscpy_s(localpath, installdir);
wcscat_s(localpath, dir);|
|
||
| wchar_t servicename[MAX_PATH]; | ||
| wchar_t datadir[MAX_PATH]; | ||
| wchar_t bindir[MAX_PATH]; |
There was a problem hiding this comment.
If INSTALLDIR is close to MAX_PATH in length, appending L"\\bin" via wcscat_s at line 596 will exceed MAX_PATH and trigger the invalid parameter handler, crashing the installer. Declaring bindir with a larger size (e.g., MAX_PATH + 10) prevents this crash.
wchar_t bindir[MAX_PATH + 10];| wcsncpy_s(name, dir, MAX_PATH); | ||
| wcscat_s(name, L"\\*.err"); |
There was a problem hiding this comment.
If dir is close to MAX_PATH in length, appending L"\\*.err" via wcscat_s will exceed MAX_PATH and trigger the invalid parameter handler, crashing the installer. Using _snwprintf_s with _TRUNCATE safely formats and truncates the path instead of crashing.
_snwprintf_s(name, _TRUNCATE, L"%s\\*.err", dir);| #define Log(format, ...) \ | ||
| do{ \ | ||
| wchar_t log_buf[1024]; \ | ||
| swprintf_s(log_buf, 1024, \ | ||
| L"MariaDB_CA %S:" L##format,__FUNCTION__, __VA_ARGS__); \ | ||
| WriteMsiLog(hInstall, log_buf); \ | ||
| } while (0) |
There was a problem hiding this comment.
The Log macro uses swprintf_s to format log messages into a 1024 character buffer. If any log message (such as a line from the error log being dumped in DumpErrorLog) exceeds this size, swprintf_s will invoke the invalid parameter handler and crash the installer. Using _snwprintf_s with _TRUNCATE safely truncates long log messages instead of crashing.
#define Log(format, ...) \
do{\
wchar_t log_buf[1024];\
_snwprintf_s(log_buf, 1024, _TRUNCATE,\
L"MariaDB_CA %S:" L##format,__FUNCTION__, __VA_ARGS__);\
WriteMsiLog(hInstall, log_buf);\
} while (0)| MsiGetPropertyW(hInstall, L"SERVICENAME", servicename, &len); | ||
| datadir[0]= 0; | ||
| len= MAX_PATH; | ||
| MsiGetPropertyW(hInstall,L"DATADIR", datadir, &len); | ||
| bindir[0]= 0; | ||
| len= MAX_PATH; | ||
| MsiGetPropertyW(hInstall,L"INSTALLDIR", bindir, &len); |
There was a problem hiding this comment.
The return value of MsiGetPropertyW is not checked. If MsiGetPropertyW fails or returns ERROR_MORE_DATA, the buffer is left un-terminated, which can lead to buffer over-reads, undefined behavior, or crashes in subsequent string operations (like wcscat_s or logging). Check the return value and safely null-terminate the buffers on failure.
if (MsiGetPropertyW(hInstall, L"SERVICENAME", servicename, &len) != ERROR_SUCCESS)
servicename[0] = L'\0';
datadir[0]= 0;
len= MAX_PATH;
if (MsiGetPropertyW(hInstall,L"DATADIR", datadir, &len) != ERROR_SUCCESS)
datadir[0] = L'\0';
bindir[0]= 0;
len= MAX_PATH;
if (MsiGetPropertyW(hInstall,L"INSTALLDIR", bindir, &len) != ERROR_SUCCESS)
bindir[0] = L'\0';There was a problem hiding this comment.
There is no unterminated-buffer path here: all three buffers are zero-initialized before the calls, a missing property yields an empty string with ERROR_SUCCESS, and on ERROR_MORE_DATA MsiGetProperty stores a truncated, null-terminated value. Empty values are a legitimate state for these properties, handled downstream. Leaving as is.
| OUTPUT_VARIABLE WIX_VERSION_OUTPUT | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE | ||
| RESULT_VARIABLE WIX_VERSION_RESULT) | ||
| STRING(REGEX MATCH "^[0-9]+\\.[0-9]+\\.[0-9]+" WIX_VERSION "${WIX_VERSION_OUTPUT}") |
There was a problem hiding this comment.
The regex ^[0-9]+\.[0-9]+\.[0-9]+ uses the ^ anchor. If wix.exe --version outputs any leading text or banners in future versions, this regex will fail to match. Removing the ^ anchor makes version detection much more robust.
STRING(REGEX MATCH "[0-9]+\\\\.[0-9]+\\\\.[0-9]+" WIX_VERSION "${WIX_VERSION_OUTPUT}")
4cd68b9 to
5f4c029
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Windows packaging configuration to support modern WiX (v5/v6) alongside legacy WiX v3, and removes the dependency on WiX's wcautil and dutil libraries in the custom action DLL by replacing them with native MSI API calls. The review feedback highlights several critical issues: CMake variables need to be unset locally as well as in the cache to prevent fallback failures; the custom Log macro can cause compilation errors due to trailing commas when no variadic arguments are passed; multiple MsiGetPropertyW calls lack return value validation, risking undefined behavior; and minor improvements are suggested for API consistency and safer path formatting.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| IF(WITH_WIX3) | ||
| UNSET(WIX_EXECUTABLE CACHE) |
There was a problem hiding this comment.
When WITH_WIX3 is enabled, unsetting only the cache variable WIX_EXECUTABLE is insufficient because the local variable WIX_EXECUTABLE may still be defined in the current scope (especially on subsequent CMake runs). This will cause the IF(NOT WIX_EXECUTABLE) check on line 62 to evaluate to false, preventing the legacy WiX v3 toolset from being located.
To fix this, unset both the cache and local variables.
IF(WITH_WIX3)
UNSET(WIX_EXECUTABLE CACHE)
UNSET(WIX_EXECUTABLE)
| IF(WIX_VERSION_RESULT OR NOT WIX_VERSION) | ||
| MESSAGE(WARNING "Can't determine version of ${WIX_EXECUTABLE}, ignoring it") | ||
| UNSET(WIX_EXECUTABLE CACHE) | ||
| ENDIF() |
There was a problem hiding this comment.
Similarly, if the version of WIX_EXECUTABLE cannot be determined, unsetting only the cache variable WIX_EXECUTABLE leaves the local variable defined. This causes IF(NOT WIX_EXECUTABLE) on line 62 to evaluate to false, skipping the search for the legacy WiX v3 toolset and leading to build failures.
Unset both the cache and local variables here as well.
IF(WIX_VERSION_RESULT OR NOT WIX_VERSION)
MESSAGE(WARNING "Can't determine version of ${WIX_EXECUTABLE}, ignoring it")
UNSET(WIX_EXECUTABLE CACHE)
UNSET(WIX_EXECUTABLE)
ENDIF()
| #define Log(format, ...) \ | ||
| do{ \ | ||
| wchar_t log_buf[1024]; \ | ||
| _snwprintf_s(log_buf, _TRUNCATE, \ | ||
| L"MariaDB_CA %S:" L##format,__FUNCTION__, __VA_ARGS__); \ | ||
| WriteMsiLog(hInstall, log_buf); \ | ||
| } while (0) |
There was a problem hiding this comment.
The Log macro uses __VA_ARGS__ directly. When Log is called with only a format string and no additional arguments (e.g., Log("Initialized.");), the macro expansion leaves a trailing comma after __FUNCTION__ (e.g., _snwprintf_s(..., __FUNCTION__, )). This is a syntax error in standard C++ and will fail to compile under strict conforming preprocessors (such as MSVC with /Zc:preprocessor or /std:c++20).
Use the ##__VA_ARGS__ extension to safely suppress the trailing comma when no variadic arguments are provided.
#define Log(format, ...) \
do{ \
wchar_t log_buf[1024]; \
_snwprintf_s(log_buf, _TRUNCATE, \
L"MariaDB_CA %S:" L##format,__FUNCTION__, ##__VA_ARGS__); \
WriteMsiLog(hInstall, log_buf); \
} while (0)| MsiGetPropertyW(hInstall, L"INSTALLDIR", path, &len); | ||
| if (!IsDirectoryEmptyOrNonExisting(path)) |
There was a problem hiding this comment.
The return value of MsiGetPropertyW is not checked. If the function fails (for example, if the buffer is too small and it returns ERROR_MORE_DATA), path will be left uninitialized or incomplete, leading to undefined behavior when passed to IsDirectoryEmptyOrNonExisting.
Validate the return value of MsiGetPropertyW before proceeding.
if (MsiGetPropertyW(hInstall, L"INSTALLDIR", path, &len) != ERROR_SUCCESS)
{
return ERROR_INSTALL_FAILURE;
}
if (!IsDirectoryEmptyOrNonExisting(path))| bool empty; | ||
| wchar_t *path= 0; | ||
|
|
||
| MsiGetPropertyW(hInstall, L"DATADIR", datadir, &len); |
There was a problem hiding this comment.
The return value of MsiGetPropertyW is not checked. If it fails, datadir will be uninitialized, leading to undefined behavior in subsequent logging and directory checks.
Validate the return value and handle failure gracefully.
if (MsiGetPropertyW(hInstall, L"DATADIR", datadir, &len) != ERROR_SUCCESS)
{
er = ERROR_INSTALL_FAILURE;
goto LExit;
}| len= MAX_PATH; | ||
| MsiGetPropertyW(hInstall, L"INSTALLDIR", installdir, &len); |
There was a problem hiding this comment.
The return value of MsiGetPropertyW for INSTALLDIR is not checked. If it fails, installdir will be uninitialized, leading to undefined behavior in wcsicmp and subsequent path concatenations.
Validate the return value and handle failure gracefully.
len= MAX_PATH;
if (MsiGetPropertyW(hInstall, L"INSTALLDIR", installdir, &len) != ERROR_SUCCESS)
{
er = ERROR_INSTALL_FAILURE;
goto LExit;
}| servicename[0]= 0; | ||
| len= MAX_PATH; | ||
| MsiGetPropertyW(hInstall, L"SERVICENAME", servicename, &len); | ||
| datadir[0]= 0; | ||
| len= MAX_PATH; | ||
| MsiGetPropertyW(hInstall,L"DATADIR", datadir, &len); | ||
| bindir[0]= 0; | ||
| len= MAX_PATH; | ||
| MsiGetPropertyW(hInstall,L"INSTALLDIR", bindir, &len); | ||
| wcscat_s(bindir, L"\\bin"); |
There was a problem hiding this comment.
The return values of MsiGetPropertyW are not checked for SERVICENAME, DATADIR, and INSTALLDIR. If any of these calls fail, the buffers will contain uninitialized or incomplete data. Specifically, if MsiGetPropertyW for INSTALLDIR fails, calling wcscat_s on bindir can cause a crash or undefined behavior.
Validate all three return values before proceeding.
servicename[0]= 0;
len= MAX_PATH;
if (MsiGetPropertyW(hInstall, L"SERVICENAME", servicename, &len) != ERROR_SUCCESS)
{
er = ERROR_INSTALL_FAILURE;
goto LExit;
}
datadir[0]= 0;
len= MAX_PATH;
if (MsiGetPropertyW(hInstall, L"DATADIR", datadir, &len) != ERROR_SUCCESS)
{
er = ERROR_INSTALL_FAILURE;
goto LExit;
}
bindir[0]= 0;
len= MAX_PATH;
if (MsiGetPropertyW(hInstall, L"INSTALLDIR", bindir, &len) != ERROR_SUCCESS)
{
er = ERROR_INSTALL_FAILURE;
goto LExit;
}
wcscat_s(bindir, L"\\bin");| MsiSetProperty(hInstall,L"DATADIRERROR", L"data directory exists and is not empty"); | ||
| goto LExit; | ||
| } | ||
| WcaSetProperty(L"DATADIRERROR", L""); | ||
|
|
||
| MsiSetProperty(hInstall, L"DATADIRERROR", L""); |
There was a problem hiding this comment.
Inconsistent use of MsiSetProperty and MsiSetPropertyW. While MsiSetProperty is a macro that resolves to MsiSetPropertyW when UNICODE is defined, it is best practice to use the explicit wide-character version MsiSetPropertyW consistently throughout the file, especially since all passed string literals are wide (L"...").
MsiSetPropertyW(hInstall,L"DATADIRERROR", L"data directory exists and is not empty");
goto LExit;
}
MsiSetPropertyW(hInstall, L"DATADIRERROR", L"");| wchar_t name[MAX_PATH + 10]; | ||
| wcscpy_s(name, dir); | ||
| wcscat_s(name, L"\\*.err"); |
There was a problem hiding this comment.
Using wcscpy_s followed by wcscat_s is verbose and can trigger an abort if the combined path exceeds MAX_PATH + 10. Using swprintf_s is cleaner, safer, and formats the search pattern in a single operation.
wchar_t name[MAX_PATH + 32];
if (swprintf_s(name, L"%s\\*.err", dir) == -1)
{
return FALSE;
}There was a problem hiding this comment.
Pull request overview
Updates the Windows MSI packaging pipeline to support both modern WiX toolsets (v5+) via wix.exe and legacy WiX v3.x via candle.exe/light.exe, while keeping WXS sources authored in v3 format and converting them on-the-fly for modern builds. The PR also removes the dependency on the WiX SDK custom action utilities by switching custom actions to native MSI APIs, and adds CI coverage for MSI build/install/uninstall on Windows.
Changes:
- Add modern WiX (
wix.exe) discovery + build path with conversion and extension management; keep WiX v3 toolchain as a fallback/override. - Force per-machine install via
InstallScope='perMachine'and adjust related WXS properties/DirectoryRefs; replace VBScript failure injection with a native custom action. - Add a Windows x64 GitHub Actions workflow that builds, builds MSI, tests rollback, installs/uninstalls, and runs MTR.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| win/packaging/mysql_server.wxs.in | Forces per-machine scope via InstallScope. |
| win/packaging/extra.wxs.in | Removes ALLUSERS, fixes DirectoryRef placement, switches FakeFailure to a native CA, and wires rollback test condition. |
| win/packaging/create_msi.cmake | Adds modern wix.exe convert/build flow with extension management; keeps legacy candle/light path. |
| win/packaging/CMakeLists.txt | Detects modern WiX, adds WITH_WIX3 switch, and passes WiX executable/version into MSI target. |
| win/packaging/ca/CustomAction.def | Exports the new FakeFailure custom action entrypoint. |
| win/packaging/ca/CustomAction.cpp | Replaces WiX SDK logging/util usage with native MSI APIs; adds FakeFailure implementation. |
| win/packaging/ca/CMakeLists.txt | Removes linking against WiX SDK libraries now that wcautil/dutil are no longer used. |
| .github/workflows/windows-x64.yml | Adds CI job to build on Windows, build/test MSI, and run MTR. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #define Log(format, ...) \ | ||
| do{ \ | ||
| wchar_t log_buf[1024]; \ | ||
| _snwprintf_s(log_buf, _TRUNCATE, \ | ||
| L"MariaDB_CA %S:" L##format,__FUNCTION__, __VA_ARGS__); \ | ||
| WriteMsiLog(hInstall, log_buf); \ | ||
| } while (0) |
There was a problem hiding this comment.
This compiles as-is with MSVC (the only compiler this file targets): _snwprintf_s(buffer, _TRUNCATE, format, ...) matches the secure C++ template overload that deduces the buffer size from the array, and MSVC accepts the trailing comma when __VA_ARGS__ is empty. Log("Initialized.") builds and works.
| if (BufferPoolSizeLen == 0 || BufferPoolSizeLen > 15 || !BufferPoolSize[0]) | ||
| { | ||
| ErrorMsg= invalidValueMsg; | ||
| Log("Error: %s", invalidValueMsg); | ||
| goto LExit; | ||
| } |
There was a problem hiding this comment.
Fixed - restored setting ErrorMsg on this path, as before the refactoring.
| bindir[0]= 0; | ||
| len= MAX_PATH; | ||
| MsiGetPropertyW(hInstall,L"INSTALLDIR", bindir, &len); | ||
| wcscat_s(bindir, L"\\bin"); |
There was a problem hiding this comment.
Fixed - bindir is now [INSTALLDIR]bin\ like before the refactoring, relying on the trailing backslash in the MSI directory property.
| branches: | ||
| - 'main' | ||
| - 'bb-*' | ||
| - '[0-9]+.[0-9]+' |
There was a problem hiding this comment.
GitHub Actions filter patterns are not plain globs: per the filter pattern cheat sheet, + matches one or more of the preceding character, so [0-9]+.[0-9]+ does match 11.4. Same pattern is already in use in the windows-arm64 workflow.
666da53 to
ad00b4a
Compare
…rsions Custom actions : get rid of wcautil and dutil library dependencies They are not an official part of WiX distribution anymore. Replace the functionality with native MSI api, the Wix helpers we used were thin wrappers anyway. Also replace the VBScript FakeFailure custom action with a native one, VBScript is deprecated in Windows and scheduled for removal.
…rsions Support both modern WiX (wix.exe, v5/v6) and legacy v3.x toolsets. WXS sources stay in v3 format, the modern toolset builds them after on-the-fly 'wix convert'. Modern WiX is preferred when installed, use WITH_WIX3=1 to force legacy.
ad00b4a to
3d8e305
Compare
Support both modern WiX (wix.exe, v5/v6) and legacy v3.x toolsets.
WXS sources stay in v3 format; the modern toolset builds them after on-the-fly
wix convert. Modern WiX is preferred when installed, useWITH_WIX3=1to force legacy. Custom actions no longer need the WiX SDK (wcautil/dutil replaced with native MSI API), and the VBScript FakeFailure test action is replaced with a native one (VBScript is deprecated in Windows).Tested with WiX 3.14 and WiX 6.0.1, with and without HeidiSQL; MSI database tables compared for equivalence.