Commit fe0f89f8 authored by Bo Liu's avatar Bo Liu Committed by Commit Bot

android: Check classloader in bindToCaller

There is a situation where chromium java code is loaded into different
class loaders in the same process. The expectation is to avoid sharing
renderer processes between the two class loaders. So pass a string
representation of the class loader into BindToCaller and check it along
with the pid.

Bug: 1051358
Change-Id: I8147582026549e7a4749f63d1941bd47e7d67ce7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2051531
Commit-Queue: Bo <boliu@chromium.org>
Reviewed-by: default avatarRobert Sesek <rsesek@chromium.org>
Reviewed-by: default avatarRichard Coles <torne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745540}
parent 2f702ab1
...@@ -83,6 +83,17 @@ public class ChildProcessConnection { ...@@ -83,6 +83,17 @@ public class ChildProcessConnection {
return BindService.supportVariableConnections(); return BindService.supportVariableConnections();
} }
/**
* The string passed to bindToCaller to identify this class loader.
*/
@VisibleForTesting
public static String getBindToCallerClazz() {
// TODO(crbug.com/1057102): Have embedder explicitly set separate different strings since
// this could still collide in theory.
ClassLoader cl = ChildProcessConnection.class.getClassLoader();
return cl.toString() + cl.hashCode();
}
/** /**
* Delegate that ChildServiceConnection should call when the service connects/disconnects. * Delegate that ChildServiceConnection should call when the service connects/disconnects.
* These callbacks are expected to happen on a background thread. * These callbacks are expected to happen on a background thread.
...@@ -529,7 +540,7 @@ public class ChildProcessConnection { ...@@ -529,7 +540,7 @@ public class ChildProcessConnection {
if (mBindToCaller) { if (mBindToCaller) {
try { try {
if (!mService.bindToCaller()) { if (!mService.bindToCaller(getBindToCallerClazz())) {
if (mServiceCallback != null) { if (mServiceCallback != null) {
mServiceCallback.onChildStartFailed(this); mServiceCallback.onChildStartFailed(this);
} }
......
...@@ -15,6 +15,7 @@ import android.os.Looper; ...@@ -15,6 +15,7 @@ import android.os.Looper;
import android.os.Parcelable; import android.os.Parcelable;
import android.os.Process; import android.os.Process;
import android.os.RemoteException; import android.os.RemoteException;
import android.text.TextUtils;
import android.util.SparseArray; import android.util.SparseArray;
import org.chromium.base.BaseSwitches; import org.chromium.base.BaseSwitches;
...@@ -77,6 +78,8 @@ public class ChildProcessService { ...@@ -77,6 +78,8 @@ public class ChildProcessService {
// PID of the client of this service, set in bindToCaller(), if mBindToCallerCheck is true. // PID of the client of this service, set in bindToCaller(), if mBindToCallerCheck is true.
@GuardedBy("mBinderLock") @GuardedBy("mBinderLock")
private int mBoundCallingPid; private int mBoundCallingPid;
@GuardedBy("mBinderLock")
private String mBoundCallingClazz;
// This is the native "Main" thread for the renderer / utility process. // This is the native "Main" thread for the renderer / utility process.
private Thread mMainThread; private Thread mMainThread;
...@@ -108,17 +111,22 @@ public class ChildProcessService { ...@@ -108,17 +111,22 @@ public class ChildProcessService {
private final IChildProcessService.Stub mBinder = new IChildProcessService.Stub() { private final IChildProcessService.Stub mBinder = new IChildProcessService.Stub() {
// NOTE: Implement any IChildProcessService methods here. // NOTE: Implement any IChildProcessService methods here.
@Override @Override
public boolean bindToCaller() { public boolean bindToCaller(String clazz) {
assert mBindToCallerCheck; assert mBindToCallerCheck;
assert mServiceBound; assert mServiceBound;
synchronized (mBinderLock) { synchronized (mBinderLock) {
int callingPid = Binder.getCallingPid(); int callingPid = Binder.getCallingPid();
if (mBoundCallingPid == 0) { if (mBoundCallingPid == 0 && mBoundCallingClazz == null) {
mBoundCallingPid = callingPid; mBoundCallingPid = callingPid;
mBoundCallingClazz = clazz;
} else if (mBoundCallingPid != callingPid) { } else if (mBoundCallingPid != callingPid) {
Log.e(TAG, "Service is already bound by pid %d, cannot bind for pid %d", Log.e(TAG, "Service is already bound by pid %d, cannot bind for pid %d",
mBoundCallingPid, callingPid); mBoundCallingPid, callingPid);
return false; return false;
} else if (!TextUtils.equals(mBoundCallingClazz, clazz)) {
Log.w(TAG, "Service is already bound by %s, cannot bind for %s",
mBoundCallingClazz, clazz);
return false;
} }
} }
return true; return true;
......
...@@ -9,10 +9,11 @@ import android.os.Bundle; ...@@ -9,10 +9,11 @@ import android.os.Bundle;
import org.chromium.base.process_launcher.IParentProcess; import org.chromium.base.process_launcher.IParentProcess;
interface IChildProcessService { interface IChildProcessService {
// |clazz| identifies the ClassLoader of the caller.
// On the first call to this method, the service will record the calling PID // On the first call to this method, the service will record the calling PID
// and return true. Subsequent calls will only return true if the calling PID // and |clazz| and return true. Subsequent calls will only return true if the
// is the same as the recorded one. // calling PID and |clazz| matches the recorded values.
boolean bindToCaller(); boolean bindToCaller(in String clazz);
// Sets up the initial IPC channel. // Sets up the initial IPC channel.
oneway void setupConnection(in Bundle args, IParentProcess parentProcess, oneway void setupConnection(in Bundle args, IParentProcess parentProcess,
......
...@@ -281,7 +281,7 @@ public class ChildProcessConnectionTest { ...@@ -281,7 +281,7 @@ public class ChildProcessConnectionTest {
verify(mServiceCallback, times(1)).onChildStarted(); verify(mServiceCallback, times(1)).onChildStarted();
verify(mServiceCallback, never()).onChildStartFailed(any()); verify(mServiceCallback, never()).onChildStartFailed(any());
verify(mServiceCallback, never()).onChildProcessDied(connection); verify(mServiceCallback, never()).onChildProcessDied(connection);
verify(mIChildProcessService, never()).bindToCaller(); verify(mIChildProcessService, never()).bindToCaller(any());
} }
@Test @Test
...@@ -290,13 +290,13 @@ public class ChildProcessConnectionTest { ...@@ -290,13 +290,13 @@ public class ChildProcessConnectionTest {
false /* bindAsExternalService */, null /* serviceBundle */); false /* bindAsExternalService */, null /* serviceBundle */);
assertNotNull(mFirstServiceConnection); assertNotNull(mFirstServiceConnection);
connection.start(false /* useStrongBinding */, mServiceCallback); connection.start(false /* useStrongBinding */, mServiceCallback);
when(mIChildProcessService.bindToCaller()).thenReturn(true); when(mIChildProcessService.bindToCaller(any())).thenReturn(true);
mFirstServiceConnection.notifyServiceConnected(mChildProcessServiceBinder); mFirstServiceConnection.notifyServiceConnected(mChildProcessServiceBinder);
// Service is started and bindToCallback is called. // Service is started and bindToCallback is called.
verify(mServiceCallback, times(1)).onChildStarted(); verify(mServiceCallback, times(1)).onChildStarted();
verify(mServiceCallback, never()).onChildStartFailed(any()); verify(mServiceCallback, never()).onChildStartFailed(any());
verify(mServiceCallback, never()).onChildProcessDied(connection); verify(mServiceCallback, never()).onChildProcessDied(connection);
verify(mIChildProcessService, times(1)).bindToCaller(); verify(mIChildProcessService, times(1)).bindToCaller(any());
} }
@Test @Test
...@@ -307,13 +307,13 @@ public class ChildProcessConnectionTest { ...@@ -307,13 +307,13 @@ public class ChildProcessConnectionTest {
connection.start(false /* useStrongBinding */, mServiceCallback); connection.start(false /* useStrongBinding */, mServiceCallback);
// Pretend bindToCaller returns false, i.e. the service is already bound to a different // Pretend bindToCaller returns false, i.e. the service is already bound to a different
// service. // service.
when(mIChildProcessService.bindToCaller()).thenReturn(false); when(mIChildProcessService.bindToCaller(any())).thenReturn(false);
mFirstServiceConnection.notifyServiceConnected(mChildProcessServiceBinder); mFirstServiceConnection.notifyServiceConnected(mChildProcessServiceBinder);
// Service fails to start. // Service fails to start.
verify(mServiceCallback, never()).onChildStarted(); verify(mServiceCallback, never()).onChildStarted();
verify(mServiceCallback, times(1)).onChildStartFailed(any()); verify(mServiceCallback, times(1)).onChildStartFailed(any());
verify(mServiceCallback, never()).onChildProcessDied(connection); verify(mServiceCallback, never()).onChildProcessDied(connection);
verify(mIChildProcessService, times(1)).bindToCaller(); verify(mIChildProcessService, times(1)).bindToCaller(any());
} }
@Test @Test
...@@ -466,7 +466,7 @@ public class ChildProcessConnectionTest { ...@@ -466,7 +466,7 @@ public class ChildProcessConnectionTest {
public void testUpdateGroupImportanceSmoke() throws RemoteException { public void testUpdateGroupImportanceSmoke() throws RemoteException {
ChildProcessConnection connection = createDefaultTestConnection(); ChildProcessConnection connection = createDefaultTestConnection();
connection.start(false /* useStrongBinding */, null /* serviceCallback */); connection.start(false /* useStrongBinding */, null /* serviceCallback */);
when(mIChildProcessService.bindToCaller()).thenReturn(true); when(mIChildProcessService.bindToCaller(any())).thenReturn(true);
mFirstServiceConnection.notifyServiceConnected(mChildProcessServiceBinder); mFirstServiceConnection.notifyServiceConnected(mChildProcessServiceBinder);
connection.updateGroupImportance(1, 2); connection.updateGroupImportance(1, 2);
assertEquals(1, connection.getGroup()); assertEquals(1, connection.getGroup());
......
...@@ -191,8 +191,8 @@ public class ChildProcessLauncherHelperTest { ...@@ -191,8 +191,8 @@ public class ChildProcessLauncherHelperTest {
}); });
Assert.assertTrue(ChildProcessLauncherTestUtils.getConnectionPid(retryConnection) Assert.assertTrue(ChildProcessLauncherTestUtils.getConnectionPid(retryConnection)
!= helperConnectionPid); != helperConnectionPid);
Assert.assertTrue( Assert.assertTrue(ChildProcessLauncherTestUtils.getConnectionService(retryConnection)
ChildProcessLauncherTestUtils.getConnectionService(retryConnection).bindToCaller()); .bindToCaller(ChildProcessConnection.getBindToCallerClazz()));
// Unbind the service. // Unbind the service.
replyHandler.mMessage = null; replyHandler.mMessage = null;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment