Description
This is a followup to #130. with my apologies for ignoring this for so long.
A very recent simple-binary-encoding build from the CME templates generated MDIncrementalRefreshBook46.h
, which does indeed contain some bounds checking, but I think it's incomplete. In the generated file, there's a class MDIncrementalRefreshBook46::NoOrderIDEntries
, and that contains:
inline void wrapForDecode(
char *buffer,
std::uint64_t *pos,
const std::uint64_t actingVersion,
const std::uint64_t bufferLength)
{
GroupSize8Byte dimensions(buffer, *pos, bufferLength, actingVersion);
m_buffer = buffer;
m_bufferLength = bufferLength;
m_blockLength = dimensions.blockLength();
m_count = dimensions.numInGroup();
m_index = 0;
m_actingVersion = actingVersion;
m_initialPosition = *pos;
m_positionPtr = pos;
*m_positionPtr = *m_positionPtr + 8;
}
And next() looks like this:
inline NoOrderIDEntries &next()
{
if (m_index >= m_count)
{
throw std::runtime_error("index >= count [E108]");
}
m_offset = *m_positionPtr;
if (SBE_BOUNDS_CHECK_EXPECT(((m_offset + m_blockLength) > m_bufferLength), false))
{
throw std::runtime_error("buffer too short for next group index [E108]");
}
*m_positionPtr = m_offset + m_blockLength;
++m_index;
return *this;
}
dimensions.blockLength()
is an untrusted value, although it happens to be an sbe_uint16_t despite being stored in an sbe_uint64_t field, so it's constrained. This dodges a bullet: otherwise m_offset + m_blockLength
could overflow. I didn't check whether other classes might not be so lucky, though.
But there's a more fundamental issue: nothing seems to check that m_blockLength is appropriate. For example:
SBE_NODISCARD std::uint64_t orderID() const SBE_NOEXCEPT
{
std::uint64_t val;
std::memcpy(&val, m_buffer + m_offset + 0, sizeof(std::uint64_t));
return SBE_LITTLE_ENDIAN_ENCODE_64(val);
}
If there are fewer than 8 bytes in the buffer, this will overflow. But m_blockLength could easily be 0 or 1 or 7, and the bounds check in next()
would pass with fewer than 8 bytes in the buffer.
This is extra complicated because of the actingVersion mechanism:
SBE_NODISCARD std::uint64_t mDOrderPriority() const SBE_NOEXCEPT
{
if (m_actingVersion < 7)
{
return UINT64_C(0xffffffffffffffff);
}
std::uint64_t val;
std::memcpy(&val, m_buffer + m_offset + 8, sizeof(std::uint64_t));
return SBE_LITTLE_ENDIAN_ENCODE_64(val);
}
blockLength == 8
is valid for NoOrderIDEntries
if actingVersion < 7, but if actingVersion >= 7, then this requires a blockLength
of at least 16! And checking that from user code (as opposed to the generated headers) would get very awkward very quickly.