Crash in usbmux_packet_alloc assert

[ramonpoca]

We're getting crash dumps from our OS X app pointing to an assertion being triggered in PTUSBHub.m line 66:

#0. Crashed: com.apple.libdispatch-io.opq
0  libsystem_kernel.dylib         0x7fff8d4b5286 __pthread_kill + 10
1  libsystem_pthread.dylib        0x7fff8c6379f9 pthread_kill + 90
2  libsystem_c.dylib              0x7fff93a8f9ab abort + 129
3  libsystem_c.dylib              0x7fff93a57a91 __assert_rtn + 319
4  Application                    0x109abf3c8 usbmux_packet_alloc (PTUSBHub.m:66)
5  libdispatch.dylib              0x7fff947f8e2b ___dispatch_operation_deliver_data_block_invoke + 118
6  libdispatch.dylib              0x7fff947e0700 _dispatch_call_block_and_release + 12
7  libdispatch.dylib              0x7fff947dce73 _dispatch_client_callout + 8
8  libdispatch.dylib              0x7fff947e05cd _dispatch_queue_drain + 1100
9  libdispatch.dylib              0x7fff947e0030 _dispatch_queue_invoke + 202
10 libdispatch.dylib              0x7fff947ed5aa _dispatch_main_queue_callback_4CF + 416
11 CoreFoundation                 0x7fff8a5fd3f9 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9
12 CoreFoundation                 0x7fff8a5b868f __CFRunLoopRun + 2159
13 CoreFoundation                 0x7fff8a5b7bd8 CFRunLoopRunSpecific + 296
14 HIToolbox                      0x7fff8968356f RunCurrentEventLoopInMode + 235
15 HIToolbox                      0x7fff896832ea ReceiveNextEventCommon + 431
16 HIToolbox                      0x7fff8968312b _BlockUntilNextEventMatchingListInModeWithFilter + 71
17 AppKit                         0x7fff872698ab _DPSNextEvent + 978
18 AppKit                         0x7fff87268e58 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 346
19 AppKit                         0x7fff8725eaf3 -[NSApplication run] + 594
20 AppKit                         0x7fff871db244 NSApplicationMain + 1832
21 Application                    0x109a935d4 start + 52

We poll the USB port repeatedly waiting for a device with our client app to connect. The crazy thing about that is the assert being triggered should be:

static usbmux_packet_t *usbmux_packet_alloc(CFIndex payloadSize) {
  assert(payloadSize <= UINT32_MAX); // protocol limitation
  uint32_t payloadLength = (uint32_t)payloadSize;

but the stack shows this coming from the dispatch_io_read in scheduleReadPacketWithCallback (PTUSBHub.m:494), which uses a uint32_t parameter, thus making it (theoretically) impossible to overflow UINT32_MAX:

    // Allocate a new usbmux_packet_t for the expected size
    uint32_t payloadLength = upacket_len - sizeof(usbmux_packet_t);
    usbmux_packet_t *upacket = usbmux_packet_alloc(payloadLength);

Any ideas? Maybe stack corruption in the previous memcpy (should be protected by the assertion, so...)?

[rsms]

Are you sure it's not the second assertion? I.e. assert(sizeof(usbmux_packet_t) == 16);

I'm working on a new version of peertalk where I fixed an issue pertaining to this, namely that depending on compiler version and settings, the actual size of a struct might be larger than what's needed (usually an optimization to align it to words.) This might cause sizeof to report not the logical size, but the effective size of the struct.

Can you repro the crash? If so, run the app through lldb (e.g. lldb /path/to/your.app/Contents/MacOS/your) and see where it actually aborts from an assertion. It's also possible you've got a stack overflow issue in which case the stack trace might be incorrect.

BTW, the fix to the struct size is this:

typedef struct usbmux_packet {
  ...
} __attribute__((__packed__)) usbmux_packet_t;

[ramonpoca]

The second assert should crash on the first call, as sizeof(usbmux_packet_t) is known at compile time (and might be even optimized out). We are getting the traces through Crashlytics, and the issue repeats quite frequently.

I tried to replicate the call sequence to check for any unexpected value of payloadLength causing the crash, but couldn't. But note that nowhere is checked that the received upacket_len is bigger than sizeof(usbmux_packet_t), so a corrupt/unexpected packet of zero or less than 16 size would cause payloadLength to have a value near to UINT32_MAX (adding back 16 in usbmux_packet_alloc will cause this to allocate undersized packets).

[rsms]

b3090d72e56b8714e6ce7d7658038e3a77c4b38f makes some changes that should help in preventing getting into these situations. usbmux_packets are only used during negotiation with the USB mux hub, but isn't used for user communication (i.e. PTChannel doesn't use these packets and doesn't have these limitations.)