Many small code quality fixes

15 files changed, 73 insertions, 48 deletions

diff --git a/src/burn/engine/bootstrapperapplication.cpp b/src/burn/engine/bootstrapperapplication.cpp
index dc3bd5da..346c47a5 100644
--- a/src/burn/engine/bootstrapperapplication.cpp
+++ b/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.
diff --git a/src/burn/engine/burnpipe.cpp b/src/burn/engine/burnpipe.cpp
index ec85822e..102ccb4f 100644
--- a/src/burn/engine/burnpipe.cpp
+++ b/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.
diff --git a/src/burn/engine/cabextract.cpp b/src/burn/engine/cabextract.cpp
index 5663c3f7..2140130d 100644
--- a/src/burn/engine/cabextract.cpp
+++ b/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;
 
diff --git a/src/burn/engine/cache.cpp b/src/burn/engine/cache.cpp
index f5df6500..c85a1be4 100644
--- a/src/burn/engine/cache.cpp
+++ b/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));
 }
diff --git a/src/burn/engine/core.cpp b/src/burn/engine/core.cpp
index de202321..2dfa4857 100644
--- a/src/burn/engine/core.cpp
+++ b/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)
diff --git a/src/burn/engine/dependency.cpp b/src/burn/engine/dependency.cpp
index 6f80c21b..94a8a1e4 100644
--- a/src/burn/engine/dependency.cpp
+++ b/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;
         }
diff --git a/src/burn/engine/elevation.cpp b/src/burn/engine/elevation.cpp
index aff05ae3..acaa8276 100644
--- a/src/burn/engine/elevation.cpp
+++ b/src/burn/engine/elevation.cpp
@@ -2824,7 +2824,7 @@ LExit:
     // TODO: do the right thing here.
     //DependencyUninitializeRegistrationAction(&action);
     ReleaseStr(action.sczDependentProviderKey);
-    ReleaseStr(action.sczBundleCode)
+    ReleaseStr(action.sczBundleCode);
 
     return hr;
 }
diff --git a/src/burn/engine/exeengine.cpp b/src/burn/engine/exeengine.cpp
index 4df762da..c001e563 100644
--- a/src/burn/engine/exeengine.cpp
+++ b/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");
diff --git a/src/burn/engine/logging.cpp b/src/burn/engine/logging.cpp
index 51b546ad..52123499 100644
--- a/src/burn/engine/logging.cpp
+++ b/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;
         }
diff --git a/src/burn/engine/msiengine.cpp b/src/burn/engine/msiengine.cpp
index 48ec0c81..a1379054 100644
--- a/src/burn/engine/msiengine.cpp
+++ b/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);
diff --git a/src/burn/engine/msuengine.cpp b/src/burn/engine/msuengine.cpp
index 43f5f76c..d31691a5 100644
--- a/src/burn/engine/msuengine.cpp
+++ b/src/burn/engine/msuengine.cpp
@@ -313,6 +313,8 @@ LExit:
         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);
 
diff --git a/src/burn/engine/plan.cpp b/src/burn/engine/plan.cpp
index a6196e3c..9feb56ff 100644
--- a/src/burn/engine/plan.cpp
+++ b/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);
diff --git a/src/burn/engine/pseudobundle.cpp b/src/burn/engine/pseudobundle.cpp
index f0d67068..ff7f185c 100644
--- a/src/burn/engine/pseudobundle.cpp
+++ b/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;
 }
diff --git a/src/burn/engine/search.cpp b/src/burn/engine/search.cpp
index 1f128e95..a60215fe 100644
--- a/src/burn/engine/search.cpp
+++ b/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;
             }
diff --git a/src/burn/engine/variant.cpp b/src/burn/engine/variant.cpp
index 2267ee7b..3fc02aa8 100644
--- a/src/burn/engine/variant.cpp
+++ b/src/burn/engine/variant.cpp
@@ -10,6 +10,9 @@ static HRESULT GetVersionInternal(
     __in BOOL fSilent,
     __out VERUTIL_VERSION** ppValue
     );
+static void FreeVariantValue(
+    __in BURN_VARIANT* pVariant
+    );
 
 // function definitions
 
@@ -17,12 +20,7 @@ extern "C" void BVariantUninitialize(
     __in BURN_VARIANT* pVariant
     )
 {
-    if (BURN_VARIANT_TYPE_FORMATTED == pVariant->Type ||
-        BURN_VARIANT_TYPE_STRING == pVariant->Type)
-    {
-        StrSecureZeroFreeString(pVariant->sczValue);
-    }
-    SecureZeroMemory(pVariant, sizeof(BURN_VARIANT));
+    FreeVariantValue(pVariant);
 }
 
 extern "C" HRESULT BVariantGetNumeric(
@@ -164,12 +162,8 @@ extern "C" HRESULT BVariantSetNumeric(
 {
     HRESULT hr = S_OK;
 
-    if (BURN_VARIANT_TYPE_FORMATTED == pVariant->Type ||
-        BURN_VARIANT_TYPE_STRING == pVariant->Type)
-    {
-        StrSecureZeroFreeString(pVariant->sczValue);
-    }
-    memset(pVariant, 0, sizeof(BURN_VARIANT));
+    FreeVariantValue(pVariant);
+
     pVariant->llValue = llValue;
     pVariant->Type = BURN_VARIANT_TYPE_NUMERIC;
 
@@ -194,7 +188,7 @@ extern "C" HRESULT BVariantSetString(
         if (BURN_VARIANT_TYPE_FORMATTED != pVariant->Type &&
             BURN_VARIANT_TYPE_STRING != pVariant->Type)
         {
-            memset(pVariant, 0, sizeof(BURN_VARIANT));
+            FreeVariantValue(pVariant);
         }
 
         hr = StrAllocStringSecure(&pVariant->sczValue, wzValue, cchValue);
@@ -220,12 +214,8 @@ extern "C" HRESULT BVariantSetVersion(
     }
     else // assign the value.
     {
-        if (BURN_VARIANT_TYPE_FORMATTED == pVariant->Type ||
-            BURN_VARIANT_TYPE_STRING == pVariant->Type)
-        {
-            StrSecureZeroFreeString(pVariant->sczValue);
-        }
-        memset(pVariant, 0, sizeof(BURN_VARIANT));
+        FreeVariantValue(pVariant);
+
         hr = VerCopyVersion(pValue, &pVariant->pValue);
         pVariant->Type = BURN_VARIANT_TYPE_VERSION;
     }
@@ -319,3 +309,20 @@ extern "C" HRESULT BVariantChangeType(
 LExit:
     return hr;
 }
+
+static void FreeVariantValue(
+    __in BURN_VARIANT* pVariant
+    )
+{
+    if ((BURN_VARIANT_TYPE_FORMATTED == pVariant->Type || BURN_VARIANT_TYPE_STRING == pVariant->Type) && 
+        pVariant->sczValue)
+    {
+        StrSecureZeroFreeString(pVariant->sczValue);
+    }
+    else if (BURN_VARIANT_TYPE_VERSION == pVariant->Type && pVariant->pValue)
+    {
+        VerFreeVersion(pVariant->pValue);
+    }
+
+    SecureZeroMemory(pVariant, sizeof(BURN_VARIANT));
+}