-
Type:
Bug
-
Resolution: Fixed
-
Priority:
Major - P3
-
Affects Version/s: None
-
Component/s: None
-
None
-
Fully Compatible
-
ALL
-
Service arch 2020-11-02
-
None
-
None
-
None
-
None
-
None
-
None
-
None
I could be wrong, but I think BSONElement::_binDataVector uses incorrect bounds for the std::vector<uint8_t> result constructor when retrieving the data for a bindata-type-2 (the ByteArrayDeprecated bindata subtype).
std::vector<uint8_t> _binDataVector() const {
if (binDataType() != ByteArrayDeprecated) {
return std::vector<uint8_t>(reinterpret_cast<const uint8_t*>(value()) + 5,
reinterpret_cast<const uint8_t*>(value()) + 5 +
valuestrsize());
} else {
// Skip the extra int32 size
return std::vector<uint8_t>(reinterpret_cast<const uint8_t*>(value()) + 4,
reinterpret_cast<const uint8_t*>(value()) + 4 +
valuestrsize() - 4);
I believe the +4 offsets in the else clause are incorrect, and it needs to be:
return std::vector<uint8_t>(reinterpret_cast<const uint8_t*>(value()) + 9,
reinterpret_cast<const uint8_t*>(value()) + 9 +
valuestrsize() - 4);
The difference between ByteArrayDeprecated (binDataType==2) and other binData payloads is that it has a wasted redundant 4-byte length field. So we need to skip over that when returning the binData payload. I think the intend of the +4 was to skip over this length, but it's incorrectly specified here. It should be that we skip 4 beyond what we'd do for other binData types, and they already skip 5.
I'll note also that valuestrsize() might be smaller than 4 if the BSON is malformed, so we should skip 4 bytes, but no more than valuestrsize(), to ensure that the vector arguments are a valid range.
In other words,
std::vector<uint8_t> _binDataVector() const {
auto first = reinterpret_cast<const uint8_t*>(value()) + 5;
auto last = first + valuestrsize();
if (binDataType() == ByteArrayDeprecated)
first += std::min<size_t>(4, last - first); // skip extra int32 size.
return {first, last};
}