Bug 214665 - [IPC hardening] WebKit::ArgumentCoder<BlobPart>::decode() and encode() should use enum BlobPart::Type
Summary: [IPC hardening] WebKit::ArgumentCoder<BlobPart>::decode() and encode() should...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-22 16:49 PDT by David Kilzer (:ddkilzer)
Modified: 2020-07-23 11:46 PDT (History)
3 users (show)

See Also:


Attachments
Patch v1 (4.82 KB, patch)
2020-07-22 17:06 PDT, David Kilzer (:ddkilzer)
darin: review+
ddkilzer: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (7.13 KB, patch)
2020-07-23 10:19 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch for landing v2 (7.38 KB, patch)
2020-07-23 10:29 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].