Fix server crash when read a malformed replication segment#9055
Conversation
| const auto length = getInt32(); | ||
| const ULONG length = static_cast<ULONG>(getInt32()); | ||
|
|
||
| if (m_data + length > m_end) |
There was a problem hiding this comment.
It is better to leave length as is and use a correct condition instead: (length <= 0 || m_end - m_data < length).
There was a problem hiding this comment.
I thought about this, but in the case of an unsigned string, the length might be longer. In what case might it be useful to have a negative string length?
There was a problem hiding this comment.
Ok, back length as is in getString.
Also add check length in defineAtom.
There was a problem hiding this comment.
We should ask @dyemanov why he chose signed result for getIntXX functions.
May be you are right about maximum string length. In this case the right condition avoiding overflow on the end of address space still would be (m_end - m_data < length). Only the last gigabyte is reserved by OSes IIRC so if the buffer by change is just below it, m_data + length may be a little above zero causing false negative.
There was a problem hiding this comment.
Maybe zero-sized string is allowed. @dyemanov ?
There was a problem hiding this comment.
We should ask @dyemanov why he chose signed result for getIntXX functions.
I tried to make getters generic, as I couldn't predict whether signed numbers could ever appear inside the replication protocol or not. Given that numbers > 2^31 are not expected this looked safe.
… was. Also add a length check to the defineAtom function.
| const auto pos = getInt32(); | ||
| const ULONG pos = static_cast<ULONG>(getInt32()); | ||
|
|
||
| if (pos >= m_atoms.getCount()) |
There was a problem hiding this comment.
I'd prefer explicit checks with a signed pos:
if (pos < 0 || pos >= m_atoms.getCount())
| const auto length = getInt32(); | ||
|
|
||
| if (m_data + length > m_end) | ||
| if (length <= 0 || m_end - m_data < length) |
There was a problem hiding this comment.
I'd allow zero-length strings even if it looks impossible now.
| const auto pos = getInt32(); | ||
| const ULONG pos = static_cast<ULONG>(getInt32()); | ||
|
|
||
| if (pos >= m_atoms.getCount()) |
| void defineAtom() | ||
| { | ||
| const auto length = getByte(); | ||
| if (length <= 0) |
3 participants
If a segment has some damage and it falls on the length of a string or the position of an atom, then the server crash with SIGSEGV, instead of output an correct error