Bug 214665

Summary: [IPC hardening] WebKit::ArgumentCoder<BlobPart>::decode() and encode() should use enum BlobPart::Type
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebKit2Assignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, useafterfree, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch v1
darin: review+, ddkilzer: commit-queue-
Patch for landing
none
Patch for landing v2 none

Description David Kilzer (:ddkilzer) 2020-07-22 16:49:24 PDT
WebCore::ArgumentCoder<BlobPart>::decode() and encode() should use enum BlobPart::Type.

We can also get rid of a default: case label in decode() while we're here to make -Wimplicit-fallthrough emit a warning if a new enum is added.

<rdar://problem/65777948>
Comment 1 David Kilzer (:ddkilzer) 2020-07-22 17:06:10 PDT
Created attachment 404997 [details]
Patch v1
Comment 2 Darin Adler 2020-07-22 18:25:10 PDT
Comment on attachment 404997 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=404997&action=review

> Source/WebCore/platform/network/BlobPart.h:100
> +namespace WTF {
> +
> +template<> struct EnumTraits<WebCore::BlobPart::Type> {
> +    using values = EnumValues<
> +        WebCore::BlobPart::Type,
> +        WebCore::BlobPart::Type::Data,
> +        WebCore::BlobPart::Type::Blob
> +    >;
> +};
> +
> +} // namespace WTF

Instead of this, I suggest:

    enum class Type : bool { Data, Blob };
Comment 3 David Kilzer (:ddkilzer) 2020-07-23 09:49:26 PDT
Comment on attachment 404997 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=404997&action=review

>> Source/WebCore/platform/network/BlobPart.h:100
>> +} // namespace WTF
> 
> Instead of this, I suggest:
> 
>     enum class Type : bool { Data, Blob };

Will fix before landing.  Thanks!
Comment 4 David Kilzer (:ddkilzer) 2020-07-23 10:19:40 PDT
Created attachment 405051 [details]
Patch for landing
Comment 5 Darin Adler 2020-07-23 10:22:43 PDT
Comment on attachment 405051 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=405051&action=review

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2481
>      case BlobPart::Data:

Seems like this won’t compile: Needs to be BlobPart::Type::Data.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2484
>      case BlobPart::Blob:

Ditto.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2506
>      case BlobPart::Blob: {

Here too, and above.
Comment 6 David Kilzer (:ddkilzer) 2020-07-23 10:29:08 PDT
Created attachment 405053 [details]
Patch for landing v2
Comment 7 David Kilzer (:ddkilzer) 2020-07-23 11:42:42 PDT
Comment on attachment 405053 [details]
Patch for landing v2

Adding cq+ since there are no logic changes from "Patch v1", and the patch built on all platforms.
Comment 8 EWS 2020-07-23 11:46:05 PDT
Committed r264780: <https://trac.webkit.org/changeset/264780>

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