diff --git a/base/check.cc b/base/check.cc index a91851e30eee2..b52283f84da2a 100644 --- a/base/check.cc +++ b/base/check.cc @@ -62,7 +62,8 @@ LogSeverity GetDumpSeverity() { // Crash outside official builds (outside user-facing builds) to detect // invariant violations early in release-build testing like fuzzing, etc. // These should eventually be migrated to fatal CHECKs. - return LOGGING_FATAL; + // [POC] Match OFFICIAL_BUILD behavior: in shipped Chrome this is non-fatal. + return LOGGING_ERROR; #endif } diff --git a/build/vs_toolchain.py b/build/vs_toolchain.py index 88489da743f6d..ef2ec5e69b41c 100755 --- a/build/vs_toolchain.py +++ b/build/vs_toolchain.py @@ -669,4 +669,5 @@ def main(): if __name__ == '__main__': - sys.exit(main()) + #sys.exit(main()) + pass diff --git a/components/download/internal/common/download_item_impl_unittest.cc b/components/download/internal/common/download_item_impl_unittest.cc index 7af843288b1dc..596afcf208b08 100644 --- a/components/download/internal/common/download_item_impl_unittest.cc +++ b/components/download/internal/common/download_item_impl_unittest.cc @@ -508,6 +508,90 @@ TEST_F(DownloadItemTest, NotificationAfterRemove) { ASSERT_TRUE(observer.download_removed()); } +// Regression POC: an observer that synchronously calls Remove() from inside +// OnDownloadUpdated() (as DangerousDownloadDialogBridge::Show() does on Android +// when window_android is null) causes a nested UpdateObservers() inside +// Remove() to clear is_updating_observers_. This lets the destructor's +// CHECK(!is_updating_observers_) pass while the OUTER UpdateObservers() is +// still on the stack, after which the outer call writes to freed `this` and +// OnTargetResolved() continues into MaybeCompleteDownload() on freed `this`. +TEST_F(DownloadItemTest, ObserverRemoveDuringTargetResolvedUAF) { + // Observer that mimics DownloadController/DangerousDownloadDialogBridge: + // when it sees the IN_PROGRESS notification carrying a dangerous danger_type + // it synchronously calls Remove() on the item. + class RemovingObserver : public DownloadItem::Observer { + public: + explicit RemovingObserver(DownloadItem* item) : item_(item) { + item_->AddObserver(this); + } + ~RemovingObserver() override { + if (item_) + item_->RemoveObserver(this); + } + void OnDownloadUpdated(DownloadItem* download) override { + // Fire exactly once on the OnTargetResolved() notification: state is + // IN_PROGRESS, target path is set, danger_type is DANGEROUS_FILE. The + // nested UpdateObservers() inside Remove() will re-enter here with state + // == CANCELLED, which we ignore. + if (fired_ || download->GetState() != DownloadItem::IN_PROGRESS || + download->GetTargetFilePath().empty() || !download->IsDangerous()) { + return; + } + fired_ = true; + download->Remove(); + // `download` (and `this->item_`) is now freed. Control returns to the + // outer UpdateObservers() loop, which then writes + // is_updating_observers_=false into freed memory and falls through to + // MaybeCompleteDownload(). + } + void OnDownloadDestroyed(DownloadItem* download) override { + download->RemoveObserver(this); + item_ = nullptr; + } + + private: + DownloadItem* item_; // Intentionally raw (cleared in OnDownloadDestroyed + // before operator delete) so BRP does not quarantine. + bool fired_ = false; + }; + + DownloadItemImpl* item = CreateDownloadItem(); + download::DownloadTargetCallback target_callback; + MockDownloadFile* download_file = + CallDownloadItemStart(item, &target_callback); + + RemovingObserver removing_observer(item); + + // ReleaseDownloadFile() during Remove() posts a Cancel() to the download + // task runner; it may or may not run before ASAN aborts. + EXPECT_CALL(*download_file, Cancel()).Times(AnyNumber()); + + // Mimic DownloadManagerImpl::DownloadRemoved(): synchronously erase the + // owning unique_ptr, running ~DownloadItemImpl() while the outer + // UpdateObservers() frame is still live. + EXPECT_CALL(*mock_delegate(), DownloadRemoved(item)) + .WillOnce(InvokeWithoutArgs([this, item] { DestroyDownloadItem(item); })); + + // Drive target determination so that OnTargetResolved() fires the outer + // UpdateObservers() with danger_type == DANGEROUS_FILE. + base::FilePath target_path(kDummyTargetPath); + base::FilePath intermediate_path(kDummyIntermediatePath); + auto task_runner = base::SingleThreadTaskRunner::GetCurrentDefault(); + SetRenameExpectation(download_file, task_runner, intermediate_path, + DOWNLOAD_INTERRUPT_REASON_NONE); + + download::DownloadTargetInfo target_info; + target_info.target_path = target_path; + target_info.intermediate_path = intermediate_path; + target_info.danger_type = DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE; + + std::move(target_callback).Run(std::move(target_info)); + // OnDownloadRenamedToIntermediateName -> OnTargetResolved runs here. + // ASAN: heap-use-after-free WRITE in DownloadItemImpl::UpdateObservers(), + // then heap-use-after-free READ in MaybeCompleteDownload(). + task_environment_.RunUntilIdle(); +} + TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) { // Setting to NOT_DANGEROUS does not trigger a notification. DownloadItemImpl* safe_item = CreateDownloadItem(); diff --git a/third_party/search_engines_data/resources_internal b/third_party/search_engines_data/resources_internal deleted file mode 160000 index d0158bfe38a17..0000000000000 --- a/third_party/search_engines_data/resources_internal +++ /dev/null @@ -1 +0,0 @@ -Subproject commit d0158bfe38a1759ead13e53ed0332aefdb0f512b