Add functions to convert from Span<const uint8> and std::string_view

These can replace the current der::Input-specific string_view methods. I
converted many of the uses within the library, but we should take a pass
over the library and turn many of the std::strings into
std::vector<uint8_t>.

On MSVC, we have to pass /Zc:__cplusplus for __cplusplus to be set
correctly. For now, I'm going to try just adding the flag, but if pki
gets used in more places before we bump our minimum to C++17, we may
want to start looking at _MSVC_LANG instead.

There are actually a few places outside of pki that could use this, but
we sadly can't use them until we require C++17 across the board.

Bug: 661
Change-Id: I1002d9f2e1e4a2592760e8938560894d42a9b58c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65908
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: Matt Mueller <mattm@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

16 files changed, 66 insertions, 47 deletions

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 0639217..5933f02 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -203,7 +203,11 @@ elseif(MSVC)
   string(REPLACE "C" " -wd" MSVC_DISABLED_WARNINGS_STR
                             ${MSVC_DISABLED_WARNINGS_LIST})
   set(CMAKE_C_FLAGS   "-utf-8 -W4 -WX ${MSVC_DISABLED_WARNINGS_STR}")
-  set(CMAKE_CXX_FLAGS "-utf-8 -W4 -WX ${MSVC_DISABLED_WARNINGS_STR}")
+  # Without /Zc:__cplusplus, MSVC does not define the right value for
+  # __cplusplus. See https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/
+  # If this becomes too problematic for downstream code, we can look at
+  # _MSVC_LANG.
+  set(CMAKE_CXX_FLAGS "-utf-8 -W4 -WX ${MSVC_DISABLED_WARNINGS_STR} -Zc:__cplusplus")
 endif()
 
 if(WIN32)
diff --git a/include/openssl/span.h b/include/openssl/span.h
index dd66f88..38196ae 100644
--- a/include/openssl/span.h
+++ b/include/openssl/span.h
@@ -26,6 +26,10 @@ extern "C++" {
 #include <algorithm>
 #include <type_traits>
 
+#if __cplusplus >= 201703L
+#include <string_view>
+#endif
+
 BSSL_NAMESPACE_BEGIN
 
 template <typename T>
@@ -210,6 +214,16 @@ constexpr Span<const T> MakeConstSpan(T (&array)[size]) {
   return array;
 }
 
+#if __cplusplus >= 201703L
+inline Span<const uint8_t> StringAsBytes(std::string_view s) {
+  return MakeConstSpan(reinterpret_cast<const uint8_t *>(s.data()), s.size());
+}
+
+inline std::string_view BytesAsStringView(bssl::Span<const uint8_t> b) {
+  return std::string_view(reinterpret_cast<const char *>(b.data()), b.size());
+}
+#endif
+
 BSSL_NAMESPACE_END
 
 }  // extern C++
diff --git a/pki/cert_error_params.cc b/pki/cert_error_params.cc
index c64fa1b..075d7ef 100644
--- a/pki/cert_error_params.cc
+++ b/pki/cert_error_params.cc
@@ -22,9 +22,9 @@ class CertErrorParams2Der : public CertErrorParams {
   CertErrorParams2Der(const char *name1, der::Input der1, const char *name2,
                       der::Input der2)
       : name1_(name1),
-        der1_(der1.AsString()),
+        der1_(BytesAsStringView(der1)),
         name2_(name2),
-        der2_(der2.AsString()) {}
+        der2_(BytesAsStringView(der2)) {}
 
   CertErrorParams2Der(const CertErrorParams2Der &) = delete;
   CertErrorParams2Der &operator=(const CertErrorParams2Der &) = delete;
diff --git a/pki/cert_issuer_source_static.cc b/pki/cert_issuer_source_static.cc
index bbf967f..fc20eb9 100644
--- a/pki/cert_issuer_source_static.cc
+++ b/pki/cert_issuer_source_static.cc
@@ -12,7 +12,7 @@ CertIssuerSourceStatic::~CertIssuerSourceStatic() = default;
 void CertIssuerSourceStatic::AddCert(
     std::shared_ptr<const ParsedCertificate> cert) {
   intermediates_.insert(std::make_pair(
-      cert->normalized_subject().AsStringView(), std::move(cert)));
+      BytesAsStringView(cert->normalized_subject()), std::move(cert)));
 }
 
 void CertIssuerSourceStatic::Clear() { intermediates_.clear(); }
@@ -20,7 +20,7 @@ void CertIssuerSourceStatic::Clear() { intermediates_.clear(); }
 void CertIssuerSourceStatic::SyncGetIssuersOf(const ParsedCertificate *cert,
                                               ParsedCertificateList *issuers) {
   auto range =
-      intermediates_.equal_range(cert->normalized_issuer().AsStringView());
+      intermediates_.equal_range(BytesAsStringView(cert->normalized_issuer()));
   for (auto it = range.first; it != range.second; ++it) {
     issuers->push_back(it->second);
   }
diff --git a/pki/general_names.cc b/pki/general_names.cc
index cc7bf88..74874ba 100644
--- a/pki/general_names.cc
+++ b/pki/general_names.cc
@@ -114,7 +114,7 @@ std::unique_ptr<GeneralNames> GeneralNames::CreateFromValue(
   } else if (tag == der::ContextSpecificPrimitive(1)) {
     // rfc822Name                      [1]     IA5String,
     name_type = GENERAL_NAME_RFC822_NAME;
-    const std::string_view s = value.AsStringView();
+    const std::string_view s = BytesAsStringView(value);
     if (!bssl::string_util::IsAscii(s)) {
       errors->AddError(kRFC822NameNotAscii);
       return false;
@@ -123,7 +123,7 @@ std::unique_ptr<GeneralNames> GeneralNames::CreateFromValue(
   } else if (tag == der::ContextSpecificPrimitive(2)) {
     // dNSName                         [2]     IA5String,
     name_type = GENERAL_NAME_DNS_NAME;
-    const std::string_view s = value.AsStringView();
+    const std::string_view s = BytesAsStringView(value);
     if (!bssl::string_util::IsAscii(s)) {
       errors->AddError(kDnsNameNotAscii);
       return false;
@@ -152,7 +152,7 @@ std::unique_ptr<GeneralNames> GeneralNames::CreateFromValue(
   } else if (tag == der::ContextSpecificPrimitive(6)) {
     // uniformResourceIdentifier       [6]     IA5String,
     name_type = GENERAL_NAME_UNIFORM_RESOURCE_IDENTIFIER;
-    const std::string_view s = value.AsStringView();
+    const std::string_view s = BytesAsStringView(value);
     if (!bssl::string_util::IsAscii(s)) {
       errors->AddError(kURINotAscii);
       return false;
diff --git a/pki/input.cc b/pki/input.cc
index 82450ea..156d248 100644
--- a/pki/input.cc
+++ b/pki/input.cc
@@ -8,15 +8,7 @@
 
 namespace bssl::der {
 
-std::string Input::AsString() const {
-  return std::string(reinterpret_cast<const char *>(data_.data()),
-                     data_.size());
-}
-
-std::string_view Input::AsStringView() const {
-  return std::string_view(reinterpret_cast<const char *>(data_.data()),
-                          data_.size());
-}
+std::string Input::AsString() const { return std::string(AsStringView()); }
 
 bool operator==(Input lhs, Input rhs) {
   return MakeConstSpan(lhs) == MakeConstSpan(rhs);
diff --git a/pki/input.h b/pki/input.h
index 4a94110..30ce5d4 100644
--- a/pki/input.h
+++ b/pki/input.h
@@ -44,6 +44,8 @@ class OPENSSL_EXPORT Input {
   constexpr explicit Input(const uint8_t *data, size_t len)
       : data_(MakeConstSpan(data, len)) {}
 
+  // Deprecated: Use StringAsBytes.
+  //
   // Creates an Input from a std::string_view. The constructed Input is only
   // valid as long as |data| points to live memory. If constructed from, say, a
   // |std::string|, mutating the vector will invalidate the Input.
@@ -69,13 +71,17 @@ class OPENSSL_EXPORT Input {
   constexpr Input first(size_t len) const { return Input(data_.first(len)); }
   constexpr Input last(size_t len) const { return Input(data_.last(len)); }
 
+  // Deprecated: use BytesAsStringView and convert to std::string.
+  //
   // Returns a copy of the data represented by this object as a std::string.
   std::string AsString() const;
 
+  // Deprecated: Use ByteAsString. 
+  //
   // Returns a std::string_view pointing to the same data as the Input. The
   // resulting string_view must not outlive the data that was used to construct
   // this Input.
-  std::string_view AsStringView() const;
+  std::string_view AsStringView() const { return BytesAsStringView(data_); }
 
   // Deprecated: This class implicitly converts to bssl::Span<const uint8_t>.
   //
diff --git a/pki/ocsp.cc b/pki/ocsp.cc
index 2e5a98f..49076e9 100644
--- a/pki/ocsp.cc
+++ b/pki/ocsp.cc
@@ -696,7 +696,7 @@ std::shared_ptr<const ParsedCertificate> OCSPParseCertificate(
   //  (3) Has signed the OCSP response using its public key.
   for (const auto &responder_cert_tlv : response.certs) {
     std::shared_ptr<const ParsedCertificate> cur_responder_certificate =
-        OCSPParseCertificate(responder_cert_tlv.AsStringView());
+        OCSPParseCertificate(BytesAsStringView(responder_cert_tlv));
 
     // If failed parsing the certificate, keep looking.
     if (!cur_responder_certificate) {
diff --git a/pki/parse_certificate.cc b/pki/parse_certificate.cc
index 2de9f71..de910d3 100644
--- a/pki/parse_certificate.cc
+++ b/pki/parse_certificate.cc
@@ -872,7 +872,7 @@ bool ParseAuthorityInfoAccessURIs(
     // GeneralName ::= CHOICE {
     if (access_location_tag == der::ContextSpecificPrimitive(6)) {
       // uniformResourceIdentifier       [6]     IA5String,
-      std::string_view uri = access_location_value.AsStringView();
+      std::string_view uri = BytesAsStringView(access_location_value);
       if (!bssl::string_util::IsAscii(uri)) {
         return false;
       }
diff --git a/pki/parse_name.cc b/pki/parse_name.cc
index 87131a5..dca76b4 100644
--- a/pki/parse_name.cc
+++ b/pki/parse_name.cc
@@ -38,7 +38,7 @@ bool X509NameAttribute::ValueAsString(std::string *out) const {
     case der::kPrintableString:
       return der::ParsePrintableString(value, out);
     case der::kUtf8String:
-      *out = value.AsString();
+      *out = BytesAsStringView(value);
       return true;
     case der::kUniversalString:
       return der::ParseUniversalString(value, out);
@@ -53,7 +53,7 @@ bool X509NameAttribute::ValueAsStringWithUnsafeOptions(
     PrintableStringHandling printable_string_handling, std::string *out) const {
   if (printable_string_handling == PrintableStringHandling::kAsUTF8Hack &&
       value_tag == der::kPrintableString) {
-    *out = value.AsString();
+    *out = BytesAsStringView(value);
     return true;
   }
   return ValueAsString(out);
@@ -65,7 +65,7 @@ bool X509NameAttribute::ValueAsStringUnsafe(std::string *out) const {
     case der::kPrintableString:
     case der::kTeletexString:
     case der::kUtf8String:
-      *out = value.AsString();
+      *out = BytesAsStringView(value);
       return true;
     case der::kUniversalString:
       return der::ParseUniversalString(value, out);
diff --git a/pki/parse_name_unittest.cc b/pki/parse_name_unittest.cc
index 27989df..9858121 100644
--- a/pki/parse_name_unittest.cc
+++ b/pki/parse_name_unittest.cc
@@ -187,13 +187,13 @@ TEST(ParseNameTest, ValidName) {
   ASSERT_EQ(3u, atv.size());
   ASSERT_EQ(1u, atv[0].size());
   ASSERT_EQ(der::Input(kTypeCountryNameOid), atv[0][0].type);
-  ASSERT_EQ("US", atv[0][0].value.AsString());
+  ASSERT_EQ("US", BytesAsStringView(atv[0][0].value));
   ASSERT_EQ(1u, atv[1].size());
   ASSERT_EQ(der::Input(kTypeOrganizationNameOid), atv[1][0].type);
-  ASSERT_EQ("Google Inc.", atv[1][0].value.AsString());
+  ASSERT_EQ("Google Inc.", BytesAsStringView(atv[1][0].value));
   ASSERT_EQ(1u, atv[2].size());
   ASSERT_EQ(der::Input(kTypeCommonNameOid), atv[2][0].type);
-  ASSERT_EQ("Google Test CA", atv[2][0].value.AsString());
+  ASSERT_EQ("Google Test CA", BytesAsStringView(atv[2][0].value));
 }
 
 TEST(ParseNameTest, InvalidNameExtraData) {
diff --git a/pki/parse_values.cc b/pki/parse_values.cc
index b90ead3..8d07b3d 100644
--- a/pki/parse_values.cc
+++ b/pki/parse_values.cc
@@ -363,12 +363,12 @@ bool ParseGeneralizedTime(Input in, GeneralizedTime *value) {
 }
 
 bool ParseIA5String(Input in, std::string *out) {
-  for (char c : in.AsStringView()) {
-    if (static_cast<uint8_t>(c) > 127) {
+  for (uint8_t c : in) {
+    if (c > 127) {
       return false;
     }
   }
-  *out = in.AsString();
+  *out = BytesAsStringView(in);
   return true;
 }
 
@@ -379,23 +379,23 @@ bool ParseVisibleString(Input in, std::string *out) {
   // Also ITU-T X.691 says it much more clearly:
   // "for VisibleString [the range] is 32 to 126 ... For VisibleString .. all
   // the values in the range are present."
-  for (char c : in.AsStringView()) {
-    if (static_cast<uint8_t>(c) < 32 || static_cast<uint8_t>(c) > 126) {
+  for (uint8_t c : in) {
+    if (c < 32 || c > 126) {
       return false;
     }
   }
-  *out = in.AsString();
+  *out = BytesAsStringView(in);
   return true;
 }
 
 bool ParsePrintableString(Input in, std::string *out) {
-  for (char c : in.AsStringView()) {
+  for (uint8_t c : in) {
     if (!(OPENSSL_isalpha(c) || c == ' ' || (c >= '\'' && c <= ':') ||
           c == '=' || c == '?')) {
       return false;
     }
   }
-  *out = in.AsString();
+  *out = BytesAsStringView(in);
   return true;
 }
 
diff --git a/pki/path_builder.cc b/pki/path_builder.cc
index 98c6438..4bb5d2f 100644
--- a/pki/path_builder.cc
+++ b/pki/path_builder.cc
@@ -295,11 +295,11 @@ void CertIssuersIter::GetNextIssuer(IssuerEntry *out) {
 
 void CertIssuersIter::AddIssuers(ParsedCertificateList new_issuers) {
   for (std::shared_ptr<const ParsedCertificate> &issuer : new_issuers) {
-    if (present_issuers_.find(issuer->der_cert().AsStringView()) !=
+    if (present_issuers_.find(BytesAsStringView(issuer->der_cert())) !=
         present_issuers_.end()) {
       continue;
     }
-    present_issuers_.insert(issuer->der_cert().AsStringView());
+    present_issuers_.insert(BytesAsStringView(issuer->der_cert()));
 
     // Look up the trust for this issuer.
     IssuerEntry entry;
@@ -423,9 +423,9 @@ class CertIssuerIterPath {
     // Note that subject_alt_names_extension().value will be empty if the cert
     // had no SubjectAltName extension, so there is no need for a condition on
     // has_subject_alt_names().
-    return Key(cert->normalized_subject().AsStringView(),
-               cert->subject_alt_names_extension().value.AsStringView(),
-               cert->tbs().spki_tlv.AsStringView());
+    return Key(BytesAsStringView(cert->normalized_subject()),
+               BytesAsStringView(cert->subject_alt_names_extension().value),
+               BytesAsStringView(cert->tbs().spki_tlv));
   }
 
   std::vector<std::unique_ptr<CertIssuersIter>> cur_path_;
diff --git a/pki/trust_store_in_memory.cc b/pki/trust_store_in_memory.cc
index 3da1b8d..1c77b63 100644
--- a/pki/trust_store_in_memory.cc
+++ b/pki/trust_store_in_memory.cc
@@ -47,7 +47,8 @@ void TrustStoreInMemory::AddCertificateWithUnspecifiedTrust(
 
 void TrustStoreInMemory::SyncGetIssuersOf(const ParsedCertificate *cert,
                                           ParsedCertificateList *issuers) {
-  auto range = entries_.equal_range(cert->normalized_issuer().AsStringView());
+  auto range =
+      entries_.equal_range(BytesAsStringView(cert->normalized_issuer()));
   for (auto it = range.first; it != range.second; ++it) {
     issuers->push_back(it->second.cert);
   }
@@ -55,7 +56,7 @@ void TrustStoreInMemory::SyncGetIssuersOf(const ParsedCertificate *cert,
 
 CertificateTrust TrustStoreInMemory::GetTrust(const ParsedCertificate *cert) {
   // Check SPKI distrust first.
-  if (distrusted_spkis_.find(cert->tbs().spki_tlv.AsString()) !=
+  if (distrusted_spkis_.find(BytesAsStringView(cert->tbs().spki_tlv)) !=
       distrusted_spkis_.end()) {
     return CertificateTrust::ForDistrusted();
   }
@@ -80,13 +81,13 @@ void TrustStoreInMemory::AddCertificate(
   entry.trust = trust;
 
   // TODO(mattm): should this check for duplicate certificates?
-  entries_.insert(
-      std::make_pair(entry.cert->normalized_subject().AsStringView(), entry));
+  entries_.emplace(BytesAsStringView(entry.cert->normalized_subject()), entry);
 }
 
 const TrustStoreInMemory::Entry *TrustStoreInMemory::GetEntry(
     const ParsedCertificate *cert) const {
-  auto range = entries_.equal_range(cert->normalized_subject().AsStringView());
+  auto range =
+      entries_.equal_range(BytesAsStringView(cert->normalized_subject()));
   for (auto it = range.first; it != range.second; ++it) {
     if (cert == it->second.cert.get() ||
         cert->der_cert() == it->second.cert->der_cert()) {
diff --git a/pki/trust_store_in_memory.h b/pki/trust_store_in_memory.h
index 7e5530d..4da26cd 100644
--- a/pki/trust_store_in_memory.h
+++ b/pki/trust_store_in_memory.h
@@ -89,7 +89,7 @@ class OPENSSL_EXPORT TrustStoreInMemory : public TrustStore {
   std::unordered_multimap<std::string_view, Entry> entries_;
 
   // Set of distrusted SPKIs.
-  std::set<std::string> distrusted_spkis_;
+  std::set<std::string, std::less<>> distrusted_spkis_;
 
   // Returns the `Entry` matching `cert`, or `nullptr` if not in the trust
   // store.
diff --git a/pki/trust_store_in_memory_unittest.cc b/pki/trust_store_in_memory_unittest.cc
index 92c3bb8..4292675 100644
--- a/pki/trust_store_in_memory_unittest.cc
+++ b/pki/trust_store_in_memory_unittest.cc
@@ -77,7 +77,8 @@ TEST_F(TrustStoreInMemoryTest, OneRootTrusted) {
 
 TEST_F(TrustStoreInMemoryTest, DistrustBySPKI) {
   TrustStoreInMemory in_memory;
-  in_memory.AddDistrustedCertificateBySPKI(newroot_->tbs().spki_tlv.AsString());
+  in_memory.AddDistrustedCertificateBySPKI(
+      std::string(BytesAsStringView(newroot_->tbs().spki_tlv)));
 
   // newroot_ is distrusted.
   CertificateTrust trust = in_memory.GetTrust(newroot_.get());
@@ -98,7 +99,8 @@ TEST_F(TrustStoreInMemoryTest, DistrustBySPKI) {
 TEST_F(TrustStoreInMemoryTest, DistrustBySPKIOverridesTrust) {
   TrustStoreInMemory in_memory;
   in_memory.AddTrustAnchor(newroot_);
-  in_memory.AddDistrustedCertificateBySPKI(newroot_->tbs().spki_tlv.AsString());
+  in_memory.AddDistrustedCertificateBySPKI(
+      std::string(BytesAsStringView(newroot_->tbs().spki_tlv)));
 
   // newroot_ is distrusted.
   CertificateTrust trust = in_memory.GetTrust(newroot_.get());