Many small code quality fixes

src/burn/engine/bootstrapperapplication.cpp

@@ -674,7 +674,7 @@ static HRESULT VerifyPipeSecret(
     hr = StrAlloc(&sczVerificationSecret, cbVerificationSecret / sizeof(WCHAR) + 1);
     ExitOnFailure(hr, "Failed to allocate buffer for bootstrapper application verification secret.");
 
-    FileReadHandle(hPipe, reinterpret_cast<LPBYTE>(sczVerificationSecret), cbVerificationSecret);
+    hr = FileReadHandle(hPipe, reinterpret_cast<LPBYTE>(sczVerificationSecret), cbVerificationSecret);
     ExitOnFailure(hr, "Failed to read verification secret from bootstrapper application pipe.");
 
     // Verify the secrets match.

src/burn/engine/burnpipe.cpp

@@ -446,11 +446,11 @@ extern "C" HRESULT BurnPipeChildConnect(
 
     // Try to connect to the parent.
     hr = PipeClientConnect(pConnection->sczName, &pConnection->hPipe);
-    ExitOnRootFailure(hr, "Failed to open parent pipe: %ls", sczPipeName)
+    ExitOnRootFailure(hr, "Failed to open parent pipe: %ls", pConnection->sczName)
 
     // Verify the parent and notify it that the child connected.
     hr = ChildPipeConnected(pConnection->hPipe, pConnection->sczSecret, &pConnection->dwProcessId);
-    ExitOnFailure(hr, "Failed to verify parent pipe: %ls", sczPipeName);
+    ExitOnFailure(hr, "Failed to verify parent pipe: %ls", pConnection->sczName);
 
     if (fCompanion)
     {
@@ -511,7 +511,7 @@ static HRESULT ChildPipeConnected(
     hr = StrAlloc(&sczVerificationSecret, cbVerificationSecret / sizeof(WCHAR) + 1);
     ExitOnFailure(hr, "Failed to allocate buffer for verification secret.");
 
-    FileReadHandle(hPipe, reinterpret_cast<LPBYTE>(sczVerificationSecret), cbVerificationSecret);
+    hr = FileReadHandle(hPipe, reinterpret_cast<LPBYTE>(sczVerificationSecret), cbVerificationSecret);
     ExitOnFailure(hr, "Failed to read verification secret from parent pipe.");
 
     // Verify the secrets match.

src/burn/engine/cabextract.cpp

@@ -833,9 +833,13 @@ static UINT FAR DIAMONDAPI CabWrite(
 
     case BURN_CAB_OPERATION_STREAM_TO_BUFFER:
         // copy to target buffer
-        memcpy_s(pContext->Cabinet.pbTargetBuffer + pContext->Cabinet.iTargetBuffer, pContext->Cabinet.cbTargetBuffer - pContext->Cabinet.iTargetBuffer, pv, cb);
-        pContext->Cabinet.iTargetBuffer += cb;
+        if (memcpy_s(pContext->Cabinet.pbTargetBuffer + pContext->Cabinet.iTargetBuffer, pContext->Cabinet.cbTargetBuffer - pContext->Cabinet.iTargetBuffer, pv, cb))
+        {
+            hr = E_INSUFFICIENT_BUFFER;
+            ExitOnRootFailure(hr, "Failed to copy data to target buffer during cabinet extraction.");
+        }
 
+        pContext->Cabinet.iTargetBuffer += cb;
         cbWrite = cb;
         break;
 

src/burn/engine/cache.cpp

@@ -363,6 +363,8 @@ extern "C" HRESULT CacheEnsureBaseWorkingFolder(
             }
 
             pWorkingFolderAcl = reinterpret_cast<LPSECURITY_ATTRIBUTES>(MemAlloc(sizeof(SECURITY_ATTRIBUTES), TRUE));
+            ExitOnNull(pWorkingFolderAcl, hr, E_OUTOFMEMORY, "Failed to allocate security attributes.");
+
             pWorkingFolderAcl->nLength = sizeof(SECURITY_ATTRIBUTES);
             pWorkingFolderAcl->lpSecurityDescriptor = psd;
             pWorkingFolderAcl->bInheritHandle = FALSE;
@@ -857,6 +859,7 @@ extern "C" HRESULT CacheSendProgressCallback(
         case PROGRESS_STOP:
             hr = HRESULT_FROM_WIN32(ERROR_INSTALL_USEREXIT);
             ExitOnRootFailure(hr, "UX aborted on download progress.");
+            break;
 
         case PROGRESS_QUIET: // Not actually an error, just an indication to the caller to stop requesting progress.
             pCallback->pfnProgress = NULL;
@@ -866,6 +869,7 @@ extern "C" HRESULT CacheSendProgressCallback(
         default:
             hr = E_UNEXPECTED;
             ExitOnRootFailure(hr, "Invalid return code from progress routine.");
+            break;
         }
     }
 
@@ -1435,8 +1439,8 @@ extern "C" void CacheUninitialize(
     ReleaseStr(pCache->sczBaseWorkingFolder);
     ReleaseStr(pCache->sczAcquisitionFolder);
     ReleaseStr(pCache->sczSourceProcessFolder);
-    ReleaseStr(pCache->sczBundleEngineWorkingPath)
-    ReleaseFileHandle(pCache->hBundleEngineWorkingFile)
+    ReleaseStr(pCache->sczBundleEngineWorkingPath);
+    ReleaseFileHandle(pCache->hBundleEngineWorkingFile);
 
     memset(pCache, 0, sizeof(BURN_CACHE));
 }

src/burn/engine/core.cpp

@@ -1232,8 +1232,11 @@ HRESULT CoreAppendLogToCommandLine(
     hr = StrAllocConcat(psczCommandLine, szLogArgFormatted, 0);
     ExitOnFailure(hr, "Failed concatenating '-log' to command line");
 
-    hr = StrAllocConcat(psczObfuscatedCommandLine, szLogArgFormatted, 0);
-    ExitOnFailure(hr, "Failed concatenating '-log' to obfuscated command line");
+    if (psczObfuscatedCommandLine)
+    {
+        hr = StrAllocConcat(psczObfuscatedCommandLine, szLogArgFormatted, 0);
+        ExitOnFailure(hr, "Failed concatenating '-log' to obfuscated command line");
+    }
 
 LExit:
     if (rgszArgs)

src/burn/engine/dependency.cpp

@@ -601,8 +601,8 @@ extern "C" HRESULT DependencyPlanPackageBegin(
             pProvider->dependentExecute = BURN_DEPENDENCY_ACTION_NONE;
         }
 
-        if (BURN_DEPENDENCY_ACTION_UNREGISTER == pProvider->dependentRollback && pProvider->fBundleRegisteredAsDependent ||
-            BURN_DEPENDENCY_ACTION_REGISTER == pProvider->dependentRollback && !pProvider->fBundleRegisteredAsDependent)
+        if ((BURN_DEPENDENCY_ACTION_UNREGISTER == pProvider->dependentRollback && pProvider->fBundleRegisteredAsDependent) ||
+            (BURN_DEPENDENCY_ACTION_REGISTER == pProvider->dependentRollback && !pProvider->fBundleRegisteredAsDependent))
         {
             pProvider->dependentRollback = BURN_DEPENDENCY_ACTION_NONE;
         }

src/burn/engine/elevation.cpp

@@ -1452,9 +1452,9 @@ extern "C" HRESULT ElevationExecutePackageDependencyAction(
     ExitOnFailure(hr, "Failed to write bundle dependency key to message buffer.");
 
     // Dependent actions.
-    for (DWORD i = 0; i < pExecuteAction->packageProvider.pPackage->cDependencyProviders; ++i)
+    for (DWORD i = 0; i < pExecuteAction->packageDependency.pPackage->cDependencyProviders; ++i)
     {
-        BURN_DEPENDENCY_PROVIDER* pProvider = pExecuteAction->packageProvider.pPackage->rgDependencyProviders + i;
+        BURN_DEPENDENCY_PROVIDER* pProvider = pExecuteAction->packageDependency.pPackage->rgDependencyProviders + i;
         BURN_DEPENDENCY_ACTION* pAction = fRollback ? &pProvider->dependentRollback : &pProvider->dependentExecute;
         hr = BuffWriteNumber(&pbData, &cbData, (DWORD)*pAction);
         ExitOnFailure(hr, "Failed to write dependent action to message buffer.");
@@ -2824,7 +2824,7 @@ static HRESULT OnProcessDependentRegistration(
     // TODO: do the right thing here.
     //DependencyUninitializeRegistrationAction(&action);
     ReleaseStr(action.sczDependentProviderKey);
-    ReleaseStr(action.sczBundleCode)
+    ReleaseStr(action.sczBundleCode);
 
     return hr;
 }
@@ -3486,9 +3486,9 @@ static HRESULT OnExecutePackageDependencyAction(
     ExitOnFailure(hr, "Failed to read bundle dependency key from message buffer.");
 
     // Read dependent actions.
-    for (DWORD i = 0; i < executeAction.packageProvider.pPackage->cDependencyProviders; ++i)
+    for (DWORD i = 0; i < executeAction.packageDependency.pPackage->cDependencyProviders; ++i)
     {
-        BURN_DEPENDENCY_PROVIDER* pProvider = executeAction.packageProvider.pPackage->rgDependencyProviders + i;
+        BURN_DEPENDENCY_PROVIDER* pProvider = executeAction.packageDependency.pPackage->rgDependencyProviders + i;
         BURN_DEPENDENCY_ACTION* pAction = fRollback ? &pProvider->dependentRollback : &pProvider->dependentExecute;
         hr = BuffReadNumber(pbData, cbData, &iData, (DWORD*)pAction);
         ExitOnFailure(hr, "Failed to read dependent action.");

src/burn/engine/exeengine.cpp

@@ -449,7 +449,7 @@ extern "C" HRESULT ExeEngineExecutePackage(
 
     if (BURN_EXE_DETECTION_TYPE_ARP == pPackage->Exe.detectionType &&
         (BOOTSTRAPPER_ACTION_STATE_UNINSTALL == pExecuteAction->exePackage.action ||
-        BOOTSTRAPPER_ACTION_STATE_INSTALL == pExecuteAction->exePackage.action && fRollback))
+         (BOOTSTRAPPER_ACTION_STATE_INSTALL == pExecuteAction->exePackage.action && fRollback)))
     {
         hr = DetectArpEntry(pPackage, &applyState, &sczArpUninstallString);
         ExitOnFailure(hr, "Failed to query ArpEntry for %hs.", BOOTSTRAPPER_ACTION_STATE_UNINSTALL == pExecuteAction->exePackage.action ? "uninstall" : "install");

src/burn/engine/logging.cpp

@@ -322,15 +322,17 @@ extern "C" HRESULT LoggingSetPackageVariable(
         ExitFunction();
     }
 
-    // For burn packages we'll add logging even it it wasn't explictly specified
+    // For burn packages we'll add logging even it it wasn't explictly specified.
     if (BURN_PACKAGE_TYPE_BUNDLE == pPackage->type || (BURN_PACKAGE_TYPE_EXE == pPackage->type && BURN_EXE_PROTOCOL_TYPE_BURN == pPackage->Exe.protocol))
     {
         if (!fRollback && (!pPackage->sczLogPathVariable || !*pPackage->sczLogPathVariable))
         {
+            // Best effort, no need to fail if we can't set the logging path.
             StrAllocFormatted(&pPackage->sczLogPathVariable, L"WixBundleLog_%ls", pPackage->sczId);
         }
         else if (fRollback && (!pPackage->sczRollbackLogPathVariable || !*pPackage->sczRollbackLogPathVariable))
         {
+            // Best effort, no need to fail if we can't set the logging path.
             StrAllocFormatted(&pPackage->sczRollbackLogPathVariable, L"WixBundleRollbackLog_%ls", pPackage->sczId);
         }
     }
@@ -1052,7 +1054,8 @@ static HRESULT GetNonSessionSpecificTempFolder(
         hr = ::StringCchLengthW(sczSessionId, STRSAFE_MAX_CCH, reinterpret_cast<size_t*>(&cchSessionId));
         ExitOnFailure(hr, "Failed to get length of session id string.");
 
-        if (CSTR_EQUAL == ::CompareStringOrdinal(sczTempFolder + cchTempFolder - cchSessionId, static_cast<DWORD>(cchSessionId), sczSessionId, static_cast<DWORD>(cchSessionId), FALSE))
+        if (cchTempFolder >= cchSessionId &&
+            CSTR_EQUAL == ::CompareStringOrdinal(sczTempFolder + cchTempFolder - cchSessionId, static_cast<DWORD>(cchSessionId), sczSessionId, static_cast<DWORD>(cchSessionId), FALSE))
         {
             cchTempFolder -= cchSessionId;
         }

src/burn/engine/msiengine.cpp

@@ -2158,7 +2158,7 @@ static HRESULT ConcatFeatureActionProperties(
 
     if (sczAddLocal)
     {
-        hr = StrAllocFormatted(&scz, L" ADDLOCAL=\"%s\"", sczAddLocal, 0);
+        hr = StrAllocFormatted(&scz, L" ADDLOCAL=\"%s\"", sczAddLocal);
         ExitOnFailure(hr, "Failed to format ADDLOCAL string.");
 
         hr = StrAllocConcatSecure(psczArguments, scz, 0);
@@ -2167,7 +2167,7 @@ static HRESULT ConcatFeatureActionProperties(
 
     if (sczAddSource)
     {
-        hr = StrAllocFormatted(&scz, L" ADDSOURCE=\"%s\"", sczAddSource, 0);
+        hr = StrAllocFormatted(&scz, L" ADDSOURCE=\"%s\"", sczAddSource);
         ExitOnFailure(hr, "Failed to format ADDSOURCE string.");
 
         hr = StrAllocConcatSecure(psczArguments, scz, 0);
@@ -2176,7 +2176,7 @@ static HRESULT ConcatFeatureActionProperties(
 
     if (sczAddDefault)
     {
-        hr = StrAllocFormatted(&scz, L" ADDDEFAULT=\"%s\"", sczAddDefault, 0);
+        hr = StrAllocFormatted(&scz, L" ADDDEFAULT=\"%s\"", sczAddDefault);
         ExitOnFailure(hr, "Failed to format ADDDEFAULT string.");
 
         hr = StrAllocConcatSecure(psczArguments, scz, 0);
@@ -2185,7 +2185,7 @@ static HRESULT ConcatFeatureActionProperties(
 
     if (sczReinstall)
     {
-        hr = StrAllocFormatted(&scz, L" REINSTALL=\"%s\"", sczReinstall, 0);
+        hr = StrAllocFormatted(&scz, L" REINSTALL=\"%s\"", sczReinstall);
         ExitOnFailure(hr, "Failed to format REINSTALL string.");
 
         hr = StrAllocConcatSecure(psczArguments, scz, 0);
@@ -2194,7 +2194,7 @@ static HRESULT ConcatFeatureActionProperties(
 
     if (sczAdvertise)
     {
-        hr = StrAllocFormatted(&scz, L" ADVERTISE=\"%s\"", sczAdvertise, 0);
+        hr = StrAllocFormatted(&scz, L" ADVERTISE=\"%s\"", sczAdvertise);
         ExitOnFailure(hr, "Failed to format ADVERTISE string.");
 
         hr = StrAllocConcatSecure(psczArguments, scz, 0);
@@ -2203,7 +2203,7 @@ static HRESULT ConcatFeatureActionProperties(
 
     if (sczRemove)
     {
-        hr = StrAllocFormatted(&scz, L" REMOVE=\"%s\"", sczRemove, 0);
+        hr = StrAllocFormatted(&scz, L" REMOVE=\"%s\"", sczRemove);
         ExitOnFailure(hr, "Failed to format REMOVE string.");
 
         hr = StrAllocConcatSecure(psczArguments, scz, 0);

src/burn/engine/msuengine.cpp

@@ -313,6 +313,8 @@ extern "C" HRESULT MsuEngineExecutePackage(
         SetServiceStartType(schWu, SERVICE_DISABLED);
     }
 
+    ReleaseServiceHandle(schWu);
+
     // Best effort to clear the execute package cache folder variable.
     VariableSetString(pVariables, BURN_BUNDLE_EXECUTE_PACKAGE_CACHE_FOLDER, NULL, TRUE, FALSE);
 

src/burn/engine/plan.cpp

@@ -1542,7 +1542,7 @@ extern "C" HRESULT PlanRelatedBundlesComplete(
 
         if (fBundle && BOOTSTRAPPER_ACTION_STATE_NONE != packageAction)
         {
-            if (pPackage->cDependencyProviders)
+            if (pPackage && pPackage->cDependencyProviders)
             {
                 // Bundles only support a single provider key.
                 const BURN_DEPENDENCY_PROVIDER* pProvider = pPackage->rgDependencyProviders;
@@ -2015,6 +2015,7 @@ extern "C" HRESULT PlanRollbackBoundaryComplete(
 
     // Add checkpoints.
     hr = PlanExecuteCheckpoint(pPlan);
+    ExitOnFailure(hr, "Failed to append execute checkpoint for rollback boundary complete.");
 
     // Add complete rollback boundary to execute plan.
     hr = PlanAppendExecuteAction(pPlan, &pExecuteAction);
@@ -2950,9 +2951,9 @@ static void ExecuteActionLog(
 
     case BURN_EXECUTE_ACTION_TYPE_PACKAGE_DEPENDENCY:
         LogStringLine(PlanDumpLevel, "%ls action[%u]: PACKAGE_DEPENDENCY package id: %ls, bundle provider key: %ls", wzBase, iAction, pAction->packageDependency.pPackage->sczId, pAction->packageDependency.sczBundleProviderKey);
-        for (DWORD j = 0; j < pAction->packageProvider.pPackage->cDependencyProviders; ++j)
+        for (DWORD j = 0; j < pAction->packageDependency.pPackage->cDependencyProviders; ++j)
         {
-            const BURN_DEPENDENCY_PROVIDER* pProvider = pAction->packageProvider.pPackage->rgDependencyProviders + j;
+            const BURN_DEPENDENCY_PROVIDER* pProvider = pAction->packageDependency.pPackage->rgDependencyProviders + j;
             LogStringLine(PlanDumpLevel, "      Provider[%u]: key: %ls, action: %hs", j, pProvider->sczKey, LoggingDependencyActionToString(fRollback ? pProvider->dependentRollback : pProvider->dependentExecute));
         }
         break;

src/burn/engine/pseudobundle.cpp

@@ -127,8 +127,8 @@ extern "C" HRESULT PseudoBundleInitializePassthrough(
     ExitOnFailure(hr, "Failed to copy cache id for passthrough pseudo bundle.");
 
     // Log variables - best effort
-    StrAllocFormatted(&pPackage->sczLogPathVariable, L"WixBundleLog_%ls", pPackage->sczId);
-    StrAllocFormatted(&pPackage->sczRollbackLogPathVariable, L"WixBundleRollbackLog_%ls", pPackage->sczId);
+    StrAllocFormatted(&pPassthroughPackage->sczLogPathVariable, L"WixBundleLog_%ls", pPackage->sczId);
+    StrAllocFormatted(&pPassthroughPackage->sczRollbackLogPathVariable, L"WixBundleRollbackLog_%ls", pPackage->sczId);
 
     hr = CoreCreatePassthroughBundleCommandLine(&sczArguments, pInternalCommand, pCommand);
     ExitOnFailure(hr, "Failed to create command-line arguments.");
@@ -155,6 +155,8 @@ extern "C" HRESULT PseudoBundleInitializeUpdateBundle(
 {
     HRESULT hr = S_OK;
     BURN_PAYLOAD* pPayload = NULL;
+    BYTE* rgbHash = NULL;
+    DWORD cbHash = 0;
 
     // Initialize the single payload, and fill out all the necessary fields
     pPackage->payloads.rgItems = (BURN_PAYLOAD_GROUP_ITEM*)MemAlloc(sizeof(BURN_PAYLOAD_GROUP_ITEM), TRUE);
@@ -185,9 +187,6 @@ extern "C" HRESULT PseudoBundleInitializeUpdateBundle(
 
     if (wzHash && *wzHash)
     {
-        BYTE* rgbHash = NULL;
-        DWORD cbHash = 0;
-
         hr = StrAllocHexDecode(wzHash, &rgbHash, &cbHash);
         ExitOnFailure(hr, "Failed to decode hash string: %ls.", wzHash);
 
@@ -223,5 +222,7 @@ extern "C" HRESULT PseudoBundleInitializeUpdateBundle(
     ExitOnFailure(hr, "Failed to copy install arguments for update bundle package");
 
 LExit:
+    ReleaseStr(rgbHash);
+
     return hr;
 }

src/burn/engine/search.cpp

@@ -334,7 +334,7 @@ extern "C" HRESULT SearchesParseFromXml(
             {
                 pSearch->MsiProductSearch.Type = BURN_MSI_PRODUCT_SEARCH_TYPE_ASSIGNMENT;
             }
-            else if (CSTR_EQUAL == ::CompareStringW(LOCALE_INVARIANT, 0, scz, -1, L"exists", -1))
+            else if (CSTR_EQUAL == ::CompareStringOrdinal(scz, -1, L"exists", -1, FALSE))
             {
                 pSearch->MsiProductSearch.Type = BURN_MSI_PRODUCT_SEARCH_TYPE_EXISTS;
             }