Bugzilla – Bug 2044
Buffer::Iterator::ReadNtohU16() and Buffer::Iterator::ReadNtohU32 are not implemented correctly
Last modified: 2015-01-30 19:32:54 EST
It seems that theses three functions, Buffer::Iterator::ReadNtohU16(), Buffer::Iterator::ReadNtohU32(), and Buffer::Iterator::ReadNtohU64(), will not behave accordingly. Here's the problem. To make it obvious, just consider these two cases. 1). When m_current > m_zeroEnd ReadNtohU16() and ReadNtohU32() will read from m_data[m_current]. But ReadNtohU64() will invoke eight ReadU8() which in turn will invoke PeekU8(). But PeekU8() will read from m_data[m_current - (m_zeroEnd - m_zeroStart)]. Because the zero area bytes are usually used to represent the application payload, and the state invariant of m_start <= m_zeroStart <= m_zeroEnd <= m_end, they should be reading from different locations. 2) When m_zeroStart < m_current < m_zeroEnd ReadNtohU16() and ReadNtohU32() will increase m_current too much. Take ReadNtohU16() as a example. It invokes SlowReadNtohU16() and then increase m_current by 2. But SlowReadNtohU16() invokes ReadU8() twice, which in turn invokes PeekU8() and increases m_current by 1. Eventually, ReadNtohU16() will increase m_current by 4 instead of 2. This looks like a bug, but it doesn't hurt too much. Because there is few manipulation on application payload or footer during packet processing.
Point 2 is not a bug. Point 1 is worth triple checking. In ReadNtohU16: [...] else if (m_current >= m_zeroEnd) { buffer = &m_data[m_current]; } In PeekU8: else // same condition as (m_current >= m_zeroEnd) { uint8_t data = m_data[m_current - (m_zeroEnd-m_zeroStart)]; return data; } This may lead to a past-array reading.
(In reply to Tommaso Pecorella from comment #1) > Point 2 is not a bug. I agree, the method returns before m_current is incremented when the slow path processing occurs. > > Point 1 is worth triple checking. > In ReadNtohU16: > [...] > else if (m_current >= m_zeroEnd) > { > buffer = &m_data[m_current]; > } > > In PeekU8: > else // same condition as (m_current >= m_zeroEnd) > { > uint8_t data = m_data[m_current - (m_zeroEnd-m_zeroStart)]; > return data; > } > > This may lead to a past-array reading. Yes, this fix is needed in both methods: - buffer = &m_data[m_current]; + buffer = &m_data[m_current - (m_zeroEnd - m_zeroStart)]; pushed in changeset 11192:441c905aa900