Bug 211000 – IPC::Decoder::isInvalid() should be renamed to isValid()

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

IPC::Decoder::isInvalid() should be renamed to isValid().

Negative logic is more difficult to reason about.

[David Kilzer (:ddkilzer)]

Created attachment 397519 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Comment on attachment 397519 [details]
Patch v1

Apparently there is usage of isInvalid() in assert statements!

[David Kilzer (:ddkilzer)]

Created attachment 397529 [details]
Patch v2

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 397533 [details]
Patch for landing

[EWS]

Committed r260704: <https://trac.webkit.org/changeset/260704>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397533 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62371804>