New issue
Advanced search Search tips

Issue 921148 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Jan 18
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[Debug] Chromium fails to analyze DMG features on macos

Project Member Reported by pran...@brave.com, Jan 11

Issue description

Chrome Version: (copy from chrome://version) Chromium	73.0.3668.0 (Developer Build) (64-bit)
OS: (e.g. Win10, MacOS 10.12, etc...) MacOS

What steps will reproduce the problem?
(1) Open https://download.gimp.org/mirror/pub/gimp/v2.10/osx/gimp-2.10.8-x86_64-2.dmg in Chromium Debug with Safe-Browsing enabled

What is the expected result?
File downloads 

What happens instead?
```
[20946:775:0111/124628.200651:FATAL:thread_restrictions.cc(34)] Check failed: !g_blocking_disallowed.Get().Get(). Function marked as blocking was called from a scope that disallows blocking! If this task is running inside the TaskScheduler, it needs to have MayBlock() in its TaskTraits. Otherwise, consider making this blocking work asynchronous or, as a last resort, you may use ScopedAllowBlocking (see its documentation for best practices).
0   libbase.dylib                       0x0000000104a925e3 base::debug::StackTrace::StackTrace(unsigned long) + 83
1   libbase.dylib                       0x0000000104a9269d base::debug::StackTrace::StackTrace(unsigned long) + 29
2   libbase.dylib                       0x0000000104779cea base::debug::StackTrace::StackTrace() + 26
3   libbase.dylib                       0x00000001047bfcbe logging::LogMessage::~LogMessage() + 142
4   libbase.dylib                       0x00000001047beab5 logging::LogMessage::~LogMessage() + 21
5   libbase.dylib                       0x00000001049ea73c base::internal::AssertBlockingAllowed() + 188
6   libbase.dylib                       0x00000001049d2eed base::ScopedBlockingCall::ScopedBlockingCall(base::BlockingType) + 237
7   libbase.dylib                       0x00000001049d321b base::ScopedBlockingCall::ScopedBlockingCall(base::BlockingType) + 27
8   libbase.dylib                       0x0000000104a9fd88 base::File::DoInitialize(base::FilePath const&, unsigned int) + 72
9   libbase.dylib                       0x000000010478c667 base::File::Initialize(base::FilePath const&, unsigned int) + 199
10  libbase.dylib                       0x000000010478c596 base::File::File(base::FilePath const&, unsigned int) + 102
11  libbase.dylib                       0x000000010478c6c3 base::File::File(base::FilePath const&, unsigned int) + 35
12  libchrome_dll.dylib                 0x000000010f316fbf safe_browsing::FileAnalyzer::StartExtractDmgFeatures() + 303
13  libchrome_dll.dylib                 0x000000010f316866 safe_browsing::FileAnalyzer::Start(base::FilePath const&, base::FilePath const&, base::OnceCallback<void (safe_browsing::FileAnalyzer::Results)>) + 566
14  libchrome_dll.dylib                 0x000000010f2f06bb safe_browsing::CheckClientDownloadRequest::AnalyzeFile() + 1259
15  libchrome_dll.dylib                 0x000000010f2ee4ad safe_browsing::CheckClientDownloadRequest::OnUrlWhitelistCheckDone(bool) + 605
```

Please use labels and text to provide additional information.

If this is a regression (i.e., worked before), please consider using the
bisect tool (https://www.chromium.org/developers/bisect-builds-py) to help
us identify the root cause and more rapidly triage the issue.

For graphics-related bugs, please copy/paste the contents of the about:gpu
page at the end of this report.


 
Components: Services>Safebrowsing
Cc: rsesek@chromium.org drubery@chromium.org
Potential fix:

diff --git a/chrome/browser/safe_browsing/download_protection/file_analyzer.cc b/chrome/browser/safe_browsing/download_protection/file_analyzer.cc
index 444fd075d886..683d4f23f2c6 100644
--- a/chrome/browser/safe_browsing/download_protection/file_analyzer.cc
+++ b/chrome/browser/safe_browsing/download_protection/file_analyzer.cc
@@ -275,29 +275,14 @@ void FileAnalyzer::StartExtractDmgFeatures() {
 
   // Directly use 'dmg' extension since download file may not have any
   // extension, but has still been deemed a DMG through file type sniffing.
-  base::File file(tmp_path_, base::File::FLAG_OPEN | base::File::FLAG_READ);
-  if (!file.IsValid()) {
-    std::move(callback_).Run(std::move(results_));
-    return;
-  }
-  int64_t size = file.GetLength();
-
-  bool too_big_to_unpack =
-      base::checked_cast<uint64_t>(size) >
-      FileTypePolicies::GetInstance()->GetMaxFileSizeToAnalyze("dmg");
-  UMA_HISTOGRAM_BOOLEAN("SBClientDownload.DmgTooBigToUnpack",
-                        too_big_to_unpack);
-  if (too_big_to_unpack) {
-    std::move(callback_).Run(std::move(results_));
-  } else {
-    dmg_analyzer_ = new SandboxedDMGAnalyzer(
-        tmp_path_,
-        base::BindRepeating(&FileAnalyzer::OnDmgAnalysisFinished,
-                            weakptr_factory_.GetWeakPtr()),
-        content::ServiceManagerConnection::GetForProcess()->GetConnector());
-    dmg_analyzer_->Start();
-    dmg_analysis_start_time_ = base::TimeTicks::Now();
-  }
+  dmg_analyzer_ = new SandboxedDMGAnalyzer(
+      tmp_path_,
+      FileTypePolicies::GetInstance()->GetMaxFileSizeToAnalyze("dmg"),
+      base::BindRepeating(&FileAnalyzer::OnDmgAnalysisFinished,
+                          weakptr_factory_.GetWeakPtr()),
+      content::ServiceManagerConnection::GetForProcess()->GetConnector());
+  dmg_analyzer_->Start();
+  dmg_analysis_start_time_ = base::TimeTicks::Now();
 }
 
 void FileAnalyzer::ExtractFileOrDmgFeatures(
diff --git a/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac.cc b/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac.cc
index ea6c8ea07d42..92ac9b299288 100644
--- a/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac.cc
+++ b/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac.cc
@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/task/post_task.h"
 #include "chrome/common/safe_browsing/archive_analyzer_results.h"
 #include "chrome/services/file_util/public/mojom/constants.mojom.h"
@@ -16,9 +17,11 @@
 
 SandboxedDMGAnalyzer::SandboxedDMGAnalyzer(
     const base::FilePath& dmg_file,
+    const uint64_t max_size,
     const ResultCallback& callback,
     service_manager::Connector* connector)
-    : file_path_(dmg_file), callback_(callback), connector_(connector) {
+    : file_path_(dmg_file), max_size_(max_size),
+      callback_(callback), connector_(connector) {
   DCHECK(callback);
 }
 
@@ -45,6 +48,18 @@ void SandboxedDMGAnalyzer::PrepareFileToAnalyze() {
     return;
   }
 
+  int64_t size = file.GetLength();
+
+  bool too_big_to_unpack = base::checked_cast<uint64_t>(size) > max_size_;
+  UMA_HISTOGRAM_BOOLEAN("SBClientDownload.DmgTooBigToUnpack",
+                        too_big_to_unpack);
+
+  if (too_big_to_unpack) {
+    DLOG(ERROR) << "File is too big: " << file_path_.value();
+    ReportFileFailure();
+    return;
+  }
+
   base::PostTaskWithTraits(FROM_HERE, {content::BrowserThread::UI},
                            base::Bind(&SandboxedDMGAnalyzer::AnalyzeFile, this,
                                       base::Passed(&file)));
diff --git a/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac.h b/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac.h
index 06ddd6228216..f8c790c1bd73 100644
--- a/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac.h
+++ b/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac.h
@@ -30,6 +30,7 @@ class SandboxedDMGAnalyzer
       base::Callback<void(const safe_browsing::ArchiveAnalyzerResults&)>;
 
   SandboxedDMGAnalyzer(const base::FilePath& dmg_file,
+                       const uint64_t max_size,
                        const ResultCallback& callback,
                        service_manager::Connector* connector);
 
@@ -56,6 +57,9 @@ class SandboxedDMGAnalyzer
   // The file path of the file to analyze.
   const base::FilePath file_path_;
 
+  // Maximum file size allowed by FilePolicy
+  const uint64_t max_size_;
+
   // Callback invoked on the UI thread with the file analyze results.
   const ResultCallback callback_;
 
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 17 (5 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc50f6659308fc734103026813da970756724681

commit cc50f6659308fc734103026813da970756724681
Author: Pranjal Jumde <pranjal@brave.com>
Date: Thu Jan 17 19:41:30 2019

Fix ScopedBlockingCall DCHECK when analyzing DMG files.

file_analyzer.cc opens dmg on the UI thread which disallows blocking leading
to a DCHECK failure on debug builds.

Test:

1. Open chromium (debug)
2. Navigate to
   https://github.com/MagerValp/AutoDMG/releases/download/v1.9/AutoDMG-1.9.dmg
3. No crash

Bug:  921148 
Change-Id: Idb3a06d747a61ed5f92c5f3be92d791877dcd3c7
Reviewed-on: https://chromium-review.googlesource.com/c/1414131
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623804}
[modify] https://crrev.com/cc50f6659308fc734103026813da970756724681/AUTHORS
[modify] https://crrev.com/cc50f6659308fc734103026813da970756724681/chrome/browser/safe_browsing/download_protection/file_analyzer.cc
[modify] https://crrev.com/cc50f6659308fc734103026813da970756724681/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac.cc
[modify] https://crrev.com/cc50f6659308fc734103026813da970756724681/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac.h
[modify] https://crrev.com/cc50f6659308fc734103026813da970756724681/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac_unittest.cc

Comment 6 by vakh@chromium.org, Jan 18 (4 days ago)

Status: Fixed (was: Untriaged)

Sign in to add a comment