Bug 211000

Summary: IPC::Decoder::isInvalid() should be renamed to isValid()
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebKit2Assignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ggaren, useafterfree, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=210949
https://bugs.webkit.org/show_bug.cgi?id=211006
https://bugs.webkit.org/show_bug.cgi?id=203880
https://bugs.webkit.org/show_bug.cgi?id=211152
Attachments:
Description Flags
Patch v1
ddkilzer: commit-queue-
Patch v2
darin: review+
Patch for landing none

Description David Kilzer (:ddkilzer) 2020-04-24 15:29:34 PDT
IPC::Decoder::isInvalid() should be renamed to isValid().

Negative logic is more difficult to reason about.
Comment 1 David Kilzer (:ddkilzer) 2020-04-24 15:32:59 PDT
Created attachment 397519 [details]
Patch v1
Comment 2 David Kilzer (:ddkilzer) 2020-04-24 15:39:27 PDT
Comment on attachment 397519 [details]
Patch v1

Apparently there is usage of isInvalid() in assert statements!
Comment 3 David Kilzer (:ddkilzer) 2020-04-24 17:23:44 PDT
Created attachment 397529 [details]
Patch v2
Comment 4 Darin Adler 2020-04-24 17:41:40 PDT
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 5 David Kilzer (:ddkilzer) 2020-04-24 18:16:59 PDT
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.
Comment 6 David Kilzer (:ddkilzer) 2020-04-24 18:25:04 PDT
Created attachment 397533 [details]
Patch for landing
Comment 7 EWS 2020-04-25 08:46:52 PDT
Committed r260704: <https://trac.webkit.org/changeset/260704>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397533 [details].
Comment 8 Radar WebKit Bug Importer 2020-04-25 08:47:12 PDT
<rdar://problem/62371804>