IssueTracker

usb_write() attempts to zend a Zero Length Packet (ZLP) when the buffer being sent is an integer multiple of the max packet size. The logic is flawed in that it assumes the max packet size is a power of 2, which we found is not the case in our environment (Big Sur on a MacBook Pro). maxPacketSize is 5120 for us. Adb gets that value by calling GetPipePropertiesV2(), which returns maxPacketSize=5120 and maxBurst=0. It then sets handle->zero_mask to maxPacketSize-1 in order to do a bit comparison in usb_write() to see if the buffer being sent is an integer multiple of the maxPacketSize. But that approach only works if maxPacketSize is a power of 2. I'm guessing the C/C++ modulo operator was avoided in order to avoid a CPU cycle or two, but that ended up being misguided since there is no corresponding check/assertion on maxPacketSize being a power of 2. So, the end result is that usb_write() sometimes sends a ZLP when it doesn't need to. Consider:
maxPacketSize=5120
zero_mask = 5120-1 = 0b1001111111111
usb_write() is called with  buffer of length 1024 0x10000000000
The following expression in usb_write() will not properly determine when to send the ZLP.
    if(!(len & handle->zero_mask)) {

In some cases, a ZLP will be written when it's not needed (the ZLP is only needed when the amount of data being written is an integer multiple of maxPacketSize, as explained here https://stackoverflow.com/questions/41855995/when-should-a-usb-device-send-a-zlp-on-a-bulk-pipe). Imagine, in the above scenario (maxPacketSize=5120), that usb_write() is called with 1024.  The expression !(1024 & 5119) resolves to true, and so a ZLP is sent, but clearly 1024 is not a integer multiple of 5120, and thus a ZLP shouldn't be sent. Does this result in buggy behavior? No. The ZLP is simply extraneous.

In a much more troubling case, a ZLP will NOT be written when it should. Imagine, in the above scenario (maxPacketSize=5120), that usb_write() is called with 5120.  The expression !(5120 & 5119) resolves to false, and so a ZLP is not sent, but clearly 5120 is an integer multiple of 5120, and thus a ZLP should be sent. This can absolutely result in buggy behavior. In fact, with Android 10 devices, it can result in adb aborting. How? Well, because a ZLP hasn't been sent, the buffer may be combined with a subsequent write and adbd might receive a combination of two amessages (the final payload portion of one message, and a header (24 bytes) of the next amessage). We are in fact seeing that happen. And because UsbFfsConnection::ProcessRead() in Android 10 has a CHECK_LE() to ensure the final read of an amessage has exactly the expected remaining bytes of the amesssage and not one byte more, then adbd aborts (effectively "crashes").