Fix WcaErrorMessage cArgs==-1 case.

- Count args before creating message record. - Document terminating NULL requirement. - Add terminating NULL in MessageExit* macros. - Enhance tests for problems encountered fixing this nightmare. Fixes wixtoolset/issues#7422. Fixes wixtoolset/issues#7444.
wixtoolset · Jun 3, 2023 · fe57e71 · fe57e71
1 parent d5cca4a
commit fe57e71
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 17 deletions.
diff --git a/src/libs/wcautil/WixToolset.WcaUtil/inc/wcautil.h b/src/libs/wcautil/WixToolset.WcaUtil/inc/wcautil.h
@@ -15,9 +15,9 @@ extern "C" {
 
 #include "dutil.h"
 
-#define MessageExitOnLastErrorSource(d, x, e, s, ...)      { x = ::GetLastError(); x = HRESULT_FROM_WIN32(x); if (FAILED(x)) { ExitTraceSource(d, x, s, __VA_ARGS__); WcaErrorMessage(e, x, MB_OK, -1, __VA_ARGS__);  goto LExit; } }
-#define MessageExitOnFailureSource(d, x, e, s, ...)           if (FAILED(x)) { ExitTraceSource(d, x, s, __VA_ARGS__); WcaErrorMessage(e, x, INSTALLMESSAGE_ERROR | MB_OK, -1, __VA_ARGS__);  goto LExit; }
-#define MessageExitOnNullWithLastErrorSource(d, p, x, e, s, ...) if (NULL == p) { x = ::GetLastError(); x = HRESULT_FROM_WIN32(x); if (!FAILED(x)) { x = E_FAIL; } ExitTraceSource(d, x, s, __VA_ARGS__); WcaErrorMessage(e, x, MB_OK, -1, __VA_ARGS__);  goto LExit; }
+#define MessageExitOnLastErrorSource(d, x, e, s, ...)      { x = ::GetLastError(); x = HRESULT_FROM_WIN32(x); if (FAILED(x)) { ExitTraceSource(d, x, s, __VA_ARGS__); WcaErrorMessage(e, x, MB_OK, -1, __VA_ARGS__, NULL);  goto LExit; } }
+#define MessageExitOnFailureSource(d, x, e, s, ...)           if (FAILED(x)) { ExitTraceSource(d, x, s, __VA_ARGS__); WcaErrorMessage(e, x, INSTALLMESSAGE_ERROR | MB_OK, -1, __VA_ARGS__, NULL);  goto LExit; }
+#define MessageExitOnNullWithLastErrorSource(d, p, x, e, s, ...) if (NULL == p) { x = ::GetLastError(); x = HRESULT_FROM_WIN32(x); if (!FAILED(x)) { x = E_FAIL; } ExitTraceSource(d, x, s, __VA_ARGS__); WcaErrorMessage(e, x, MB_OK, -1, __VA_ARGS__, NULL);  goto LExit; }
 
 #define MessageExitOnLastError(x, e, s, ...) MessageExitOnLastErrorSource(DUTIL_SOURCE_DEFAULT, x, e, s, __VA_ARGS__)
 #define MessageExitOnFailure(x, e, s, ...) MessageExitOnFailureSource(DUTIL_SOURCE_DEFAULT, x, e, s, __VA_ARGS__)

diff --git a/src/libs/wcautil/WixToolset.WcaUtil/wcawrap.cpp b/src/libs/wcautil/WixToolset.WcaUtil/wcawrap.cpp
@@ -27,7 +27,9 @@ WcaErrorMessage() - sends an error message from the CustomAction using
 the Error table
 
 NOTE: Any and all var_args (...) must be WCHAR*
-      If you pass -1 to cArgs the count will be determined
+      If you pass -1 to cArgs, the count will be determined by
+      looking for a trailing NULL argment. If you omit a terminating
+      NULL, the results are undefined and probably crashy.
 ********************************************************************/
 extern "C" UINT __cdecl WcaErrorMessage(
     __in int iError,
@@ -41,6 +43,22 @@ extern "C" UINT __cdecl WcaErrorMessage(
     MSIHANDLE hRec = NULL;
     va_list args = NULL;
 
+    if (-1 == cArgs)
+    {
+        LPCWSTR wzArg = NULL;
+        va_list iter = NULL;
+
+        va_start(iter, cArgs);
+        cArgs = 0;
+
+        while (NULL != (wzArg = va_arg(iter, WCHAR*)) && L'\0' != *wzArg)
+        {
+            ++cArgs;
+        }
+
+        va_end(iter);
+    }
+
     uiType |= INSTALLMESSAGE_ERROR;  // ensure error type is set
     hRec = ::MsiCreateRecord(cArgs + 2);
     if (!hRec)
@@ -56,18 +74,6 @@ extern "C" UINT __cdecl WcaErrorMessage(
     ExitOnFailure(HRESULT_FROM_WIN32(er), "failed to set hresult code into error message");
 
     va_start(args, cArgs);
-    if (-1 == cArgs)
-    {
-        LPCWSTR wzArg = NULL;
-        va_list iter = args;
-        cArgs = 0;
-
-        while (NULL != (wzArg = va_arg(iter, WCHAR*)) && L'\0' != *wzArg)
-        {
-            ++cArgs;
-        }
-    }
-
     for (INT i = 0; i < cArgs; i++)
     {
         er = ::MsiRecordSetStringW(hRec, i + 3, va_arg(args, WCHAR*));
@@ -76,6 +82,7 @@ extern "C" UINT __cdecl WcaErrorMessage(
     va_end(args);
 
     er = WcaProcessMessage(static_cast<INSTALLMESSAGE>(uiType), hRec);
+
 LExit:
     if (args)
     {

diff --git a/src/test/burn/WixTestTools/MSIExec.cs b/src/test/burn/WixTestTools/MSIExec.cs
@@ -111,7 +111,7 @@ public void SetDefaultArguments()
             this.ForceRestart = false;
             this.PromptRestart = false;
             this.LogFile = String.Empty;
-            this.LoggingOptions = MSIExecLoggingOptions.VOICEWARMUP;
+            this.LoggingOptions = MSIExecLoggingOptions.Log_All_Information | MSIExecLoggingOptions.Verbose_Output | MSIExecLoggingOptions.Extra_Debugging_Information; // `/l*vx`
             this.OtherArguments = String.Empty;
         }
 

diff --git a/src/test/msi/TestData/UtilExtensionUserTests/ProductNonVitalUserGroup/NonVitalUserGroup.wxs b/src/test/msi/TestData/UtilExtensionUserTests/ProductNonVitalUserGroup/NonVitalUserGroup.wxs
@@ -10,12 +10,14 @@
 
     <Fragment>
         <util:Group Id="ShouldNotExist" Name="Should Not Exist" />
+        <util:Group Id="AlsoShouldNotExist" Name="Also Should Not Exist" />
 
         <Component Id="Component1" Guid="00030829-0000-0000-C000-000000000046" Directory="INSTALLFOLDER">
             <File Source="$(sys.SOURCEFILEPATH)" KeyPath="yes" />
 
             <util:User Id="CurrentUser" Name="[LogonUser]" Domain="[%USERDOMAIN]" RemoveOnUninstall="no" Vital="no">
                 <util:GroupRef Id="ShouldNotExist" />
+                <util:GroupRef Id="AlsoShouldNotExist" />
             </util:User>
         </Component>
     </Fragment>