Add metrics on archived_binaries, and address prev CL comments

* Follow up to comments from https://crrev.com/c/767663
* Also limit archived_binaries from DMGs
* Add metrics so we can see how many we're dropping.

Bug: 774664
Change-Id: I2f901ad612763f36f84e8ea2cbb1b8d8909d9509
Reviewed-on: https://chromium-review.googlesource.com/773789Reviewed-by: Luke Z <lpz@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Nathan Parker <nparker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519959}

chrome/browser/safe_browsing/download_protection/check_client_download_request.cc

@@ -516,12 +516,16 @@ void CheckClientDownloadRequest::OnZipAnalysisFinished(
   archive_is_valid_ =
       (results.success ? ArchiveValid::VALID : ArchiveValid::INVALID);
   archived_executable_ = results.has_executable;
-  CopyArchivedBinaries(results.archived_binary, &archived_binary_);
+  CopyArchivedBinaries(results.archived_binary, &archived_binaries_);
   DVLOG(1) << "Zip analysis finished for " << item_->GetFullPath().value()
            << ", has_executable=" << results.has_executable
            << ", has_archive=" << results.has_archive
            << ", success=" << results.success;
 
+  if (archived_executable_) {
+    UMA_HISTOGRAM_COUNTS("SBClientDownload.ZipFileArchivedBinariesCount",
+                         results.archived_binary.size());
+  }
   UMA_HISTOGRAM_BOOLEAN("SBClientDownload.ZipFileSuccess", results.success);
   UMA_HISTOGRAM_BOOLEAN("SBClientDownload.ZipFileHasExecutable",
                         archived_executable_);
@@ -615,7 +619,8 @@ void CheckClientDownloadRequest::OnDmgAnalysisFinished(
   archive_is_valid_ =
       (results.success ? ArchiveValid::VALID : ArchiveValid::INVALID);
   archived_executable_ = results.has_executable;
-  archived_binary_.CopyFrom(results.archived_binary);
+  CopyArchivedBinaries(results.archived_binary, &archived_binaries_);
+
   DVLOG(1) << "DMG analysis has finished for " << item_->GetFullPath().value()
            << ", has_executable=" << results.has_executable
            << ", success=" << results.success;
@@ -634,6 +639,8 @@ void CheckClientDownloadRequest::OnDmgAnalysisFinished(
   if (archived_executable_) {
     UMA_HISTOGRAM_SPARSE_SLOWLY("SBClientDownload.DmgFileHasExecutableByType",
                                 uma_file_type);
+    UMA_HISTOGRAM_COUNTS("SBClientDownload.DmgFileArchivedBinariesCount",
+                         results.archived_binary.size());
   } else {
     UMA_HISTOGRAM_SPARSE_SLOWLY("SBClientDownload.DmgFileHasNoExecutableByType",
                                 uma_file_type);
@@ -890,8 +897,8 @@ void CheckClientDownloadRequest::SendRequest() {
   request.mutable_signature()->CopyFrom(signature_info_);
   if (image_headers_)
     request.set_allocated_image_headers(image_headers_.release());
-  if (!archived_binary_.empty())
-    request.mutable_archived_binary()->Swap(&archived_binary_);
+  if (!archived_binaries_.empty())
+    request.mutable_archived_binary()->Swap(&archived_binaries_);
   if (!request.SerializeToString(&client_download_request_data_)) {
     FinishRequest(DownloadCheckResult::UNKNOWN, REASON_INVALID_REQUEST_PROTO);
     return;
@@ -910,7 +917,7 @@ void CheckClientDownloadRequest::SendRequest() {
   service_->client_download_request_callbacks_.Notify(item_, &request);
   DVLOG(2) << "Sending a request for URL: " << item_->GetUrlChain().back();
   DVLOG(2) << "Detected " << request.archived_binary().size() << " archived "
-           << "binaries";
+           << "binaries (may be capped)";
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("client_download_request", R"(
           semantics {

chrome/browser/safe_browsing/download_protection/check_client_download_request.h

@@ -130,7 +130,7 @@ class CheckClientDownloadRequest
 
   ClientDownloadRequest_SignatureInfo signature_info_;
   std::unique_ptr<ClientDownloadRequest_ImageHeaders> image_headers_;
-  ArchivedBinaries archived_binary_;
+  ArchivedBinaries archived_binaries_;
   CheckDownloadCallback callback_;
   // Will be NULL if the request has been canceled.
   DownloadProtectionService* service_;

chrome/browser/safe_browsing/download_protection/check_client_download_request_unittest.cc

@@ -40,19 +40,26 @@ class CheckClientDownloadRequestTest : public testing::Test {
 TEST_F(CheckClientDownloadRequestTest, CheckLimitArchivedExtensions) {
   CheckClientDownloadRequest::ArchivedBinaries src, dest;
 
-  for (int i = 0; i < 4; i++) {
+  const int max_to_try = 12;
+  for (int i = 0; i < max_to_try; i++) {
     src.Add();
   }
 
-  SetMaxArchivedBinariesToReport(0);
+  // First check against the value set in .asciipb, which is currently 10
+  // If that is raised above |max_to_try|, raise the latter.
   dest.Clear();
   CheckClientDownloadRequest::CopyArchivedBinaries(src, &dest);
-  EXPECT_EQ(dest.size(), 0);
+  EXPECT_EQ(10, dest.size());
 
   SetMaxArchivedBinariesToReport(2);
   dest.Clear();
   CheckClientDownloadRequest::CopyArchivedBinaries(src, &dest);
-  EXPECT_EQ(dest.size(), 2);
+  EXPECT_EQ(2, dest.size());
+
+  SetMaxArchivedBinariesToReport(100000);
+  dest.Clear();
+  CheckClientDownloadRequest::CopyArchivedBinaries(src, &dest);
+  EXPECT_EQ(max_to_try, dest.size());
 }
 
 }  // namespace safe_browsing

tools/metrics/histograms/histograms.xml

@@ -73005,6 +73005,15 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="SBClientDownload.DmgFileArchivedBinariesCount" units="count">
+  <owner>nparker@chromium.org</owner>
+  <summary>
+    The original number of archived_binaries found in a DMG-like file when it's
+    scanned, if at least one is found.  The actual number sent in the download
+    request may be capped below this value.
+  </summary>
+</histogram>
+
 <histogram name="SBClientDownload.DmgFileFailureByType"
     enum="SBClientDownloadExtensions">
   <owner>nparker@chromium.org</owner>
@@ -73261,6 +73270,15 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="SBClientDownload.ZipFileArchivedBinariesCount" units="count">
+  <owner>nparker@chromium.org</owner>
+  <summary>
+    The original number of archived_binaries found in a zip-like file when it's
+    scanned, if at least one is found.  The actual number sent in the download
+    request may be capped below this value.
+  </summary>
+</histogram>
+
 <histogram name="SBClientDownload.ZipFileContainsAppDirectory" enum="Boolean">
   <owner>jialiul@chromium.org</owner>
   <summary>