Skip to content

Commit 5c46ba1

Browse files
authored
Improve MOTW error handling in download flow (#6127)
#### DownloadFlow.cpp - Reorder workflow: RenameDownloadedInstaller now runs before UpdateInstallerFileMotwIfApplicable, so AES always scans the properly-named installer file. #### Downloader.cpp — RemoveMotwIfApplicable - Also handle ERROR_PATH_NOT_FOUND (in addition to ERROR_FILE_NOT_FOUND) from IPersistFile::Load — different Windows builds return different codes when no Zone.Identifier ADS is present. - Restore missing THROW_IF_FAILED(hr) to correctly surface any other Load failures rather than falling through to Remove()/Save() on an unloaded object. #### Downloader.cpp — ApplyMotwUsingIAttachmentExecuteIfApplicable - Wrap RemoveMotwIfApplicable in a try/catch inside the updateMotw lambda — log a warning but proceed to IAttachmentExecute::Save(). MOTW removal is best-effort; a removal failure should not abort the security scan. - Wrap the entire AES thread body in try/catch and log exceptions via AICLI_LOG, so unhandled errors are surfaced rather than silently leaving hr in an indeterminate state. Use LOG_IF_FAILED for CoInitializeEx so failures are captured through WIL. - Fix the return value: previously aesSaveResult was always returned, reporting S_OK when Save() was never reached (e.g. after CoInitializeEx failure). Now returns the thread's hr when aesSaveResult was never updated
1 parent f90155a commit 5c46ba1

2 files changed

Lines changed: 36 additions & 13 deletions

File tree

src/AppInstallerCLICore/Workflows/DownloadFlow.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -338,8 +338,8 @@ namespace AppInstaller::CLI::Workflow
338338

339339
context <<
340340
VerifyInstallerHash <<
341-
UpdateInstallerFileMotwIfApplicable <<
342-
RenameDownloadedInstaller;
341+
RenameDownloadedInstaller <<
342+
UpdateInstallerFileMotwIfApplicable;
343343

344344
if (installerDownloadOnly)
345345
{

src/AppInstallerCommonCore/Downloader.cpp

Lines changed: 34 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -481,16 +481,20 @@ namespace AppInstaller::Utility
481481
THROW_IF_FAILED(zoneIdentifier.As(&persistFile));
482482

483483
auto hr = persistFile->Load(filePath.c_str(), STGM_READ);
484-
if (hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
484+
if (hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) ||
485+
hr == HRESULT_FROM_WIN32(ERROR_PATH_NOT_FOUND))
485486
{
486-
// IPersistFile::Load returns same error for "file not found" and "motw not found".
487+
// IPersistFile::Load can return ERROR_FILE_NOT_FOUND or ERROR_PATH_NOT_FOUND for
488+
// both "file not found" and "motw not found" depending on the Windows build.
487489
// Check if the file exists to be sure we are on the "motw not found" case.
488490
THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND), !std::filesystem::exists(filePath));
489491

490492
AICLI_LOG(Core, Info, << "File does not contain motw. Skipped removing motw");
491493
return;
492494
}
493495

496+
THROW_IF_FAILED(hr);
497+
494498
THROW_IF_FAILED(zoneIdentifier->Remove());
495499
THROW_IF_FAILED(persistFile->Save(NULL, TRUE));
496500

@@ -516,8 +520,17 @@ namespace AppInstaller::Utility
516520
RETURN_IF_FAILED(attachmentExecute->SetLocalPath(filePath.c_str()));
517521
RETURN_IF_FAILED(attachmentExecute->SetSource(Utility::ConvertToUTF16(source).c_str()));
518522

519-
// IAttachmentExecute::Save() expects the local file to be clean(i.e. it won't clear existing motw if it thinks the source url is trusted)
520-
RemoveMotwIfApplicable(filePath);
523+
// IAttachmentExecute::Save() expects the local file to be clean (i.e. it won't clear existing motw if it thinks the source url is trusted).
524+
// If removal fails for any reason, log a warning and proceed — a removal failure should not abort the security check.
525+
try
526+
{
527+
RemoveMotwIfApplicable(filePath);
528+
}
529+
catch (...)
530+
{
531+
HRESULT hrRemoveMotw = wil::ResultFromCaughtException();
532+
AICLI_LOG(Core, Warning, << "RemoveMotwIfApplicable failed before IAttachmentExecute::Save(). Result: " << hrRemoveMotw << ". Proceeding with Save()");
533+
}
521534

522535
aesSaveResult = attachmentExecute->Save();
523536

@@ -536,21 +549,31 @@ namespace AppInstaller::Utility
536549

537550
std::thread aesThread([&]()
538551
{
539-
hr = CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED);
540-
if (FAILED(hr))
552+
try
541553
{
542-
return;
543-
}
554+
hr = LOG_IF_FAILED(CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED));
555+
if (FAILED(hr))
556+
{
557+
return;
558+
}
544559

545-
hr = updateMotw();
546-
CoUninitialize();
560+
hr = updateMotw();
561+
CoUninitialize();
562+
}
563+
catch (...)
564+
{
565+
hr = wil::ResultFromCaughtException();
566+
AICLI_LOG(Core, Error, << "Exception in IAttachmentExecute thread. Result: " << hr);
567+
}
547568
});
548569

549570
aesThread.join();
550571

551572
AICLI_LOG(Core, Info, << "Finished applying motw using IAttachmentExecute. Result: " << hr << " IAttachmentExecute::Save() result: " << aesSaveResult);
552573

553-
return aesSaveResult;
574+
// Return the thread's hr when aesSaveResult was never updated (e.g. CoInitializeEx failure or exception
575+
// before IAttachmentExecute::Save() was called), so the caller sees the real failure instead of S_OK.
576+
return (FAILED(hr) && aesSaveResult == S_OK) ? hr : aesSaveResult;
554577
}
555578

556579
Microsoft::WRL::ComPtr<IStream> GetReadOnlyStreamFromURI(std::string_view uriStr)

0 commit comments

Comments
 (0)