From 7c3c2cad3c5e95fac151debc89c2f5629b4c6b21 Mon Sep 17 00:00:00 2001 From: Rob Mensching Date: Fri, 30 Jan 2026 15:18:43 -0800 Subject: Many small code quality fixes --- src/burn/engine/bootstrapperapplication.cpp | 2 +- src/burn/engine/burnpipe.cpp | 6 ++-- src/burn/engine/cabextract.cpp | 8 +++-- src/burn/engine/cache.cpp | 8 +++-- src/burn/engine/core.cpp | 7 +++-- src/burn/engine/dependency.cpp | 4 +-- src/burn/engine/elevation.cpp | 2 +- src/burn/engine/exeengine.cpp | 2 +- src/burn/engine/logging.cpp | 7 +++-- src/burn/engine/msiengine.cpp | 12 ++++---- src/burn/engine/msuengine.cpp | 2 ++ src/burn/engine/plan.cpp | 3 +- src/burn/engine/pseudobundle.cpp | 11 +++---- src/burn/engine/search.cpp | 2 +- src/burn/engine/variant.cpp | 45 +++++++++++++++++------------ 15 files changed, 73 insertions(+), 48 deletions(-) (limited to 'src') 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(sczVerificationSecret), cbVerificationSecret); + hr = FileReadHandle(hPipe, reinterpret_cast(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(sczVerificationSecret), cbVerificationSecret); + hr = FileReadHandle(hPipe, reinterpret_cast(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(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(&cchSessionId)); ExitOnFailure(hr, "Failed to get length of session id string."); - if (CSTR_EQUAL == ::CompareStringOrdinal(sczTempFolder + cchTempFolder - cchSessionId, static_cast(cchSessionId), sczSessionId, static_cast(cchSessionId), FALSE)) + if (cchTempFolder >= cchSessionId && + CSTR_EQUAL == ::CompareStringOrdinal(sczTempFolder + cchTempFolder - cchSessionId, static_cast(cchSessionId), sczSessionId, static_cast(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)); +} -- cgit v1.2.3-55-g6feb