-
Type: Bug
-
Resolution: Fixed
-
Priority: Major - P3
-
Affects Version/s: None
-
Component/s: None
-
Labels:None
-
Fully Compatible
-
ALL
-
Service arch 2020-11-02
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}; }