adb server: don't close stale fd when TCP transport is closed
I think this fixes a scary bug that could be on all host platforms. When running 'adb unroot' with an emulator, the connection to the emulator is dropped (as expected). I noticed that the adb.log showed: _fh_from_int: 1168: 5280 | _fh_from_int: invalid fd 106 passed to adb_close Background: Every transport has a socketpair (two bidirectional sockets connected to each other to form one 'pipe') that are used as follows: * When adb wants to write to a transport, it writes to t->transport_socket (half of the socketpair). An input thread reads from t->fd (the other half of the socketpair) and writes the data to the underlying transport (TCP, USB). * An output thread reads from the underlying transport (TCP, USB) and writes the data to t->fd. The main thread runs fdevent_loop() which reads from t->transport_socket and processes the packets (that really came from the underlying transport). So t->fd and t->transport_socket are just an intermediate pipe between transport agnostic code in adb and the underlying transport (TCP, USB). Here's what I think is going on: 1. When the TCP transport is closed (such as when running adb unroot), adb server's output thread notices this (adb_read() returns zero), and it writes a special packet to t->fd. 2. The main thread processes the special packet by writing the special packet to the input thread. 3. input_thread() sees the special packet, so it breaks out of a read loop and calls transport_unref() which calls transport_unref_locked(). 4. transport_unref_locked() calls t->close() which is a function pointer that points to transport_local.cpp: remote_close() which calls adb_close(t->fd). <----- ****THIS IS THE BUG**** I think this is a (very old) typo and it should instead be adb_close(t->sfd) (the transport’s actual TCP socket) because it does not make sense for the particular transport mechanism (TCP, USB) to be messing with a socket of the socketpair of the transport agnostic code (t->fd). 5. transport_unref_locked() calls remove_transport() which writes an action to another special socketpair. 6. The action is read and eventually transport_registration_func() is called and it calls adb_close(t->fd). But t->fd was already (erroneously) closed in #4 above!! Anyway, this causes the adb.log output. The fix is to fix the typo changing t->fd to t->sfd and adding some resiliency around whether the socket has already been closed (probably by remote_kick()). I tested this by putting a new adbd on an emulator, a new adb on Linux and Windows and running the adb unroot scenario and checking adb.log. I also ran test_adb.py (which doesn't totally work without problems with an emulator, but I'll leave that to another day.) Change-Id: I188b6c74917a3d721c150fd17ed0f2b63a2178c3 Signed-off-by: Spencer Low <CompareAndSwap@gmail.com>
Loading
Please register or sign in to comment