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>
1 file changed