Make CertPathBuilder::Run return Result rather than being an out-param of the constructor.

Bug: 410574
Change-Id: I407332f05d591709f5602d92e5697caa48140ab3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1731102
Commit-Queue: Matt Mueller <mattm@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Reviewed-by: Doug Steedman <dougsteed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683697}

components/cast_certificate/cast_cert_validator.cc

@@ -303,14 +303,13 @@ CastCertError VerifyDeviceCertUsingCustomTrustStore(
   net::der::GeneralizedTime verification_time;
   if (!net::der::EncodeTimeAsGeneralizedTime(time, &verification_time))
     return CastCertError::ERR_UNEXPECTED;
-  net::CertPathBuilder::Result result;
   net::CertPathBuilder path_builder(
       target_cert.get(), trust_store, &path_builder_delegate, verification_time,
       net::KeyPurpose::CLIENT_AUTH, net::InitialExplicitPolicy::kFalse,
       {net::AnyPolicy()}, net::InitialPolicyMappingInhibit::kFalse,
-      net::InitialAnyPolicyInhibit::kFalse, &result);
+      net::InitialAnyPolicyInhibit::kFalse);
   path_builder.AddCertIssuerSource(&intermediate_cert_issuer_source);
-  path_builder.Run();
+  net::CertPathBuilder::Result result = path_builder.Run();
   if (!result.HasValidPath())
     return MapToCastError(result);
 

components/cast_certificate/cast_crl.cc

@@ -137,13 +137,12 @@ bool VerifyCRL(const Crl& crl,
   net::SimplePathBuilderDelegate path_builder_delegate(
       2048, net::SimplePathBuilderDelegate::DigestPolicy::kWeakAllowSha1);
 
-  net::CertPathBuilder::Result result;
   net::CertPathBuilder path_builder(
       parsed_cert.get(), trust_store, &path_builder_delegate, verification_time,
       net::KeyPurpose::ANY_EKU, net::InitialExplicitPolicy::kFalse,
       {net::AnyPolicy()}, net::InitialPolicyMappingInhibit::kFalse,
-      net::InitialAnyPolicyInhibit::kFalse, &result);
-  path_builder.Run();
+      net::InitialAnyPolicyInhibit::kFalse);
+  net::CertPathBuilder::Result result = path_builder.Run();
   if (!result.HasValidPath()) {
     VLOG(2) << "CRL - Issuer certificate verification failed.";
     // TODO(crbug.com/634443): Log the error information.

net/cert/cert_verify_proc_builtin.cc

@@ -421,26 +421,25 @@ struct BuildPathAttempt {
   SimplePathBuilderDelegate::DigestPolicy digest_policy;
 };
 
-void TryBuildPath(const scoped_refptr<ParsedCertificate>& target,
-                  CertIssuerSourceStatic* intermediates,
-                  SystemTrustStore* ssl_trust_store,
-                  base::Time verification_time,
-                  base::TimeTicks deadline,
-                  VerificationType verification_type,
-                  SimplePathBuilderDelegate::DigestPolicy digest_policy,
-                  int flags,
-                  const std::string& ocsp_response,
-                  const CRLSet* crl_set,
-                  CertNetFetcher* net_fetcher,
-                  const EVRootCAMetadata* ev_metadata,
-                  CertPathBuilder::Result* result,
-                  bool* checked_revocation) {
+CertPathBuilder::Result TryBuildPath(
+    const scoped_refptr<ParsedCertificate>& target,
+    CertIssuerSourceStatic* intermediates,
+    SystemTrustStore* ssl_trust_store,
+    base::Time verification_time,
+    base::TimeTicks deadline,
+    VerificationType verification_type,
+    SimplePathBuilderDelegate::DigestPolicy digest_policy,
+    int flags,
+    const std::string& ocsp_response,
+    const CRLSet* crl_set,
+    CertNetFetcher* net_fetcher,
+    const EVRootCAMetadata* ev_metadata,
+    bool* checked_revocation) {
   der::GeneralizedTime der_verification_time;
   if (!der::EncodeTimeAsGeneralizedTime(verification_time,
                                         &der_verification_time)) {
     // This shouldn't be possible.
-    *result = CertPathBuilder::Result();
-    return;
+    return CertPathBuilder::Result();
   }
 
   // Path building will require candidate paths to conform to at least one of
@@ -462,8 +461,7 @@ void TryBuildPath(const scoped_refptr<ParsedCertificate>& target,
       target, ssl_trust_store->GetTrustStore(), &path_builder_delegate,
       der_verification_time, KeyPurpose::SERVER_AUTH,
       InitialExplicitPolicy::kFalse, user_initial_policy_set,
-      InitialPolicyMappingInhibit::kFalse, InitialAnyPolicyInhibit::kFalse,
-      result);
+      InitialPolicyMappingInhibit::kFalse, InitialAnyPolicyInhibit::kFalse);
 
   // Allow the path builder to discover the explicitly provided intermediates in
   // |input_cert|.
@@ -481,7 +479,7 @@ void TryBuildPath(const scoped_refptr<ParsedCertificate>& target,
   path_builder.SetIterationLimit(kPathBuilderIterationLimit);
   path_builder.SetDeadline(deadline);
 
-  path_builder.Run();
+  return path_builder.Run();
 }
 
 int AssignVerifyResult(X509Certificate* input_cert,
@@ -652,11 +650,11 @@ int CertVerifyProcBuiltin::VerifyInternal(
     verification_type = cur_attempt.verification_type;
 
     // Run the attempt through the path builder.
-    TryBuildPath(target, &intermediates, ssl_trust_store.get(),
-                 verification_time, deadline, cur_attempt.verification_type,
-                 cur_attempt.digest_policy, flags, ocsp_response, crl_set,
-                 net_fetcher_.get(), ev_metadata, &result,
-                 &checked_revocation_for_some_path);
+    result = TryBuildPath(
+        target, &intermediates, ssl_trust_store.get(), verification_time,
+        deadline, cur_attempt.verification_type, cur_attempt.digest_policy,
+        flags, ocsp_response, crl_set, net_fetcher_.get(), ev_metadata,
+        &checked_revocation_for_some_path);
 
     if (result.HasValidPath() || result.exceeded_deadline)
       break;

net/cert/internal/path_builder.cc

@@ -545,8 +545,7 @@ CertPathBuilder::CertPathBuilder(
     InitialExplicitPolicy initial_explicit_policy,
     const std::set<der::Input>& user_initial_policy_set,
     InitialPolicyMappingInhibit initial_policy_mapping_inhibit,
-    InitialAnyPolicyInhibit initial_any_policy_inhibit,
-    Result* result)
+    InitialAnyPolicyInhibit initial_any_policy_inhibit)
     : cert_path_iter_(new CertPathIter(std::move(cert), trust_store)),
       delegate_(delegate),
       time_(time),
@@ -554,10 +553,8 @@ CertPathBuilder::CertPathBuilder(
       initial_explicit_policy_(initial_explicit_policy),
       user_initial_policy_set_(user_initial_policy_set),
       initial_policy_mapping_inhibit_(initial_policy_mapping_inhibit),
-      initial_any_policy_inhibit_(initial_any_policy_inhibit),
-      out_result_(result) {
+      initial_any_policy_inhibit_(initial_any_policy_inhibit) {
   DCHECK(delegate);
-  *result = Result();
   // The TrustStore also implements the CertIssuerSource interface.
   AddCertIssuerSource(trust_store);
 }
@@ -577,7 +574,8 @@ void CertPathBuilder::SetDeadline(base::TimeTicks deadline) {
   deadline_ = deadline;
 }
 
-void CertPathBuilder::Run() {
+CertPathBuilder::Result CertPathBuilder::Run() {
+  Result result;
   uint32_t iteration_count = 0;
 
   while (true) {
@@ -589,13 +587,13 @@ void CertPathBuilder::Run() {
                                       &iteration_count, max_iteration_count_)) {
       // No more paths to check.
       if (max_iteration_count_ > 0 && iteration_count > max_iteration_count_) {
-        out_result_->exceeded_iteration_limit = true;
+        result.exceeded_iteration_limit = true;
       }
       if (!deadline_.is_null() && base::TimeTicks::Now() > deadline_) {
-        out_result_->exceeded_deadline = true;
+        result.exceeded_deadline = true;
       }
       RecordIterationCountHistogram(iteration_count);
-      return;
+      return result;
     }
 
     // Verify the entire certificate chain.
@@ -613,25 +611,26 @@ void CertPathBuilder::Run() {
 
     bool path_is_good = result_path->IsValid();
 
-    AddResultPath(std::move(result_path));
+    AddResultPath(std::move(result_path), &result);
 
     if (path_is_good) {
       RecordIterationCountHistogram(iteration_count);
       // Found a valid path, return immediately.
       // TODO(mattm): add debug/test mode that tries all possible paths.
-      return;
+      return result;
     }
     // Path did not verify. Try more paths.
   }
 }
 
 void CertPathBuilder::AddResultPath(
-    std::unique_ptr<CertPathBuilderResultPath> result_path) {
+    std::unique_ptr<CertPathBuilderResultPath> result_path,
+    Result* out_result) {
   // TODO(mattm): set best_result_index based on number or severity of errors.
   if (result_path->IsValid())
-    out_result_->best_result_index = out_result_->paths.size();
+    out_result->best_result_index = out_result->paths.size();
   // TODO(mattm): add flag to only return a single path or all attempted paths?
-  out_result_->paths.push_back(std::move(result_path));
+  out_result->paths.push_back(std::move(result_path));
 }
 
 }  // namespace net

net/cert/internal/path_builder.h

@@ -141,16 +141,14 @@ class NET_EXPORT CertPathBuilder {
   };
 
   // Creates a CertPathBuilder that attempts to find a path from |cert| to a
-  // trust anchor in |trust_store| and is valid at |time|. Details of attempted
-  // path(s) are stored in |*result|.
+  // trust anchor in |trust_store| and is valid at |time|.
   //
-  // The caller must keep |trust_store|, |delegate| and |*result| valid for the
-  // lifetime of the CertPathBuilder.
+  // The caller must keep |trust_store| and |delegate| valid for the lifetime
+  // of the CertPathBuilder.
   //
   // See VerifyCertificateChain() for a more detailed explanation of the
   // same-named parameters not defined below.
   //
-  // * |result|: Storage for the result of path building.
   // * |delegate|: Must be non-null. The delegate is called at various points in
   //               path building to verify specific parts of certificates or the
   //               final chain. See CertPathBuilderDelegate and
@@ -163,8 +161,7 @@ class NET_EXPORT CertPathBuilder {
                   InitialExplicitPolicy initial_explicit_policy,
                   const std::set<der::Input>& user_initial_policy_set,
                   InitialPolicyMappingInhibit initial_policy_mapping_inhibit,
-                  InitialAnyPolicyInhibit initial_any_policy_inhibit,
-                  Result* result);
+                  InitialAnyPolicyInhibit initial_any_policy_inhibit);
   ~CertPathBuilder();
 
   // Adds a CertIssuerSource to provide intermediates for use in path building.
@@ -188,13 +185,12 @@ class NET_EXPORT CertPathBuilder {
 
   // Executes verification of the target certificate.
   //
-  // Upon return results are written to the |result| object passed into the
-  // constructor. Run must not be called more than once on each CertPathBuilder
-  // instance.
-  void Run();
+  // Run must not be called more than once on each CertPathBuilder instance.
+  Result Run();
 
  private:
-  void AddResultPath(std::unique_ptr<CertPathBuilderResultPath> result_path);
+  void AddResultPath(std::unique_ptr<CertPathBuilderResultPath> result_path,
+                     Result* out_result);
 
   std::unique_ptr<CertPathIter> cert_path_iter_;
   CertPathBuilderDelegate* delegate_;
@@ -207,7 +203,7 @@ class NET_EXPORT CertPathBuilder {
   uint32_t max_iteration_count_ = 0;
   base::TimeTicks deadline_;
 
-  Result* out_result_;
+  Result out_result_;
 
   DISALLOW_COPY_AND_ASSIGN(CertPathBuilder);
 };

net/cert/internal/path_builder_pkits_unittest.cc

@@ -157,15 +157,14 @@ class PathBuilderPkitsTestDelegate {
           1024, SimplePathBuilderDelegate::DigestPolicy::kWeakAllowSha1);
     }
 
-    CertPathBuilder::Result result;
     CertPathBuilder path_builder(
         std::move(target_cert), &trust_store, path_builder_delegate.get(),
         info.time, KeyPurpose::ANY_EKU, info.initial_explicit_policy,
         info.initial_policy_set, info.initial_policy_mapping_inhibit,
-        info.initial_inhibit_any_policy, &result);
+        info.initial_inhibit_any_policy);
     path_builder.AddCertIssuerSource(&cert_issuer_source);
 
-    path_builder.Run();
+    CertPathBuilder::Result result = path_builder.Run();
 
     if (info.should_validate != result.HasValidPath()) {
       for (size_t i = 0; i < result.paths.size(); ++i) {

net/cert/internal/path_builder_verify_certificate_chain_unittest.cc

@@ -42,16 +42,15 @@ class PathBuilderTestDelegate {
     for (size_t i = 1; i < test.chain.size(); ++i)
       intermediate_cert_issuer_source.AddCert(test.chain[i]);
 
-    CertPathBuilder::Result result;
     // First cert in the |chain| is the target.
     CertPathBuilder path_builder(
         test.chain.front(), &trust_store, &path_builder_delegate, test.time,
         test.key_purpose, test.initial_explicit_policy,
         test.user_initial_policy_set, test.initial_policy_mapping_inhibit,
-        test.initial_any_policy_inhibit, &result);
+        test.initial_any_policy_inhibit);
     path_builder.AddCertIssuerSource(&intermediate_cert_issuer_source);
 
-    path_builder.Run();
+    CertPathBuilder::Result result = path_builder.Run();
     EXPECT_EQ(!test.HasHighSeverityErrors(), result.HasValidPath());
   }
 };

net/tools/cert_verify_tool/verify_using_path_builder.cc

@@ -166,12 +166,11 @@ bool VerifyUsingPathBuilder(
   // Verify the chain.
   net::SimplePathBuilderDelegate delegate(
       2048, net::SimplePathBuilderDelegate::DigestPolicy::kWeakAllowSha1);
-  net::CertPathBuilder::Result result;
   net::CertPathBuilder path_builder(
       target_cert, ssl_trust_store->GetTrustStore(), &delegate, time,
       net::KeyPurpose::SERVER_AUTH, net::InitialExplicitPolicy::kFalse,
       {net::AnyPolicy()}, net::InitialPolicyMappingInhibit::kFalse,
-      net::InitialAnyPolicyInhibit::kFalse, &result);
+      net::InitialAnyPolicyInhibit::kFalse);
   path_builder.AddCertIssuerSource(&intermediate_cert_issuer_source);
 
   std::unique_ptr<net::CertIssuerSourceAia> aia_cert_issuer_source;
@@ -182,7 +181,7 @@ bool VerifyUsingPathBuilder(
   }
 
   // Run the path builder.
-  path_builder.Run();
+  net::CertPathBuilder::Result result = path_builder.Run();
 
   // TODO(crbug.com/634443): Display any errors/warnings associated with path
   //                         building that were not part of a particular