LineageOS/android_packages_modules_Connectivity
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Use the binder to identify keepalive in IConnectivityManager

Browse Source 
This is much simpler and less error-prone, as well as less
subject to race conditions.

It also allows for cleaning up some TODOs.

Test: FrameworksNetTests
      CtsNetTestCases
Bug: 267116236
Change-Id: I470c709446946ef35a0324427defe2f58b434339
This commit is contained in:
Chalard Jean
2023-02-03 22:11:20 +09:00
committed by chiachangwang
parent e3e9f5622b
commit f0b261e7cc
10 changed files with 24 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
framework/src/android/net/ConnectivityManager.java
View File

@@ -2279,23 +2279,12 @@ public class ConnectivityManager {
private final ISocketKeepaliveCallback mCallback; private final ISocketKeepaliveCallback mCallback;
private final ExecutorService mExecutor; private final ExecutorService mExecutor;
private volatile Integer mSlot;
@UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553) @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553)
public void stop() { public void stop() {
try { try {
mExecutor.execute(() -> { mExecutor.execute(() -> {
try { try {
if (mSlot != null) { mService.stopKeepalive(mCallback);
// TODO : this is incorrect, because in the presence of auto on/off
// keepalive the slot associated with this keepalive can have
// changed. Also, this actually causes another problem where some other
// app might stop your keepalive if it just knows the network and
// the slot and goes through the trouble of grabbing the aidl object.
// This code should use the callback to identify what keepalive to
// stop instead.
mService.stopKeepalive(mNetwork, mSlot);
}
} catch (RemoteException e) { } catch (RemoteException e) {
Log.e(TAG, "Error stopping packet keepalive: ", e); Log.e(TAG, "Error stopping packet keepalive: ", e);
throw e.rethrowFromSystemServer(); throw e.rethrowFromSystemServer();
@@ -2313,11 +2302,10 @@ public class ConnectivityManager {
mExecutor = Executors.newSingleThreadExecutor(); mExecutor = Executors.newSingleThreadExecutor();
mCallback = new ISocketKeepaliveCallback.Stub() { mCallback = new ISocketKeepaliveCallback.Stub() {
@Override @Override
public void onStarted(int slot) { public void onStarted() {
final long token = Binder.clearCallingIdentity(); final long token = Binder.clearCallingIdentity();
try { try {
mExecutor.execute(() -> { mExecutor.execute(() -> {
mSlot = slot;
callback.onStarted(); callback.onStarted();
}); });
} finally { } finally {
@@ -2330,7 +2318,6 @@ public class ConnectivityManager {
final long token = Binder.clearCallingIdentity(); final long token = Binder.clearCallingIdentity();
try { try {
mExecutor.execute(() -> { mExecutor.execute(() -> {
mSlot = null;
callback.onStopped(); callback.onStopped();
}); });
} finally { } finally {
@@ -2344,7 +2331,6 @@ public class ConnectivityManager {
final long token = Binder.clearCallingIdentity(); final long token = Binder.clearCallingIdentity();
try { try {
mExecutor.execute(() -> { mExecutor.execute(() -> {
mSlot = null;
callback.onError(error); callback.onError(error);
}); });
} finally { } finally {

2
framework/src/android/net/IConnectivityManager.aidl
View File

@@ -193,7 +193,7 @@ interface IConnectivityManager
void startTcpKeepalive(in Network network, in ParcelFileDescriptor pfd, int intervalSeconds, void startTcpKeepalive(in Network network, in ParcelFileDescriptor pfd, int intervalSeconds,
in ISocketKeepaliveCallback cb); in ISocketKeepaliveCallback cb);
void stopKeepalive(in Network network, int slot); void stopKeepalive(in ISocketKeepaliveCallback cb);
String getCaptivePortalServerUrl(); String getCaptivePortalServerUrl();

2
framework/src/android/net/ISocketKeepaliveCallback.aidl
View File

@@ -24,7 +24,7 @@ package android.net;
oneway interface ISocketKeepaliveCallback oneway interface ISocketKeepaliveCallback
{ {
/** The keepalive was successfully started. */ /** The keepalive was successfully started. */
void onStarted(int slot); void onStarted();
/** The keepalive was successfully stopped. */ /** The keepalive was successfully stopped. */
void onStopped(); void onStopped();
/** The keepalive was stopped because of an error. */ /** The keepalive was stopped because of an error. */

4
framework/src/android/net/NattSocketKeepalive.java
View File

@@ -91,9 +91,7 @@ public final class NattSocketKeepalive extends SocketKeepalive {
protected void stopImpl() { protected void stopImpl() {
mExecutor.execute(() -> { mExecutor.execute(() -> {
try { try {
if (mSlot != null) { mService.stopKeepalive(mCallback);
mService.stopKeepalive(mNetwork, mSlot);
}
} catch (RemoteException e) { } catch (RemoteException e) {
Log.e(TAG, "Error stopping socket keepalive: ", e); Log.e(TAG, "Error stopping socket keepalive: ", e);
throw e.rethrowFromSystemServer(); throw e.rethrowFromSystemServer();

4
framework/src/android/net/NetworkAgent.java
View File

@@ -291,7 +291,9 @@ public abstract class NetworkAgent {
/** /**
* Requests that the specified keepalive packet be stopped. * Requests that the specified keepalive packet be stopped.
* *
* arg1 = hardware slot number of the keepalive to stop. * arg1 = unused
* arg2 = error code (SUCCESS)
* obj = callback to identify the keepalive
* *
* Also used internally by ConnectivityService / KeepaliveTracker, with different semantics. * Also used internally by ConnectivityService / KeepaliveTracker, with different semantics.
* @hide * @hide

10
framework/src/android/net/SocketKeepalive.java
View File

@@ -21,7 +21,6 @@ import static android.annotation.SystemApi.Client.PRIVILEGED_APPS;
import android.annotation.IntDef; import android.annotation.IntDef;
import android.annotation.IntRange; import android.annotation.IntRange;
import android.annotation.NonNull; import android.annotation.NonNull;
import android.annotation.Nullable;
import android.annotation.SystemApi; import android.annotation.SystemApi;
import android.os.Binder; import android.os.Binder;
import android.os.ParcelFileDescriptor; import android.os.ParcelFileDescriptor;
@@ -249,9 +248,6 @@ public abstract class SocketKeepalive implements AutoCloseable {
@NonNull protected final Executor mExecutor; @NonNull protected final Executor mExecutor;
/** @hide */ /** @hide */
@NonNull protected final ISocketKeepaliveCallback mCallback; @NonNull protected final ISocketKeepaliveCallback mCallback;
// TODO: remove slot since mCallback could be used to identify which keepalive to stop.
/** @hide */
@Nullable protected Integer mSlot;
/** @hide */ /** @hide */
public SocketKeepalive(@NonNull IConnectivityManager service, @NonNull Network network, public SocketKeepalive(@NonNull IConnectivityManager service, @NonNull Network network,
@@ -263,11 +259,10 @@ public abstract class SocketKeepalive implements AutoCloseable {
mExecutor = executor; mExecutor = executor;
mCallback = new ISocketKeepaliveCallback.Stub() { mCallback = new ISocketKeepaliveCallback.Stub() {
@Override @Override
public void onStarted(int slot) { public void onStarted() {
final long token = Binder.clearCallingIdentity(); final long token = Binder.clearCallingIdentity();
try { try {
mExecutor.execute(() -> { mExecutor.execute(() -> {
mSlot = slot;
callback.onStarted(); callback.onStarted();
}); });
} finally { } finally {
@@ -280,7 +275,6 @@ public abstract class SocketKeepalive implements AutoCloseable {
final long token = Binder.clearCallingIdentity(); final long token = Binder.clearCallingIdentity();
try { try {
executor.execute(() -> { executor.execute(() -> {
mSlot = null;
callback.onStopped(); callback.onStopped();
}); });
} finally { } finally {
@@ -293,7 +287,6 @@ public abstract class SocketKeepalive implements AutoCloseable {
final long token = Binder.clearCallingIdentity(); final long token = Binder.clearCallingIdentity();
try { try {
executor.execute(() -> { executor.execute(() -> {
mSlot = null;
callback.onError(error); callback.onError(error);
}); });
} finally { } finally {
@@ -306,7 +299,6 @@ public abstract class SocketKeepalive implements AutoCloseable {
final long token = Binder.clearCallingIdentity(); final long token = Binder.clearCallingIdentity();
try { try {
executor.execute(() -> { executor.execute(() -> {
mSlot = null;
callback.onDataReceived(); callback.onDataReceived();
}); });
} finally { } finally {

4
framework/src/android/net/TcpSocketKeepalive.java
View File

@@ -69,9 +69,7 @@ public final class TcpSocketKeepalive extends SocketKeepalive {
protected void stopImpl() { protected void stopImpl() {
mExecutor.execute(() -> { mExecutor.execute(() -> {
try { try {
if (mSlot != null) { mService.stopKeepalive(mCallback);
mService.stopKeepalive(mNetwork, mSlot);
}
} catch (RemoteException e) { } catch (RemoteException e) {
Log.e(TAG, "Error stopping packet keepalive: ", e); Log.e(TAG, "Error stopping packet keepalive: ", e);
throw e.rethrowFromSystemServer(); throw e.rethrowFromSystemServer();

17
service/src/com/android/server/ConnectivityService.java
View File

@@ -5577,14 +5577,14 @@ public class ConnectivityService extends IConnectivityManager.Stub
} }
// Sent by KeepaliveTracker to process an app request on the state machine thread. // Sent by KeepaliveTracker to process an app request on the state machine thread.
case NetworkAgent.CMD_STOP_SOCKET_KEEPALIVE: { case NetworkAgent.CMD_STOP_SOCKET_KEEPALIVE: {
NetworkAgentInfo nai = getNetworkAgentInfoForNetwork((Network) msg.obj); final AutomaticOnOffKeepalive ki = mKeepaliveTracker.getKeepaliveForBinder(
if (nai == null) { (IBinder) msg.obj);
Log.e(TAG, "Attempt to stop keepalive on nonexistent network"); if (ki == null) {
Log.e(TAG, "Attempt to stop an already stopped keepalive");
return; return;
} }
int slot = msg.arg1; final int reason = msg.arg2;
int reason = msg.arg2; mKeepaliveTracker.handleStopKeepalive(ki, reason);
mKeepaliveTracker.handleStopKeepalive(nai, slot, reason);
break; break;
} }
case EVENT_REPORT_NETWORK_CONNECTIVITY: { case EVENT_REPORT_NETWORK_CONNECTIVITY: {
@@ -9865,9 +9865,10 @@ public class ConnectivityService extends IConnectivityManager.Stub
} }
@Override @Override
public void stopKeepalive(Network network, int slot) { public void stopKeepalive(@NonNull final ISocketKeepaliveCallback cb) {
mHandler.sendMessage(mHandler.obtainMessage( mHandler.sendMessage(mHandler.obtainMessage(
NetworkAgent.CMD_STOP_SOCKET_KEEPALIVE, slot, SocketKeepalive.SUCCESS, network)); NetworkAgent.CMD_STOP_SOCKET_KEEPALIVE, 0, SocketKeepalive.SUCCESS,
Objects.requireNonNull(cb).asBinder()));
} }
@Override @Override

35
service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java
View File

@@ -329,30 +329,6 @@ public class AutomaticOnOffKeepaliveTracker {
handleResumeKeepalive(newKi); handleResumeKeepalive(newKi);
} }
// TODO : this method should be removed ; the keepalives should always be indexed by callback
private int findAutomaticOnOffKeepaliveIndex(@NonNull Network network, int slot) {
ensureRunningOnHandlerThread();
int index = 0;
for (AutomaticOnOffKeepalive ki : mAutomaticOnOffKeepalives) {
if (ki.match(network, slot)) {
return index;
}
index++;
}
return -1;
}
// TODO : this method should be removed ; the keepalives should always be indexed by callback
@Nullable
private AutomaticOnOffKeepalive findAutomaticOnOffKeepalive(@NonNull Network network,
int slot) {
ensureRunningOnHandlerThread();
final int index = findAutomaticOnOffKeepaliveIndex(network, slot);
return (index >= 0) ? mAutomaticOnOffKeepalives.get(index) : null;
}
/** /**
* Find the AutomaticOnOffKeepalive associated with a given callback. * Find the AutomaticOnOffKeepalive associated with a given callback.
* @return the keepalive associated with this callback, or null if none * @return the keepalive associated with this callback, or null if none
@@ -415,17 +391,12 @@ public class AutomaticOnOffKeepaliveTracker {
/** /**
* Handle stop keepalives on the specific network with given slot. * Handle stop keepalives on the specific network with given slot.
*/ */
public void handleStopKeepalive(@NonNull NetworkAgentInfo nai, int slot, int reason) { public void handleStopKeepalive(@NonNull final AutomaticOnOffKeepalive autoKi, int reason) {
final AutomaticOnOffKeepalive autoKi = findAutomaticOnOffKeepalive(nai.network, slot);
if (null == autoKi) {
Log.e(TAG, "Attempt to stop nonexistent keepalive " + slot + " on " + nai);
return;
}
// Stop the keepalive unless it was suspended. This includes the case where it's managed // Stop the keepalive unless it was suspended. This includes the case where it's managed
// but enabled, and the case where it's always on. // but enabled, and the case where it's always on.
if (autoKi.mAutomaticOnOffState != STATE_SUSPENDED) { if (autoKi.mAutomaticOnOffState != STATE_SUSPENDED) {
mKeepaliveTracker.handleStopKeepalive(nai, slot, reason); final KeepaliveTracker.KeepaliveInfo ki = autoKi.mKi;
mKeepaliveTracker.handleStopKeepalive(ki.getNai(), ki.getSlot(), reason);
} }
cleanupAutoOnOffKeepalive(autoKi); cleanupAutoOnOffKeepalive(autoKi);

4
service/src/com/android/server/connectivity/KeepaliveTracker.java
View File

@@ -589,9 +589,9 @@ public class KeepaliveTracker {
Log.d(TAG, "Started keepalive " + slot + " on " + nai.toShortString()); Log.d(TAG, "Started keepalive " + slot + " on " + nai.toShortString());
ki.mStartedState = KeepaliveInfo.STARTED; ki.mStartedState = KeepaliveInfo.STARTED;
try { try {
ki.mCallback.onStarted(slot); ki.mCallback.onStarted();
} catch (RemoteException e) { } catch (RemoteException e) {
Log.w(TAG, "Discarded onStarted(" + slot + ") callback"); Log.w(TAG, "Discarded onStarted callback");
} }
} else { } else {
Log.d(TAG, "Failed to start keepalive " + slot + " on " + nai.toShortString() Log.d(TAG, "Failed to start keepalive " + slot + " on " + nai.toShortString()