From 47fae70ec0e85adf9753ee617a261a026c272aa3 Mon Sep 17 00:00:00 2001 From: Romain Vimont Date: Mon, 24 Apr 2017 09:37:04 +0200 Subject: [PATCH] Transfer client id on connection open Whenever a port redirection is enabled (typically through "adb reverse"), connect() will succeed even if the relay server is not listening on the other side of the tunnel. Commit 301867efe7caee8a91b73a014979eed757d85010 provided a workaround, assuming that the port forwarding was local: the client attempted to read from the socket with the smallest possible timeout. Since there was nothing to read, a read error indicated that no server was listening on the other side of the tunnel, while the connection was considered ok on timeout. But this does not generally fix the problem. For example, if the relay server is executed remotely and exposed through a SSH local port forwarding, then connect() would still succeed (a server was listening on the other side of the "adb reverse" tunnel) even if the relay server was not listening behind the SSH tunnel. Therefore, modify the protocol between the client and the relay server: when a client connects, the relay server immediately writes its id (an integer) to the TCP socket. The client considers itself connected only once it has received this number. Thus, it will be able to detect connection failure before switching its state to CONNECTED. --- DEVELOP.md | 8 ++++ .../com/genymobile/gnirehtet/RelayTunnel.java | 43 +++++++++-------- .../java/com/genymobile/relay/Client.java | 46 ++++++++++++++++++- 3 files changed, 73 insertions(+), 24 deletions(-) diff --git a/DEVELOP.md b/DEVELOP.md index 22e390d2..28fed6ef 100644 --- a/DEVELOP.md +++ b/DEVELOP.md @@ -137,6 +137,14 @@ Each [`Client`] manages a TCP socket, used to transmit raw IP packets from and to the _Gnirehtet_ Android client. Thus, these IP packets are encapsulated into TCP (they are transmitted as the TCP payload). +When a client connects, the relay server assigns an integer id to it, which it +writes to the TCP socket. The client considers itself connected to the relay +server only once it has received this number. This allows to detect any +end-to-end connection issue immediately. For instance, a TCP _connect_ initiated +by a client succeeds whenever a port redirection is enabled (typically through +`adb reverse`), even if the relay server is not listening. In that case, the +first _read_ will fail. + The [`IPv4Packet`] class provides a structured view to read and write packet data, which is physically stored in the buffers (the little squares on the schema). Since we handle one packet at a time with asynchronous I/O, there is no diff --git a/app/src/main/java/com/genymobile/gnirehtet/RelayTunnel.java b/app/src/main/java/com/genymobile/gnirehtet/RelayTunnel.java index 0ec51dbe..10f8e518 100644 --- a/app/src/main/java/com/genymobile/gnirehtet/RelayTunnel.java +++ b/app/src/main/java/com/genymobile/gnirehtet/RelayTunnel.java @@ -22,9 +22,8 @@ import java.io.IOException; import java.net.Inet4Address; import java.net.InetSocketAddress; -import java.net.Socket; -import java.net.SocketTimeoutException; import java.nio.ByteBuffer; +import java.nio.channels.ReadableByteChannel; import java.nio.channels.SocketChannel; public final class RelayTunnel implements Tunnel { @@ -48,7 +47,7 @@ public static RelayTunnel open(VpnService vpnService) throws IOException { public void connect() throws IOException { channel.connect(new InetSocketAddress(Inet4Address.getLocalHost(), DEFAULT_PORT)); - fakeRead(channel.socket()); + readClientId(channel); } /** @@ -60,30 +59,30 @@ public void connect() throws IOException { * As a consequence, the connection state of the relay server would be invalid temporarily (we * would switch to CONNECTED state then switch back to DISCONNECTED). *

- * To avoid this problem, we must actually try to read from the server, so that an error occurs + * To avoid this problem, we must actually read from the server, so that an error occurs * immediately if the relay server is not accessible. *

- * To do so, we set the lowest timeout possible (1) that is not "infinity" (0), try to read, - * then reset the timeout option. Since the client is always the first peer to send data, there - * is never anything to read. + * Therefore, the relay server immediately sends the client id: consume it and log it. * - * @param socket the socket to read + * @param channel the channel to communicate with the relay server * @throws IOException if an I/O error occurs */ - private static void fakeRead(Socket socket) throws IOException { - int timeout = socket.getSoTimeout(); - socket.setSoTimeout(1); - boolean eof = false; - try { - eof = socket.getInputStream().read() == -1; - } catch (SocketTimeoutException e) { - // ignore, expected timeout - } finally { - socket.setSoTimeout(timeout); - } - if (eof) { - throw new IOException("Cannot connect to the relay server"); - } + private static void readClientId(ReadableByteChannel channel) throws IOException { + Log.d(TAG, "Requesting client id"); + int clientId = readInt(channel); + Log.d(TAG, "Connected to the relay server as #" + Binary.unsigned(clientId)); + } + + private static int readInt(ReadableByteChannel channel) throws IOException { + final int intSize = 4; + ByteBuffer buffer = ByteBuffer.allocate(intSize); + do { + if (channel.read(buffer) == -1) { + throw new IOException("Cannot read from channel"); + } + } while (buffer.hasRemaining()); + buffer.flip(); + return buffer.getInt(); } @Override diff --git a/relay/src/main/java/com/genymobile/relay/Client.java b/relay/src/main/java/com/genymobile/relay/Client.java index 4a5b18f6..a07263c5 100644 --- a/relay/src/main/java/com/genymobile/relay/Client.java +++ b/relay/src/main/java/com/genymobile/relay/Client.java @@ -17,6 +17,7 @@ package com.genymobile.relay; import java.io.IOException; +import java.nio.ByteBuffer; import java.nio.channels.ClosedChannelException; import java.nio.channels.SelectionKey; import java.nio.channels.Selector; @@ -42,10 +43,14 @@ public class Client { private final List pendingPacketSources = new ArrayList<>(); + // store the remaining bytes of "id" to send to the client before relaying any data + private ByteBuffer pendingIdBuffer; + public Client(Selector selector, SocketChannel clientChannel, RemoveHandler removeHandler) throws ClosedChannelException { id = nextId++; this.clientChannel = clientChannel; router = new Router(this, selector); + pendingIdBuffer = createIntBuffer(id); SelectionHandler selectionHandler = (selectionKey) -> { if (selectionKey.isValid() && selectionKey.isWritable()) { @@ -58,12 +63,20 @@ public Client(Selector selector, SocketChannel clientChannel, RemoveHandler