IssueTracker

Correction:
   So, the end result is that usb_write() sometimes sends a ZLP when it doesn't need to
should be
   So, the end result is that usb_write() sometimes sends a ZLP when it doesn't need to, and sometimes neglects to send a ZLP when it should

Note that the CHECK_LE has since been replaced with HandleError(), which causes adbd to not abort...but that stowaway amessage header is being dropped all the same...and so the behavior is pretty much undefined, I imagine. I'm not sure the switch to HandleError was a good idea, since it more or less swept the real problem under the rug.

Thanks for the quick response.

Indeed, I see similar logic in usb_write() in usb_linux. maxPacketSize is 512 (a power of 2) on my Ubuntu environment, so all good there as far as runtime behavior. It sure is odd that on macOS it's 5120, which happens to be exactly 10x the linux value. Seem suspicious, doesn't it?

Yeah, on the CHECK thing... I guess I see the client and server as a closed system in that the code is developed and distributed together. That said, obviously anyone can write their own adb client/server, so in that respect, I agree with the disagreement :-)

We've been discussing it here some more and we have a theory:

5120 is likely not really the maxPacketSize on our Macs. It's probably 512. And a burst is probably 10 packets.

Note that usb_osx.cpp calculates the maxPacketSize as follows

        if (maxBurst != 0)
            // bMaxBurst is the number of additional packets in the burst.
            maxPacketSize /= (maxBurst + 1);

If the theory about packet size truly being 512 is correct, then it means the real bug is in GetPipePropertiesV2() returning zero for maxBurst instead of 9. If it returned 9, then maxPacketSize would be 512 (again, a power of 2), and the zero_mask expression would work out just fine. All situations where we've had our adbd abort because of the unexpected stowaway amessage header in a packet are ones where the sent buffer are an integer multiple of 512. None are an integer multiple of 5120.

Still there's no denying that the unchecked assumption of maxPacketSize being a power of 2 is a bug. Changing the code to use modulus operation instead of a bit-mask check would be technically more correct, but wouldn't fix the broken behavior in our case, since ultimately, it appears the maxPacketSize adb is getting is wrong. But if you had an assert in the client/server on macPacketSize being a power of 2, then that would at least have brought the incorrect maxPacketSize issue front and center.

Finally, I forced my macOS server to use 512 for maxPacketSize instead of the suspicious (and almost certainly wrong) 5120...and the daemon no longer aborts because it eventually receives a stowaway amessage header in UsbFfsConnection::ProcessRead(). This gives further credence to our theory that the root cause of our problem is that the macOS adb server isn't querying/calculating the max packet size correctly, at least on our MacBook Pros with Big Sur on them.