When the target of your bindService() call is hosted in a different process, Android supplies + * you with an {@link IBinder} that proxies your transactions to the remote {@link + * android.os.Binder} instance. But when the target Service is hosted in the same process, Android + * supplies you with that local instance of {@link android.os.Binder} directly. This in-process + * implementation of {@link IBinder} is problematic for clients that want "oneway" transaction + * semantics because its transact() method simply invokes onTransact() on the caller's thread, even + * when the {@link IBinder#FLAG_ONEWAY} flag is set. Even though this behavior is documented, its + * consequences with respect to reentrancy, locking, and transaction dispatch order can be + * surprising and dangerous. + * + *
Wrap your {@link IBinder}s with an instance of this class to ensure the following + * out-of-process "oneway" semantics are always in effect: + * + *
NB: One difference that this class can't conceal is that calls to onTransact() are serialized + * per {@link OneWayBinderProxy} instance, not per instance of the wrapped {@link IBinder}. An + * android.os.Binder with in-process callers could still receive concurrent calls to onTransact() on + * different threads if callers used different {@link OneWayBinderProxy} instances or if that Binder + * also had out-of-process callers. + */ +public abstract class OneWayBinderProxy { + private static final Logger logger = Logger.getLogger(OneWayBinderProxy.class.getName()); + protected final IBinder delegate; + + private OneWayBinderProxy(IBinder iBinder) { + this.delegate = iBinder; + } + + /** + * Returns a new instance of {@link OneWayBinderProxy} that wraps {@code iBinder}. + * + * @param iBinder the binder to wrap + * @param inProcessThreadHopExecutor a non-direct Executor used to dispatch calls to onTransact(), + * if necessary + * @return a new instance of {@link OneWayBinderProxy} + */ + public static OneWayBinderProxy wrap(IBinder iBinder, Executor inProcessThreadHopExecutor) { + return (iBinder instanceof Binder) + ? new InProcessImpl(iBinder, inProcessThreadHopExecutor) + : new OutOfProcessImpl(iBinder); + } + + /** + * Enqueues a transaction for the wrapped {@link IBinder} with guaranteed "oneway" semantics. + * + *
NB: Unlike {@link IBinder#transact}, implementations of this method take ownership of the + * {@code data} Parcel. When this method returns, {@code data} will normally be empty, but callers + * should still unconditionally {@link ParcelHolder#close()} it to avoid a leak in case they or + * the implementation throws before ownership is transferred. + * + * @param code identifies the type of this transaction + * @param data a non-empty container of the Parcel to be sent + * @throws RemoteException if the transaction could not even be queued for dispatch on the server. + * Failures from {@link Binder#onTransact} are *never* reported this way. + */ + public abstract void transact(int code, ParcelHolder data) throws RemoteException; + + /** + * Returns the wrapped {@link IBinder} for the purpose of calling methods other than {@link + * IBinder#transact(int, Parcel, Parcel, int)}. + */ + public IBinder getDelegate() { + return delegate; + } + + static class OutOfProcessImpl extends OneWayBinderProxy { + OutOfProcessImpl(IBinder iBinder) { + super(iBinder); + } + + @Override + public void transact(int code, ParcelHolder data) throws RemoteException { + if (!transactAndRecycleParcel(code, data.release())) { + // This cannot happen (see g/android-binder/c/jM4NvS234Rw) but, just in case, let the caller + // handle it along with all the other possible transport-layer errors. + throw new RemoteException("BinderProxy#transact(" + code + ", FLAG_ONEWAY) returned false"); + } + } + } + + protected boolean transactAndRecycleParcel(int code, Parcel data) throws RemoteException { + try { + return delegate.transact(code, data, null, IBinder.FLAG_ONEWAY); + } finally { + data.recycle(); + } + } + + static class InProcessImpl extends OneWayBinderProxy { + private final SerializingExecutor executor; + + InProcessImpl(IBinder binder, Executor executor) { + super(binder); + this.executor = new SerializingExecutor(executor); + } + + @Override + public void transact(int code, ParcelHolder wrappedParcel) { + // Transfer ownership, taking care to handle any RuntimeException from execute(). + Parcel parcel = wrappedParcel.get(); + executor.execute( + () -> { + try { + if (!transactAndRecycleParcel(code, parcel)) { + // onTransact() in our same process returned this. Ignore it, just like Android + // would have if the android.os.Binder was in another process. + logger.log(Level.FINEST, "A oneway transaction was not understood - ignoring"); + } + } catch (Exception e) { + // onTransact() in our same process threw this. Ignore it, just like Android would + // have if the android.os.Binder was in another process. + logger.log(Level.FINEST, "A oneway transaction threw - ignoring", e); + } + }); + wrappedParcel.release(); + } + } +} diff --git a/binder/src/main/java/io/grpc/binder/internal/Outbound.java b/binder/src/main/java/io/grpc/binder/internal/Outbound.java index 2a4e968b1e8..e2896be02a1 100644 --- a/binder/src/main/java/io/grpc/binder/internal/Outbound.java +++ b/binder/src/main/java/io/grpc/binder/internal/Outbound.java @@ -221,15 +221,14 @@ final void send() throws StatusException { @GuardedBy("this") @SuppressWarnings("fallthrough") protected final void sendInternal() throws StatusException { - Parcel parcel = Parcel.obtain(); - int flags = 0; - parcel.writeInt(0); // Placeholder for flags. Will be filled in below. - parcel.writeInt(transactionIndex++); - try { + try (ParcelHolder parcel = ParcelHolder.obtain()) { + int flags = 0; + parcel.get().writeInt(0); // Placeholder for flags. Will be filled in below. + parcel.get().writeInt(transactionIndex++); switch (outboundState) { case INITIAL: flags |= TransactionUtils.FLAG_PREFIX; - flags |= writePrefix(parcel); + flags |= writePrefix(parcel.get()); onOutboundState(State.PREFIX_SENT); if (!messageAvailable() && !suffixReady) { break; @@ -239,7 +238,7 @@ protected final void sendInternal() throws StatusException { InputStream messageStream = peekNextMessage(); if (messageStream != null) { flags |= TransactionUtils.FLAG_MESSAGE_DATA; - flags |= writeMessageData(parcel, messageStream); + flags |= writeMessageData(parcel.get(), messageStream); } else { checkState(suffixReady); } @@ -252,20 +251,19 @@ protected final void sendInternal() throws StatusException { // Fall-through. case ALL_MESSAGES_SENT: flags |= TransactionUtils.FLAG_SUFFIX; - flags |= writeSuffix(parcel); + flags |= writeSuffix(parcel.get()); onOutboundState(State.SUFFIX_SENT); break; default: throw new AssertionError(); } - TransactionUtils.fillInFlags(parcel, flags); + TransactionUtils.fillInFlags(parcel.get(), flags); + int dataSize = parcel.get().dataSize(); transport.sendTransaction(callId, parcel); - statsTraceContext.outboundWireSize(parcel.dataSize()); - statsTraceContext.outboundUncompressedSize(parcel.dataSize()); + statsTraceContext.outboundWireSize(dataSize); + statsTraceContext.outboundUncompressedSize(dataSize); } catch (IOException e) { throw Status.INTERNAL.withCause(e).asException(); - } finally { - parcel.recycle(); } } diff --git a/binder/src/main/java/io/grpc/binder/internal/ParcelHolder.java b/binder/src/main/java/io/grpc/binder/internal/ParcelHolder.java new file mode 100644 index 00000000000..cad8da9712f --- /dev/null +++ b/binder/src/main/java/io/grpc/binder/internal/ParcelHolder.java @@ -0,0 +1,76 @@ +package io.grpc.binder.internal; + +import static com.google.common.base.Preconditions.checkState; + +import android.os.Parcel; +import com.google.common.annotations.VisibleForTesting; +import java.io.Closeable; +import javax.annotation.Nullable; + +/** + * Wraps a {@link Parcel} from the static {@link Parcel#obtain()} pool with methods that make it + * easy to eventually {@link Parcel#recycle()} it. + */ +class ParcelHolder implements Closeable { + + @Nullable private Parcel parcel; + + /** + * Creates a new instance that owns a {@link Parcel} newly obtained from Android's object pool. + */ + public static ParcelHolder obtain() { + return new ParcelHolder(Parcel.obtain()); + } + + /** Creates a new instance taking ownership of the specified {@code parcel}. */ + public ParcelHolder(Parcel parcel) { + this.parcel = parcel; + } + + /** + * Returns the wrapped {@link Parcel} if we still own it. + * + * @throws IllegalStateException if ownership has already been given up by {@link #release()} + */ + public Parcel get() { + checkState(parcel != null, "get() after close()/release()"); + return parcel; + } + + /** + * Returns the wrapped {@link Parcel} and releases ownership of it. + * + * @throws IllegalStateException if ownership has already been given up by {@link #release()} + */ + public Parcel release() { + Parcel result = get(); + this.parcel = null; + return result; + } + + /** + * Recycles the wrapped {@link Parcel} to Android's object pool, if we still own it. + * + *
Otherwise, this method has no effect. + */ + @Override + public void close() { + if (parcel != null) { + parcel.recycle(); + parcel = null; + } + } + + /** + * Returns true iff this container no longer owns a {@link Parcel}. + * + *
{@link #isEmpty()} is true after all call to {@link #close()} or {@link #release()}. + * + *
Typically only used for debugging or testing since Parcel-owning code should be calling
+ * {@link #close()} unconditionally.
+ */
+ @VisibleForTesting
+ public boolean isEmpty() {
+ return parcel == null;
+ }
+}
diff --git a/binder/src/test/java/io/grpc/binder/internal/OneWayBinderProxyTest.java b/binder/src/test/java/io/grpc/binder/internal/OneWayBinderProxyTest.java
new file mode 100644
index 00000000000..167e33e29a5
--- /dev/null
+++ b/binder/src/test/java/io/grpc/binder/internal/OneWayBinderProxyTest.java
@@ -0,0 +1,197 @@
+package io.grpc.binder.internal;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import android.os.Binder;
+import android.os.IBinder;
+import android.os.Parcel;
+import android.os.RemoteException;
+import io.grpc.binder.internal.OneWayBinderProxy.InProcessImpl;
+import io.grpc.binder.internal.OneWayBinderProxy.OutOfProcessImpl;
+import java.util.ArrayDeque;
+import java.util.ArrayList;
+import java.util.Queue;
+import java.util.concurrent.Executor;
+import java.util.concurrent.RejectedExecutionException;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+import org.robolectric.RobolectricTestRunner;
+
+/** Unit tests for the {@link OneWayBinderProxy} implementations. */
+@RunWith(RobolectricTestRunner.class)
+public class OneWayBinderProxyTest {
+ @Rule public final MockitoRule mockito = MockitoJUnit.rule();
+
+ QueuingExecutor queuingExecutor = new QueuingExecutor();
+
+ @Mock IBinder mockBinder;
+
+ RecordingBinder recordingBinder = new RecordingBinder();
+
+ @Test
+ public void shouldProxyInProcessTransactionsOnExecutor() throws RemoteException {
+ InProcessImpl proxy = new InProcessImpl(recordingBinder, queuingExecutor);
+ try (ParcelHolder parcel = ParcelHolder.obtain()) {
+ parcel.get().writeInt(123);
+ proxy.transact(456, parcel);
+ assertThat(parcel.isEmpty()).isTrue();
+ assertThat(recordingBinder.txnLog).isEmpty();
+ queuingExecutor.runAllQueued();
+ assertThat(recordingBinder.txnLog).hasSize(1);
+ assertThat(recordingBinder.txnLog.get(0).argument).isEqualTo(123);
+ assertThat(recordingBinder.txnLog.get(0).code).isEqualTo(456);
+ assertThat(recordingBinder.txnLog.get(0).flags).isEqualTo(IBinder.FLAG_ONEWAY);
+ }
+ }
+
+ @Test
+ public void shouldNotLeakParcelsInCaseOfRejectedExecution() throws RemoteException {
+ InProcessImpl proxy = new InProcessImpl(recordingBinder, queuingExecutor);
+ queuingExecutor.shutdown();
+ try (ParcelHolder parcel = ParcelHolder.obtain()) {
+ parcel.get().writeInt(123);
+ assertThrows(RejectedExecutionException.class, () -> proxy.transact(123, parcel));
+ assertThat(parcel.isEmpty()).isFalse(); // Parcel didn't leak because we still own it.
+ }
+ }
+
+ @Test
+ public void shouldProxyOutOfProcessTransactionsSynchronously() throws RemoteException {
+ OutOfProcessImpl proxy = new OutOfProcessImpl(recordingBinder);
+ try (ParcelHolder parcel = ParcelHolder.obtain()) {
+ parcel.get().writeInt(123);
+ proxy.transact(456, parcel);
+ assertThat(parcel.isEmpty()).isTrue();
+ assertThat(recordingBinder.txnLog).hasSize(1);
+ assertThat(recordingBinder.txnLog.get(0).argument).isEqualTo(123);
+ assertThat(recordingBinder.txnLog.get(0).code).isEqualTo(456);
+ assertThat(recordingBinder.txnLog.get(0).flags).isEqualTo(IBinder.FLAG_ONEWAY);
+ }
+ }
+
+ @Test
+ public void shouldIgnoreInProcessRemoteExceptions() throws RemoteException {
+ when(mockBinder.transact(anyInt(), any(), any(), anyInt())).thenThrow(RemoteException.class);
+ InProcessImpl proxy = new InProcessImpl(mockBinder, queuingExecutor);
+ try (ParcelHolder parcel = ParcelHolder.obtain()) {
+ proxy.transact(123, parcel); // Doesn't throw.
+ verify(mockBinder, never()).transact(anyInt(), any(), any(), anyInt());
+ queuingExecutor.runAllQueued();
+ }
+ }
+
+ @Test
+ public void shouldExposeOutOfProcessRemoteExceptions() throws RemoteException {
+ when(mockBinder.transact(anyInt(), any(), any(), anyInt())).thenThrow(RemoteException.class);
+ OutOfProcessImpl proxy = new OutOfProcessImpl(mockBinder);
+ try (ParcelHolder parcel = ParcelHolder.obtain()) {
+ assertThrows(RemoteException.class, () -> proxy.transact(123, parcel));
+ }
+ }
+
+ @Test
+ public void shouldIgnoreUnknownTransactionReturnValueInProcess() throws RemoteException {
+ when(mockBinder.transact(anyInt(), any(), any(), anyInt())).thenReturn(false);
+ InProcessImpl proxy = new InProcessImpl(mockBinder, queuingExecutor);
+ try (ParcelHolder parcel = ParcelHolder.obtain()) {
+ proxy.transact(123, parcel); // Doesn't throw.
+ verify(mockBinder, never()).transact(anyInt(), any(), any(), anyInt());
+ queuingExecutor.runAllQueued();
+ verify(mockBinder).transact(eq(123), any(), any(), anyInt());
+ }
+ }
+
+ @Test
+ public void shouldReportImpossibleUnknownTransactionReturnValueOutOfProcess()
+ throws RemoteException {
+ when(mockBinder.transact(anyInt(), any(), any(), anyInt())).thenReturn(false);
+ OutOfProcessImpl proxy = new OutOfProcessImpl(mockBinder);
+ try (ParcelHolder parcel = ParcelHolder.obtain()) {
+ assertThrows(RemoteException.class, () -> proxy.transact(123, parcel));
+ verify(mockBinder).transact(eq(123), any(), any(), anyInt());
+ }
+ }
+
+ /** An Executor that queues up Runnables for later manual execution by a unit test. */
+ static class QueuingExecutor implements Executor {
+ private final Queue