Bug 232567

Summary: Crash under HIDDevice::HIDDevice()
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, darin, ggaren, sam, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Chris Dumez 2021-11-01 09:20:40 PDT
Crash under HIDDevice::HIDDevice():

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000000
Exception Codes:       0x0000000000000001, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Termination Reason:    Namespace SIGNAL, Code 11 Segmentation fault: 11
Terminating Process:   exc handler [15076]

Thread 0 Crashed ↩::   Dispatch queue: com.apple.main-thread
0   com.apple.CoreFoundation      	       0x7ff81d68e8b4 CFNumberGetValue + 32
1   com.apple.WebCore             	       0x7ff92047a292 WebCore::HIDDevice::HIDDevice(__IOHIDDevice*) + 178
2   com.apple.WebCore             	       0x7ff920335cac WebCore::HIDGamepad::create(__IOHIDDevice*, unsigned int) + 28
3   com.apple.WebCore             	       0x7ff91f4ec964 WebCore::deviceAddedCallback(void*, int, void*, __IOHIDDevice*) + 692
4   com.apple.framework.IOKit     	       0x7ff820009252 __IOHIDManagerDeviceApplier + 762
5   com.apple.CoreFoundation      	       0x7ff81d6bbc1e __CFSetApplyFunction_block_invoke + 18
6   com.apple.CoreFoundation      	       0x7ff81d6bba8e CFBasicHashApply + 110
7   com.apple.CoreFoundation      	       0x7ff81d6bb9e0 CFSetApplyFunction + 130
8   com.apple.framework.IOKit     	       0x7ff820007c2a __ApplyToDevices + 104
9   com.apple.framework.IOKit     	       0x7ff8200094a2 __IOHIDManagerInitialEnumCallback + 38
10  com.apple.CoreFoundation      	       0x7ff81d6f194c __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 16
11  com.apple.CoreFoundation      	       0x7ff81d6f18b4 __CFRunLoopDoSource0 + 178
12  com.apple.CoreFoundation      	       0x7ff81d6f1634 __CFRunLoopDoSources0 + 242
13  com.apple.CoreFoundation      	       0x7ff81d6f006a __CFRunLoopRun + 892
14  com.apple.CoreFoundation      	       0x7ff81d6ef62c CFRunLoopRunSpecific + 562
15  com.apple.HIToolbox           	       0x7ff8262fc850 RunCurrentEventLoopInMode + 290
16  com.apple.HIToolbox           	       0x7ff8262fc478 ReceiveNextEventCommon + 284
17  com.apple.HIToolbox           	       0x7ff8262fc344 _BlockUntilNextEventMatchingListInModeWithFilter + 68
18  com.apple.AppKit              	       0x7ff820118cb4 _DPSNextEvent + 886
19  com.apple.AppKit              	       0x7ff820117320 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1410
20  com.apple.Safari.framework    	       0x7ff929d3db62 -[BrowserApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 230
Comment 1 Chris Dumez 2021-11-01 09:21:20 PDT
<rdar://79414185>
Comment 2 Chris Dumez 2021-11-01 09:23:06 PDT
Created attachment 442983 [details]
Patch
Comment 3 Simon Fraser (smfr) 2021-11-01 09:28:09 PDT
Comment on attachment 442983 [details]
Patch

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

> Source/WebCore/platform/mac/HIDDevice.cpp:45
> +    int propertyValue = -1;

Is the caller prepared to handle -1? Maybe return std::optional<int> and do something in the caller?
Comment 4 Chris Dumez 2021-11-01 09:28:50 PDT
Comment on attachment 442983 [details]
Patch

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

>> Source/WebCore/platform/mac/HIDDevice.cpp:45
>> +    int propertyValue = -1;
> 
> Is the caller prepared to handle -1? Maybe return std::optional<int> and do something in the caller?

This is intentional

> Source/WebCore/platform/mac/HIDDevice.cpp:57
>      if (vendorID < 0 || vendorID > std::numeric_limits<uint16_t>::max()) {

Look here
Comment 5 Chris Dumez 2021-11-01 09:48:06 PDT
Comment on attachment 442983 [details]
Patch

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

>> Source/WebCore/platform/mac/HIDDevice.cpp:57
>>      if (vendorID < 0 || vendorID > std::numeric_limits<uint16_t>::max()) {
> 
> Look here

Same thing is done for productID a few lines below.

I could have returned 0 but returning -1 makes it though that we log about malformed values.
Comment 6 Chris Dumez 2021-11-01 12:03:03 PDT
Comment on attachment 442983 [details]
Patch

Thanks for reviewing
Comment 7 EWS 2021-11-01 12:21:18 PDT
Committed r285118 (243759@main): <https://commits.webkit.org/243759@main>

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