IPC::Decoder::isInvalid() should be renamed to isValid(). Negative logic is more difficult to reason about.
Created attachment 397519 [details] Patch v1
Comment on attachment 397519 [details] Patch v1 Apparently there is usage of isInvalid() in assert statements!
Created attachment 397529 [details] Patch v2
Comment on attachment 397529 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=397529&action=review > Source/WebKit/ChangeLog:3 > + IPC::Decoder::isInvalid() should be renamed to isValid() You don’t say why > Source/WebKit/Platform/IPC/Decoder.h:79 > - bool isInvalid() const > + bool isValid() const > { > // (m_bufferPos == m_bufferEnd) is a valid state for decoding if the last parameter > // is a variable length byte array and its size == 0. > - return m_bufferPos < m_buffer || m_bufferPos > m_bufferEnd; > + return m_bufferPos >= m_buffer && m_bufferPos <= m_bufferEnd; > } This is a very peculiar function. Why isn’t it just a null check? If we run off the end of the buffer, the damage has been done. Returning false from isValid doesn’t seem to do much good.
Comment on attachment 397529 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=397529&action=review >> Source/WebKit/ChangeLog:3 >> + IPC::Decoder::isInvalid() should be renamed to isValid() > > You don’t say why I'll add this to the ChangeLog (see Comment #0): Negative logic is more difficult to reason about. >> Source/WebKit/Platform/IPC/Decoder.h:79 >> } > > This is a very peculiar function. Why isn’t it just a null check? If we run off the end of the buffer, the damage has been done. Returning false from isValid doesn’t seem to do much good. Tracking this issue in Bug 211006.
Created attachment 397533 [details] Patch for landing
Committed r260704: <https://trac.webkit.org/changeset/260704> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397533 [details].
<rdar://problem/62371804>