Bug in usb_osx.cpp logic for sending Zero Length Packet [208675141]

Verified

Bug

Status Update

No update yet.

Description

jc...@magicleap.com

created issue #1

Dec 2, 2021 04:09PM

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").

Comments

jc...@magicleap.com <jc...@magicleap.com> #2Dec 2, 2021 04:11PM

I'm pretty far from being a C++ expert, but I think this may be due to holding a reference to the Config beyond it's lifetime?

See StartDiscovery(): https://cs.android.com/android/platform/superproject/main/+/main:packages/modules/adb/client/transport_mdns.cpp;drc=2c73f5abdf31943e12c68e75f8148c10d2abbf6a;l=154?q=transport_mdns.cpp&ss=android

void StartDiscovery() {
    ...

    g_state->task_runner->PostTask([]() {
        //  optional<Config> created here
        auto config = GetConfigForAllInterfaces();
        if (!config) {
            return;
        }

        // long-lived reference of config given to OpenScreen
        g_state->service = discovery::CreateDnsSdService(g_state->task_runner.get(),
                                                         g_state->reporting_client.get(), *config);

        for (int i = 0; i < kNumADBDNSServices; ++i) {
            auto receiver = std::make_unique<ServiceReceiver>(
                    g_state->service.get(), kADBDNSServices[i], OnServiceReceiverResult);
            receiver->StartDiscovery();
            
            ...
        }

       ...

       // config destructed here
    });
}

jc...@magicleap.com <jc...@magicleap.com> #3Dec 2, 2021 05:03PM

A word on impact:

We rely on WiFi debugging because we use the USB port for additional devices.
This has made WiFi based debugging unreliable and painful to use because when ADB crashes the connection is lost and is not automatically re-created. Often it can required manually toggling WiFi debugging on the tablet.

en...@google.com <en...@google.com> #4Dec 2, 2021 06:46PM

Reassigned to sh...@google.com.

I've left Wireshark running over the weekend to detect truncated MDNS queries and can confirm they are killing ADB. The truncated messages seem like valid network traffic, so it shouldn't be killing ADB.

Attached are wireshark records in a screenshot which line up with the ADB logs (timezone is Adelaide +10:30):

daniel@RES-0128:/ressys/maxedge$ cat /tmp/adb.5005.log | grep -ie Bridge -e OSP | tail
01-15 00:02:38.265 37828 37828 F adb     : logging.cpp:39 OSP_CHECK((max_allowed_messages_) > (0)) failed: 0 vs. 0: 
01-15 00:02:41.413 37846 37846 I adb     : main.cpp:63 Android Debug Bridge version 1.0.41
01-15 07:58:02.786 37846 37846 F adb     : logging.cpp:39 OSP_CHECK((max_allowed_messages_) > (0)) failed: 0 vs. 0: 
01-15 07:58:05.928 40026 40026 I adb     : main.cpp:63 Android Debug Bridge version 1.0.41
01-15 08:02:46.408 40026 40026 F adb     : logging.cpp:39 OSP_CHECK((max_allowed_messages_) > (0)) failed: 0 vs. 0: 
01-15 08:02:48.548 40054 40054 I adb     : main.cpp:63 Android Debug Bridge version 1.0.41
01-15 08:11:55.906 40054 40054 F adb     : logging.cpp:39 OSP_CHECK((max_allowed_messages_) > (0)) failed: 0 vs. 0: 
01-15 08:11:59.048 40092 40092 I adb     : main.cpp:63 Android Debug Bridge version 1.0.41
01-15 08:48:03.117 40092 40092 F adb     : logging.cpp:39 OSP_CHECK((max_allowed_messages_) > (0)) failed: 0 vs. 0: 
01-15 08:48:06.280 41928 41928 I adb     : main.cpp:63 Android Debug Bridge version 1.0.41

jc...@magicleap.com <jc...@magicleap.com> #5Dec 2, 2021 07:11PM

Have made a PR: https://android-review.googlesource.com/c/platform/packages/modules/adb/+/2909778

jc...@magicleap.com <jc...@magicleap.com> #6Dec 2, 2021 07:59PM

redacted

jc...@magicleap.com <jc...@magicleap.com> #7Dec 3, 2021 03:59PM

My previous comment giving the steps to replicate the issue appears to have been deleted. I have since forgotten the specifics of the steps, but can share the broad outlines.

Attached is a PCAP containing a truncated message.
Replay this PCAP over UDP (using a tool like https://github.com/ska-sa/udpreplay)

df...@magicleap.com <df...@magicleap.com> #8Dec 3, 2021 05:00PM

I am able to repro. I can reply with tcpreplay.

tcpreplay -i en0 ~/Downloads/truncated\ mDNS\ packets.pcapng

en...@google.com <en...@google.com> #9Dec 3, 2021 05:11PM

It looks like we are not using openscreen properly. Config should live as long as the Service is alive. The docs of MdnsResponder and MdnsServiceImpl are explicit.

MdnsResponder:

 // |record_handler|, |sender|, |receiver|, |task_runner|, |random_delay|, and
  // |config| are expected to persist for the duration of this instance's
  // lifetime.

MdnsServiceImpl

  // |task_runner|, |reporting_client|, and |config| must exist for the duration
  // of this instance's life.

df...@magicleap.com <df...@magicleap.com> #10Dec 3, 2021 05:36PM

Fixed in aosp/2909778

sh...@google.com <sh...@google.com> Dec 16, 2022 10:45PM

Accepted by sh...@google.com.

[Deleted User] <[Deleted User]> #11Dec 28, 2022 11:39PM

Eihafmryelhafmaree@gmail.com

sh...@google.com <sh...@google.com> #12Dec 29, 2022 12:08AM

Eihafmryelhafmaree@gmail.com

sh...@google.com <sh...@google.com> #13Jan 4, 2023 12:18AM

daemon not running; starting now at tcp:5037

sh...@google.com <sh...@google.com> #14Jan 4, 2023 01:17AM

Mine has an error with ui nit just the I, not sure how literal this verbose problem is...please keep us updated.....64 arm correct?

sh...@google.com <sh...@google.com> #15Jan 10, 2023 04:27AM

Hello

en...@google.com <en...@google.com> #16Jan 10, 2023 05:43PM

Приветствую вас

ph...@gmail.com <ph...@gmail.com> #17Feb 12, 2023 07:55AM

هالو

sh...@google.com <sh...@google.com> #18Feb 12, 2023 08:14AM

👍🏻

ju...@gmail.com <ju...@gmail.com> #19Feb 14, 2023 05:50PM

bugreport-X692-GL-QP1A.190711.020-2024-10-18-14-13-07.zip

ju...@gmail.com <ju...@gmail.com> #20Feb 14, 2023 06:08PM

redacted

sh...@gmail.com <sh...@gmail.com> #21Feb 17, 2023 04:25PM

navarretesergio404@gmail.com

ka...@gmail.com <ka...@gmail.com> #22Feb 22, 2023 06:16AM

Reda

ju...@gmail.com <ju...@gmail.com> #23Feb 23, 2023 05:16AM

https://drive.google.com/open?id=1AaFQQv5BQwDTOFOnTjGr1OBpyMbwJ0Mw

ju...@gmail.com <ju...@gmail.com> #24Feb 23, 2023 12:03PM

Setup

ju...@gmail.com <ju...@gmail.com> #25Feb 23, 2023 12:03PM

One ui 7.0

[Deleted User] <[Deleted User]> #26Mar 31, 2023 11:57PM

https://issuetracker.google.com/294120933#attachment46070672

tr...@gmail.com <tr...@gmail.com> #27Apr 5, 2023 07:11PM

Que bueno que ya está bien

xu...@gmail.com <xu...@gmail.com> #28Apr 24, 2023 06:51AM

Bardzo dobre

xu...@gmail.com <xu...@gmail.com> #29Apr 24, 2023 06:52AM

Zadowolony

fa...@hotmail.com <fa...@hotmail.com> #30May 4, 2023 11:00PM

Scan hacking

ju...@gmail.com <ju...@gmail.com> #31May 9, 2023 12:52AM

Please update my best practice

fr...@gmail.com <fr...@gmail.com> #32May 9, 2023 03:42AM

Excelente inicio

ka...@gmail.com <ka...@gmail.com> #33Jun 18, 2023 05:11AM

Please update

fa...@gmail.com <fa...@gmail.com> #34Aug 2, 2023 02:49AM

Done

br...@gmail.com <br...@gmail.com> #35Aug 2, 2023 06:16AM

I have been having a lot of problems lately

Get Outlook for Android<

https://aka.ms/AAb9ysg>
________________________________
From: buganizer-system@google.com <buganizer-system@google.com>
Sent: Tuesday, January 7, 2025 11:29:46 PM
To: b-system+-1739714008@google.com <b-system+-1739714008@google.com>
Cc: kwamanabrown1@gmail.com <kwamanabrown1@gmail.com>
Subject: Re:

Issue 294120933

: ADB crashes with often due to openscreen error

Replying to this email means your email address will be shared with the team that works on this product.

https://issuetracker.google.com/issues/294120933

Changed

am...@gmail.com added

comment #34

https://issuetracker.google.com/issues/294120933#comment34>:

Done

_______________________________

Reference Info: 294120933 ADB crashes with often due to openscreen error
component: Android Public Tracker > App Development > SDK > platform tools > adb<

https://issuetracker.google.com/components/192795>
status: Fixed
reporter: da...@maxmine.com.au
assignee: sa...@google.com
cc: ad...@google.com, sa...@google.com
type: Bug
access level: Default access
priority: P2
severity: S2
retention: Component default

Generated by Google IssueTracker notification system.

You're receiving this email because you are subscribed to updates on Google IssueTracker

issue 294120933

https://issuetracker.google.com/issues/294120933> where you have the roles: subscribed
Unsubscribe from this issue.<

https://issuetracker.google.com/issues/294120933?unsubscribe=true>

sh...@google.com <sh...@google.com> Aug 23, 2023 02:32AM

Marked as fixed.

on...@gmail.com <on...@gmail.com> #36Aug 27, 2023 12:32PM

.

ro...@gmail.com <ro...@gmail.com> #37Oct 19, 2023 02:41AM

tnx

ro...@gmail.com <ro...@gmail.com> #38Oct 19, 2023 02:42AM

Brigadao

pr...@gmail.com <pr...@gmail.com> #39Nov 2, 2023 02:31AM

Add a comment

da...@gmail.com <da...@gmail.com> #40Dec 20, 2023 02:30PM

af...@gmail.com <af...@gmail.com> #41Jun 5, 2024 01:24PM

Comment has been deleted.

Message last modified on Jun 5, 2024 01:24PM

7a...@gmail.com <7a...@gmail.com> #42Jul 19, 2024 07:17PM

ไม่มี

mw...@gmail.com <mw...@gmail.com> #43Oct 20, 2024 08:38PM

https://www.earntp.com/m/h1jze

sa...@google.com <sa...@google.com> Nov 8, 2024 06:15AM

Status: Assigned (reopened)

sa...@google.com <sa...@google.com> #44Dec 16, 2024 06:47PM

Verified by sa...@google.com.

حل المشكلات