Update some documentation for CertVerifyResult::verified_cert

net/cert/cert_verify_result.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_CERT_CERT_VERIFY_RESULT_H_
 #define NET_CERT_CERT_VERIFY_RESULT_H_
 
 #include <vector>
 
 #include "base/memory/ref_counted.h"
 #include "net/base/net_export.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/cert/ocsp_verify_result.h"
 #include "net/cert/x509_cert_types.h"
 
 namespace net {
 
 class X509Certificate;
 
 // The result of certificate verification.
 class NET_EXPORT CertVerifyResult {
  public:
   CertVerifyResult();
   CertVerifyResult(const CertVerifyResult& other);
   ~CertVerifyResult();
 
   void Reset();
 
+  // Comparing CertVerifyResult with a nullptr |verified_cert| is invalid.

[Ryan Sleevi, 2017/01/09 23:20:14]

Why? Are you documenting implementation details or interface contract? Feels
like the former.

Alternatively, if the contract is "verified_cert should never be nullptr",
perhaps that's part of the contract for verified_cert, rather than being
documented here?

[eroman, 2017/01/09 23:33:12]

I was documenting the current behavior, which relies on not being called with a
default-initialized (or Reset()) CertVerifyResult.

This looked a bit suspicious when reading through the code, since one generally
expects operator== to work on any instance, but in practice it doesn't get
called in that mode.

Instead of the comment I could modify the implementation to test for nullptr,
however I have not reviewed the callers to see if that policy is appropriate yet
(i.e. if hitting that situation is wrong, crashing may be preferable)

   bool operator==(const CertVerifyResult& other) const;
 
   // The certificate and chain that was constructed during verification.
   // Note that the though the verified certificate will match the originally
   // supplied certificate, the intermediate certificates stored within may
-  // be substantially different. In the event of a verification failure, this
-  // will contain the chain as supplied by the server. This may be NULL if
-  // running within the sandbox.
+  // be substantially different (both their order and their content).
+  // In the event of a verification failure, |verified_cert| may contain a
+  // partially constructed path, or the original chain.

[Ryan Sleevi, 2017/01/09 23:20:14]

s/, / /

[eroman, 2017/01/09 23:33:12]

Done.

+  // In the case of successful verification the trust anchor is the final
+  // intermediate, or the target certificate if there are no intermediates.

[Ryan Sleevi, 2017/01/09 23:20:14]

Documentation wise, there's also some pre-existing subject/verb agreement issues

// The certificate chain that was constructed during verification.
// Note: Although |verified_cert| will match the originally supplied
// certificate to be validated, the results of
// GetIntermediateCertificates() may be substantially different,
// both in order and in content, then the originally supplied
// intermediates.
// In the event of validation failures, this may contain the
// originally supplied certificate chain or a partially
// constructed path, depending on the implementation.
// In the event of validation success, the trust anchor will be
// |verified_cert->GetIntermediateCertificates().back()| if
// there was a certificate chain to the trust anchor, and will
// be |verified_cert->os_cert_handle()| if the certificate was
// the trust anchor.

WDYT? Probably still botched the wording.

[eroman, 2017/01/09 23:33:12]

Done.

   scoped_refptr<X509Certificate> verified_cert;
 
   // Bitmask of CERT_STATUS_* from net/cert/cert_status_flags.h. Note that
   // these status flags apply to the certificate chain returned in
   // |verified_cert|, rather than the originally supplied certificate
   // chain.
   CertStatus cert_status;
 
   // Properties of the certificate chain.
   bool has_md2;
@@ skipping 21 matching lines @@
   // name, rather than using the subjectAltName.
   bool common_name_fallback_used;
 
   // Verification of stapled OCSP response, if present.
   OCSPVerifyResult ocsp_result;
 };
 
 }  // namespace net
 
 #endif  // NET_CERT_CERT_VERIFY_RESULT_H_