wix - Failed to get MSI property in UPGRADINGPRODUCTCODE, WIX_UPGRADE_DETECTED

Question

I wanted to skip some of my custom actions during upgrade, uninstallDriver, to achieve this I have tried to retrieve property WIX_UPGRADE_DETECTED and UPGRADINGPRODUCTCODE, but both of them are not set.

Sample code:

UninstallDriver(MSIHANDLE hInstall)  
{
   char szBuff[1024]; DWORD dwValue = 0;

   OutputDebugStringA("UninstallDriver");
   MsiGetPropertyA(hInstall, "UPGRADINGPRODUCTCODE", szBuff, &dwValue);
   OutputDebugStringA("UPGRADINGPRODUCTCODE");OutputDebugStringA(szBuff);

   MsiGetPropertyA(hInstall, "WIX_UPGRADE_DETECTED", szBuff, &dwValue);
   OutputDebugStringA("WIX_UPGRADE_DETECTED");OutputDebugStringA(szBuff);

   result = UninstallDriver();

   return result;
}

And my customAction is,

<Custom Action='UninstallMYDriverAction'
 After='InstallInitialize'>
        REMOVE~="ALL" OR REINSTALL</Custom>

Answer 1

Ignoring the debug strings, it's easier to see that the buffer handling is incorrect. I would suggest also outputting the return values from MsiGetPropertyA() and the value in dwValue to confirm, but here's what I think is happening (comments refer to dwValue):

char szBuff[1024]; DWORD dwValue = 0;
MsiGetPropertyA(hInstall, "UPGRADINGPRODUCTCODE", szBuff, &dwValue); // passes 0, updated to ?x?
MsiGetPropertyA(hInstall, "WIX_UPGRADE_DETECTED", szBuff, &dwValue); // passes ?x?, updated to ?y?

When requesting UPGRADINGPRODUCTCODE property with a claimed buffer length of zero, the fetch will never succeed as it must always accept at least a null character. Thus this will return ERROR_MORE_DATA and set dwValue to the length excluding a null character (?x?).

Then it will request the value of WIX_UPGRADE_DETECTED with a claimed buffer length of (?x?). If the new length (?y?) is less than the old length (?x?) you'll get its contents in your buffer; otherwise it will also just query the length of this new property.

Since WIX_UPGRADE_DETECTED contains a list of one or more GUIDs, and UPGRADINGPRODUCTCODE only contains one, and this code never increments dwValue to account for the null, it will only possibly succeed if the latter is ?y? is 0 (empty) and ?x? is non-empty. But note that this second call passed an unverified value as the length of your buffer, this pattern is a buffer overflow waiting to happen.

So fix your buffer handling. The pattern I like to use (below) is similar to what Stein describes, but avoids the second call if I know a good default size for the buffer. In your case, it sounds like you're happy with the 1024 element buffer, but do consider if you ever need to handle more than 1024 / len(GUID) related upgrade codes.

(My guess is you're fine. But at least think it through. And even though GUIDs are ASCII so the contents won't matter, please please please build UNICODE these days...)

WCHAR szBuf[1024];
DWORD cchBuf = 1024; // or _countof(szBuf);
DWORD dwErr = MsiGetPropertyW(hInstall, L"UPGRADINGPRODUCTCODE", szBuf, &cchBuf);
if (dwErr != ERROR_MORE_DATA) {
    // exercise: increment cchBuf for null, adjust buffer, call MsiGetPropertyW again
}
if (dwErr != ERROR_SUCCESS) {
    // per https://docs.microsoft.com/en-us/windows/desktop/msi/custom-action-return-values
    return ERROR_INSTALL_FAILURE;
}

// reset buffer length for next call, in case second property is longer than first
cchBuf = 1024;
dwErr = MsiGetPropertyW(hInstall, L"WIX_UPGRADE_DETECTED", szBuf, &cchBuf);
// : : :