Bug 182404

Summary: Fix crashes due to mishandling custom sections.
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, jfbastien, mark.lam, msaboff, natashenka, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing keith_miller: commit-queue+

Description Keith Miller 2018-02-01 13:18:22 PST
Fix crashes due to mishandling custom sections.
Comment 1 Keith Miller 2018-02-01 13:43:30 PST
Created attachment 332906 [details]
Patch
Comment 2 Keith Miller 2018-02-01 13:44:18 PST
rdar://problem/36935863
Comment 3 Saam Barati 2018-02-01 14:12:29 PST
Comment on attachment 332906 [details]
Patch

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

r=me

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:58
> +    // This is  not really a known section

extra space

> Source/JavaScriptCore/wasm/WasmSections.h:55
> +    // It's important that Begin is less than every other section number and that Custom is greater.
> +    // This only works because section numbers are currently monotonically increasing and we don't support
> +    // any custom sections. Also, Begin is not a real section but is used as a marker for validating the ordering
> +    // of sections.
> +    Begin = 0,

static_assert please
Comment 4 JF Bastien 2018-02-01 14:14:00 PST
Comment on attachment 332906 [details]
Patch

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

r=me

> Source/JavaScriptCore/wasm/WasmSections.h:53
> +    // any custom sections. Also, Begin is not a real section but is used as a marker for validating the ordering

We support the "name" custom section.

> Source/JavaScriptCore/wasm/WasmSections.h:77
> +    if (!sectionByte)

Compare to static_cast<uint8_t>(Section::Custom) instead?
Comment 5 Keith Miller 2018-02-01 14:18:42 PST
Comment on attachment 332906 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmSections.h:53
>> +    // any custom sections. Also, Begin is not a real section but is used as a marker for validating the ordering
> 
> We support the "name" custom section.

Oh right, I'll change the comment.

>> Source/JavaScriptCore/wasm/WasmSections.h:55
>> +    Begin = 0,
> 
> static_assert please

What static assert are you looking for? Begin < Type?

>> Source/JavaScriptCore/wasm/WasmSections.h:77
>> +    if (!sectionByte)
> 
> Compare to static_cast<uint8_t>(Section::Custom) instead?

That would be incorrect...
Comment 6 Keith Miller 2018-02-01 15:22:45 PST
Created attachment 332914 [details]
Patch for landing
Comment 7 Keith Miller 2018-02-01 20:30:42 PST
Committed r227994: <https://trac.webkit.org/changeset/227994>
Comment 8 Brent Fulgham 2018-05-01 08:22:59 PDT
*** Bug 182208 has been marked as a duplicate of this bug. ***