SERVER-27209 Eliminate dangerous BSONElement string extraction methods

- Fix: Change return type of BSONObj::getStringField to include size
 (StringData vs. char*). A char* only contains the data with an ending NULL
 termination. Whereas a StringData contains data + size so caller knows how
 to interpret data if there are embedded NULLs.

- Cleanup: Remove old tag - CachedSizeTag - that disambiguated BSONElement ctors.
 A dangling reference to 'maxLen' in a comment led me to this historical issue.
 $ git log -S'maxLen' -- src/mongo/bson/bsonelement.h
 commit 0d38ef5
 Author: Mathias Stearn mathias@10gen.com
 Date: Tue Dec 19 14:23:08 2017 -0500

 SERVER-32302 Compute BSONElement sizes eagerly

- Test: Add tests for NULL bytes being returned by getStringField

  - $ ninja -j400 +bson_obj_test

- Cleanup: Move BSONElement::valuestr() from public to private

- Cleanup: Remove BSONElement::valuestrsafe()

- Cleanup: Remove all external callers of valuestr/valuestrsafe and cleanup
 their callsites with better alternatives.

- Cleanup: Make multi-line BSONElement & BSONObj public API comments
 conform to style guidelines

- Nit: Fix spelling in a comment

6 files changed, 9 insertions, 8 deletions

diff --git a/src/mongo/client/authenticate.cpp b/src/mongo/client/authenticate.cpp
index 2c26e03953e..3c1b7ad6535 100644
--- a/src/mongo/client/authenticate.cpp
+++ b/src/mongo/client/authenticate.cpp
@@ -112,7 +112,7 @@ StatusWith<OpMsgRequest> createX509AuthCmd(const BSONObj& params, StringData cli
     if (username != clientName.toString()) {
         StringBuilder message;
         message << "Username \"";
-        message << params[saslCommandUserFieldName].valuestr();
+        message << params[saslCommandUserFieldName].valueStringData();
         message << "\" does not match the provided client certificate user \"";
         message << clientName.toString() << "\"";
         return {ErrorCodes::AuthenticationFailed, message.str()};
diff --git a/src/mongo/client/dbclient_base.cpp b/src/mongo/client/dbclient_base.cpp
index c4b38ddb3e3..98c49806f40 100644
--- a/src/mongo/client/dbclient_base.cpp
+++ b/src/mongo/client/dbclient_base.cpp
@@ -97,7 +97,8 @@ bool DBClientBase::isOk(const BSONObj& o) {
 
 bool DBClientBase::isNotPrimaryErrorString(const BSONElement& e) {
     return e.type() == String &&
-        (str::contains(e.valuestr(), "not primary") || str::contains(e.valuestr(), "not master"));
+        (str::contains(e.valueStringData(), "not primary") ||
+         str::contains(e.valueStringData(), "not master"));
 }
 
 void DBClientBase::setRequestMetadataWriter(rpc::RequestMetadataWriter writer) {
diff --git a/src/mongo/client/replica_set_monitor_integration_test.cpp b/src/mongo/client/replica_set_monitor_integration_test.cpp
index 3f2af0e8ce4..383411d0d61 100644
--- a/src/mongo/client/replica_set_monitor_integration_test.cpp
+++ b/src/mongo/client/replica_set_monitor_integration_test.cpp
@@ -141,7 +141,7 @@ public:
         ASSERT_OK(cmdStatus);
         const auto shards = res.data["shards"].Array();
         ASSERT_FALSE(shards.empty());
-        return shards.front().embeddedObject().getStringField("host");
+        return shards.front().embeddedObject().getStringField("host").toString();
     }
 
 protected:
diff --git a/src/mongo/client/sdam/sdam_json_test_runner.cpp b/src/mongo/client/sdam/sdam_json_test_runner.cpp
index 749585cf01c..b3fdcba09f0 100644
--- a/src/mongo/client/sdam/sdam_json_test_runner.cpp
+++ b/src/mongo/client/sdam/sdam_json_test_runner.cpp
@@ -486,7 +486,7 @@ private:
             _jsonTest = fromjson(json.str());
         }
 
-        _testName = _jsonTest.getStringField("description");
+        _testName = _jsonTest.getStringField("description").toString();
         _testUri = uassertStatusOK(mongo::MongoURI::parse(_jsonTest["uri"].String()));
 
         _replicaSetName = _testUri.getOption("replicaSet");
diff --git a/src/mongo/client/sdam/server_description.cpp b/src/mongo/client/sdam/server_description.cpp
index 042ad352c28..babe5d1a777 100644
--- a/src/mongo/client/sdam/server_description.cpp
+++ b/src/mongo/client/sdam/server_description.cpp
@@ -129,7 +129,7 @@ void ServerDescription::saveHosts(const BSONObj response) {
 void ServerDescription::saveTags(BSONObj tagsObj) {
     const auto keys = tagsObj.getFieldNames<std::set<std::string>>();
     for (const auto& key : keys) {
-        _tags[key] = tagsObj.getStringField(key);
+        _tags[key] = tagsObj.getStringField(key).toString();
     }
 }
 
diff --git a/src/mongo/client/sdam/server_selection_json_test_runner.cpp b/src/mongo/client/sdam/server_selection_json_test_runner.cpp
index 72379e27165..a8ebedef4c7 100644
--- a/src/mongo/client/sdam/server_selection_json_test_runner.cpp
+++ b/src/mongo/client/sdam/server_selection_json_test_runner.cpp
@@ -162,7 +162,7 @@ private:
         // Only create the initial server description if the original avg rtt is not "NULL". If it
         // is, the test case is meant to mimic creating the first ServerDescription which we will do
         // above.
-        std::string origRttAsString = _jsonTest.getStringField("avg_rtt_ms");
+        std::string origRttAsString = _jsonTest.getStringField("avg_rtt_ms").toString();
         if (origRttAsString.compare("NULL") != 0) {
             auto serverDescription = ServerDescriptionBuilder()
                                          .withAddress(HostAndPort("dummy"))
@@ -286,7 +286,7 @@ private:
         // lowercased keywords. Also, change the key "tags_set" to "tags".
         // This can throw for test cases that have invalid read preferences.
         auto readPrefObj = _jsonTest.getObjectField("read_preference");
-        std::string mode = readPrefObj.getStringField("mode");
+        std::string mode = readPrefObj.getStringField("mode").toString();
         mode[0] = ctype::toLower(mode[0]);
         auto tagSetsObj = readPrefObj["tag_sets"];
         auto tags = tagSetsObj ? BSONArray(readPrefObj["tag_sets"].Obj()) : BSONArray();
@@ -364,7 +364,7 @@ private:
             auto tagsObj = server.getObjectField("tags");
             const auto keys = tagsObj.getFieldNames<std::set<std::string>>();
             for (const auto& key : keys) {
-                serverDescription.withTag(key, tagsObj.getStringField(key));
+                serverDescription.withTag(key, tagsObj.getStringField(key).toString());
             }
 
             serverDescriptions.push_back(serverDescription.instance());