Details
-
Bug
-
Resolution: Fixed
-
Major - P3
-
None
-
None
-
None
-
Fully Compatible
-
ALL
-
Service arch 2020-11-02
Description
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}; |
}
|