Skip to content

Commit

Permalink
Revert "SERVER-95279 Check for embedded NUL bytes when appending fiel…
Browse files Browse the repository at this point in the history 
…d names (#27966)" (#28032)

GitOrigin-RevId: 60b3270bc4f165da07b1f909e09a20de73a2283a
  • Loading branch information
@auto-revert-app
auto-revert-app[bot] authored and MongoDB Bot committed Oct 12, 2024
1 parent 0c2b5b0 commit 10bfb7f
Show file tree
Hide file tree
Showing 40 changed files with 119 additions and 286 deletions.
1 change: 0 additions & 1 deletion src/mongo/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,6 @@ mongo_cc_library(
"//src/mongo/util:stacktrace_windows.h",
"//src/mongo/util:static_immortal.h",
"//src/mongo/util:str.h",
"//src/mongo/util:str_basic.h",
"//src/mongo/util:str_escape.h",
"//src/mongo/util:string_map.h",
"//src/mongo/util:synchronized_value.h",
Expand Down
11 changes: 9 additions & 2 deletions src/mongo/base/string_data.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ class MONGO_GSL_POINTER StringData {
* Used where string length is not known in advance.
* 'c' must be null or point to a null-terminated string.
*/
constexpr StringData(const char* c)
: StringData{c ? std::string_view{c} : std::string_view{}} {}
StringData(const char* c)
: StringData{std::string_view{c, (c && c[0] != '\0') ? std::strlen(c) : 0}} {}

StringData(const std::string& s) : StringData{std::string_view{s}} {}

Expand Down Expand Up @@ -352,6 +352,13 @@ class MONGO_GSL_POINTER StringData {
});
}

void copyTo(char* dest, bool includeEndingNull) const {
if (!empty())
copy(dest, size());
if (includeEndingNull)
dest[size()] = 0;
}

private:
explicit constexpr StringData(std::string_view sv) : _sv{sv} {}

Expand Down
8 changes: 4 additions & 4 deletions src/mongo/bson/bson_validate_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ using std::unique_ptr;
void appendInvalidStringElement(const char* fieldName, BufBuilder* bb) {
// like a BSONObj string, but without a NUL terminator.
bb->appendChar(String);
bb->appendCStr(fieldName);
bb->appendStr(fieldName, /*withNUL*/ true);
bb->appendNum(4);
bb->appendStrBytes("asdf"); // Missing required final NUL.
bb->appendStr("asdf", /*withNUL*/ false);
}

/**
Expand All @@ -90,7 +90,7 @@ void appendInvalidStringElement(const char* fieldName, BufBuilder* bb) {
*/
void appendObjectNameAndSize(const char* fieldName, BufBuilder* bb, int objectSize) {
bb->appendChar(Object);
bb->appendCStr(fieldName);
bb->appendStr(fieldName, /*withNUL*/ true);
bb->appendNum(objectSize);
}

Expand Down Expand Up @@ -689,7 +689,7 @@ TEST(BSONValidateFast, StringHasSomething) {
BufBuilder bb;
BSONObjBuilder ob(bb);
bb.appendChar(String);
bb.appendCStr("x");
bb.appendStr("x", /*withNUL*/ true);
bb.appendNum(0);
const BSONObj x = ob.done();
ASSERT_EQUALS(5 // overhead
Expand Down
2 changes: 1 addition & 1 deletion src/mongo/bson/bsonobjbuilder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ Derived& BSONObjBuilderBase<Derived, B>::appendMaxForType(StringData fieldName,
template <class Derived, class B>
Derived& BSONObjBuilderBase<Derived, B>::appendDate(StringData fieldName, Date_t dt) {
_b.appendNum((char)Date);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendNum(dt.toMillisSinceEpoch());
return static_cast<Derived&>(*this);
}
Expand Down
62 changes: 31 additions & 31 deletions src/mongo/bson/bsonobjbuilder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,15 +178,15 @@ class BSONObjBuilderBase {
// do not append eoo, that would corrupt us. the builder auto appends when done() is called.
MONGO_verify(!e.eoo());
_b.appendNum((char)e.type());
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendBuf((void*)e.value(), e.valuesize());
return static_cast<Derived&>(*this);
}

/** add a subobject as a member */
Derived& append(StringData fieldName, BSONObj subObj) {
_b.appendNum((char)Object);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendBuf((void*)subObj.objdata(), subObj.objsize());
return static_cast<Derived&>(*this);
}
Expand All @@ -201,7 +201,7 @@ class BSONObjBuilderBase {
MONGO_verify(size > 4 && size < 100000000);

_b.appendNum((char)Object);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendBuf((void*)objdata, size);
return static_cast<Derived&>(*this);
}
Expand All @@ -219,7 +219,7 @@ class BSONObjBuilderBase {
*/
B& subobjStart(StringData fieldName) {
_b.appendNum((char)Object);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
return _b;
}

Expand All @@ -228,7 +228,7 @@ class BSONObjBuilderBase {
*/
Derived& appendArray(StringData fieldName, const BSONObj& subObj) {
_b.appendNum((char)Array);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendBuf((void*)subObj.objdata(), subObj.objsize());

return static_cast<Derived&>(*this);
Expand All @@ -241,14 +241,14 @@ class BSONObjBuilderBase {
the subarray's body */
B& subarrayStart(StringData fieldName) {
_b.appendNum((char)Array);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
return _b;
}

/** Append a boolean element */
Derived& appendBool(StringData fieldName, int val) {
_b.appendNum((char)Bool);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendNum((char)(val ? 1 : 0));
return static_cast<Derived&>(*this);
}
Expand All @@ -258,7 +258,7 @@ class BSONObjBuilderBase {
Derived& append(StringData fieldName, const T& n) {
constexpr BSONType type = BSONObjAppendFormat<T>::value;
_b.appendNum(static_cast<char>(type));
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
if constexpr (type == Bool) {
_b.appendNum(static_cast<char>(n));
} else if constexpr (type == NumberInt) {
Expand Down Expand Up @@ -309,7 +309,7 @@ class BSONObjBuilderBase {
*/
Derived& appendOID(StringData fieldName, OID* oid = nullptr, bool generateIfBlank = false) {
_b.appendNum((char)jstOID);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
if (oid)
_b.appendBuf(oid->view().view(), OID::kOIDSize);
else {
Expand All @@ -330,7 +330,7 @@ class BSONObjBuilderBase {
*/
Derived& append(StringData fieldName, OID oid) {
_b.appendNum((char)jstOID);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendBuf(oid.view().view(), OID::kOIDSize);
return static_cast<Derived&>(*this);
}
Expand All @@ -349,7 +349,7 @@ class BSONObjBuilderBase {
*/
Derived& appendTimeT(StringData fieldName, time_t dt) {
_b.appendNum((char)Date);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendNum(static_cast<unsigned long long>(dt) * 1000);
return static_cast<Derived&>(*this);
}
Expand All @@ -368,9 +368,9 @@ class BSONObjBuilderBase {
*/
Derived& appendRegex(StringData fieldName, StringData regex, StringData options = "") {
_b.appendNum((char)RegEx);
_b.appendCStr(fieldName);
_b.appendCStr(regex);
_b.appendCStr(options);
_b.appendStr(fieldName);
_b.appendStr(regex);
_b.appendStr(options);

return static_cast<Derived&>(*this);
}
Expand All @@ -381,9 +381,9 @@ class BSONObjBuilderBase {

Derived& appendCode(StringData fieldName, StringData code) {
_b.appendNum((char)Code);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendNum((int)code.size() + 1);
_b.appendStrBytesAndNul(code);
_b.appendStr(code);
return static_cast<Derived&>(*this);
}

Expand All @@ -395,7 +395,7 @@ class BSONObjBuilderBase {
@param sz size includes terminating null character */
Derived& append(StringData fieldName, const char* str, int sz) {
_b.appendNum((char)String);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendNum((int)sz);
_b.appendBuf(str, sz);

Expand All @@ -408,17 +408,17 @@ class BSONObjBuilderBase {
/** Append a string element */
Derived& append(StringData fieldName, StringData str) {
_b.appendNum((char)String);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendNum((int)str.size() + 1);
_b.appendStrBytesAndNul(str);
_b.appendStr(str, true);
return static_cast<Derived&>(*this);
}

Derived& appendSymbol(StringData fieldName, StringData symbol) {
_b.appendNum((char)Symbol);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendNum((int)symbol.size() + 1);
_b.appendStrBytesAndNul(symbol);
_b.appendStr(symbol);
return static_cast<Derived&>(*this);
}

Expand All @@ -429,21 +429,21 @@ class BSONObjBuilderBase {
/** Append a Null element to the object */
Derived& appendNull(StringData fieldName) {
_b.appendNum((char)jstNULL);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);

return static_cast<Derived&>(*this);
}

// Append an element that is less than all other keys.
Derived& appendMinKey(StringData fieldName) {
_b.appendNum((char)MinKey);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
return static_cast<Derived&>(*this);
}
// Append an element that is greater than all other keys.
Derived& appendMaxKey(StringData fieldName) {
_b.appendNum((char)MaxKey);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
return static_cast<Derived&>(*this);
}

Expand All @@ -464,9 +464,9 @@ class BSONObjBuilderBase {
*/
Derived& appendDBRef(StringData fieldName, StringData ns, const OID& oid) {
_b.appendNum((char)DBRef);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendNum((int)ns.size() + 1);
_b.appendStrBytesAndNul(ns);
_b.appendStr(ns);
_b.appendBuf(oid.view().view(), OID::kOIDSize);

return static_cast<Derived&>(*this);
Expand All @@ -485,7 +485,7 @@ class BSONObjBuilderBase {
*/
Derived& appendBinData(StringData fieldName, int len, BinDataType type, const void* data) {
_b.appendNum((char)BinData);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendNum(len);
_b.appendNum((char)type);
_b.appendBuf(data, len);
Expand All @@ -505,7 +505,7 @@ class BSONObjBuilderBase {
*/
Derived& appendBinDataArrayDeprecated(const char* fieldName, const void* data, int len) {
_b.appendNum((char)BinData);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendNum(len + 4);
_b.appendNum((char)0x2);
_b.appendNum(len);
Expand All @@ -519,10 +519,10 @@ class BSONObjBuilderBase {
*/
Derived& appendCodeWScope(StringData fieldName, StringData code, const BSONObj& scope) {
_b.appendNum((char)CodeWScope);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
_b.appendNum((int)(4 + 4 + code.size() + 1 + scope.objsize()));
_b.appendNum((int)code.size() + 1);
_b.appendStrBytesAndNul(code);
_b.appendStr(code);
_b.appendBuf((void*)scope.objdata(), scope.objsize());

return static_cast<Derived&>(*this);
Expand All @@ -534,7 +534,7 @@ class BSONObjBuilderBase {

Derived& appendUndefined(StringData fieldName) {
_b.appendNum((char)Undefined);
_b.appendCStr(fieldName);
_b.appendStr(fieldName);
return static_cast<Derived&>(*this);
}

Expand Down
2 changes: 1 addition & 1 deletion src/mongo/bson/timestamp.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ class Timestamp {
// No endian conversions needed, since we store in-memory representation
// in little endian format, regardless of target endian.
builder.appendNum(static_cast<char>(bsonTimestamp));
builder.appendCStr(fieldName);
builder.appendStr(fieldName);
builder.appendNum(asULL());
}
BSONObj toBSON() const;
Expand Down
1 change: 0 additions & 1 deletion src/mongo/bson/util/SConscript
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ env.CppUnitTest(
source=[
"bson_check_test.cpp",
"bson_extract_test.cpp",
"builder_test.cpp",
],
LIBDEPS=[
"$BUILD_DIR/mongo/base",
Expand Down
36 changes: 4 additions & 32 deletions src/mongo/bson/util/builder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@
#include "mongo/util/itoa.h"
#include "mongo/util/shared_buffer.h"
#include "mongo/util/shared_buffer_fragment.h"
#include "mongo/util/str_basic.h"
#include "mongo/util/tracking_allocator.h"

namespace mongo {
Expand Down Expand Up @@ -425,36 +424,9 @@ class BasicBufBuilder {
appendBuf(&s, sizeof(T));
}

/**
* Appends the raw bytes of str with no NUL terminator.
*/
void appendStrBytes(StringData str) {
str.copy(grow(str.size()), str.size());
}

/**
* Appends the raw bytes of str followed by a final NUL byte.
*
* WARNING: only use this method for formats with explicit string lengths where the NUL byte is
* not used to find the end. This method does not check for embedded NUL bytes, so they can
* trick a parser into thinking the string has ended. Use appendCStr() instead for that use
* case.
*/
void appendStrBytesAndNul(StringData str) {
auto dest = grow(str.size() + 1);
dest += str.copy(dest, str.size());
*dest = '\0';
}

/**
* Appends the raw bytes of str followed by a final NUL byte, throwing if str already has an
* embedded NUL byte.
*
* This method is intended to pair with BufReader::readCStr() on the parse side.
*/
void appendCStr(StringData str) {
str::uassertNoEmbeddedNulBytes(str);
appendStrBytesAndNul(str);
void appendStr(StringData str, bool includeEndingNull = true) {
const size_t len = str.size() + (includeEndingNull ? 1 : 0);
str.copyTo(grow(len), includeEndingNull);
}

/** Returns the length of data in the current buffer */
Expand Down Expand Up @@ -833,7 +805,7 @@ class StringBuilderImpl {
}

void append(StringData str) {
_buf.appendStrBytes(str);
str.copyTo(_buf.grow(str.size()), false);
}

void reset(int maxSize = 0) {
Expand Down
Loading

0 comments on commit 10bfb7f

Please sign in to comment.